I don't really have a high level view of the various threads / thread lifecycles in Blink. If we have a lot of thread churn this could be an issue but I don't think we do that and instead opt to have a few long lived threads.

Do the values in the TLS get cleaned up? Are you saying we're leaking the slots in the pthread system or the objects?

Both, I think (on Windows). (1) is for the objects, (2) for the slots.

Hmm, so doing:

for (var i = 0; i < 10000; ++i) {
  let worker = new Worker(...);
  worker.postMessage(...);
}

would cause a constant memory leak? That seems bad for ServiceWorker where we intentionally start/stop it repeatedly.

Yeah, I was assuming that TLS of non-main threads is cleaned up. Otherwise, it will leak objects as Elliott pointed out.

I added code to intentionally leak TLS of the main thread a couple of months ago (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/wtf/ThreadSpecific.h?q=threadspecific&sq=package:chromium&dr=CSs&l=239), but it's only for the main thread. TLS of other threads should be cleaned up.

It looks like the example in #5 is very similar to issue 502298.

Hm yeah, I agree that this should be fixed.

(Side note: thread-local storage implementations on Windows and on Mac are
known to be slow. How about using C++11 standard thread_local? Is there any
known deficiencies? Can we migrate ThreadSpecific to thread_local?)

(Worth a try, but if the underlying OS primitive is slow, then the C++11 thread_local will likely also be slow.)

I did some experiments with thread_local, and encountered significant issues.

While it's awesome on some platforms, on Mac it was torn down before all of thread shutdown completed (so if some destructors accessed the atomic string table, which we currently support -- we just tear it down again afterwards, it would fail -- and by fail, I mean crash the process).

Infuriatingly, this is true even for plain old data, which shouldn't need destruction at all (and could just live in the thread segment until the thread really dies).

There's an optimization inside ThreadSpecific to avoid TLS entirely if you're on the main thread, thread_local can't make assumptions about stack addresses like that.

It's unfortunate that thread_local isn't fast on _all_ our platforms.

e.g. on Linux (with suitable build configuration), it's just an offset from a register (which is super fast and super small), and the main-thread hack we use is totally unnecessary.

But some of our platforms still have caveats that getting consistently fast TLS everywhere is hard. :(

1. Agreed that it looks like we aren't destroying TLS properly on Windows (!).

2. I don't think we should be calling ~ThreadSpecific on thread exit. It destroys the TLS slot (which is process-global state), and it should only be called when no thread will ever access that TLS slot again. (For most of our thread-specific variables, that is "when the process exits", so we just leak the TLS slot.)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e87cffa68c3d964c61909b2ea31603ac05cf6af2

commit e87cffa68c3d964c61909b2ea31603ac05cf6af2
Author: erikchen <erikchen@chromium.org>
Date: Fri Mar 30 18:15:40 2018

Update ThreadSpecific to use base::ThreadLocalStorage.

The current implementation has two problems:

1) It can call into base::TLS after the destructor of base::TLS has been
invoked. This is highly undesirable, as it will cause extra, unnecessary work on
POSIX, and causes memory leaks on Windows.

Technically this isn't an issue on Windows, but only because
ThreadSpecific::Destroy isn't called, which in itself is a bug that causes
memory leaks. See next point.

2) ThreadSpecific::Destroy isn't called on Windows, resulting in memory leaks.
See https://bugs.chromium.org/p/chromium/issues/detail?id=718110.

Bug: 825218,  826397 , 718110
Change-Id: I88f383621abb4c7fe904688a314afe06e8f1e5b0
Reviewed-on: https://chromium-review.googlesource.com/984522
Commit-Queue: Erik Chen <erikchen@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547209}
[modify] https://crrev.com/e87cffa68c3d964c61909b2ea31603ac05cf6af2/third_party/WebKit/Source/platform/wtf/BUILD.gn
[modify] https://crrev.com/e87cffa68c3d964c61909b2ea31603ac05cf6af2/third_party/WebKit/Source/platform/wtf/DEPS
[modify] https://crrev.com/e87cffa68c3d964c61909b2ea31603ac05cf6af2/third_party/WebKit/Source/platform/wtf/ThreadSpecific.h
[delete] https://crrev.com/34cee7038e43d6537a80b5f4aa311b3b783b13b2/third_party/WebKit/Source/platform/wtf/ThreadSpecificWin.cpp
[modify] https://crrev.com/e87cffa68c3d964c61909b2ea31603ac05cf6af2/third_party/WebKit/Source/platform/wtf/ThreadingPthreads.cpp
[modify] https://crrev.com/e87cffa68c3d964c61909b2ea31603ac05cf6af2/third_party/WebKit/Source/platform/wtf/ThreadingWin.cpp