Bug 25973 – Avoid calling WTF::CurrentThread() from thread-local destructors in Chromium on OSX

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Dmitry Titov]

Due to bug 25348, which is still waiting for a solution, the workers in Chromium on OSX (pthreads) trip ASSERT in JavaScripCore/wtf/ThreadingPthreads.cpp, in function establishIdentifierForPthreadHandle() because Chromium variant of isMainThread uses CurrentThread(0 which inserts a thread id into threading map, and if called from thread-specific destructor, this happens after the thread is detached.

Long story short, on Chromium we should stop calling IsMainThread(), and since the only place it is called now is ~ThreadGlobalData() inside ASSERT, I'm disabling the ASSERT.

[Dmitry Titov]

Created attachment 30601 [details]
Proposed patch

Note the non-Chromium OSX pthread builds are fine since isMainThread is not calling CurrentThread on those.

[Eric Seidel (no email)]

Comment on attachment 30601 [details]
Proposed patch

Can this be tested via a layout test?

Why is it correct for chromium darwin to call CurrnetThread()?

Also some of your description from the bug belongs in teh changlog.

And a comment around your ifdef explaining what it's there for would help.

r- for the lack of test (or explanation why no test possible), and above issues.

[Dmitry Titov]

Created attachment 30686 [details]
Updated patch.

A better patch and a bit better attempt to explain the change:

Pthreads invoke thread-specific destructors after WTF::detachThread() is called and ThreadIdentifier
for the thread removed from the WTF thread map. Calling CurrentThread() in such destructor causes
the ThreadIdentifier to be re-created and inserted into the map again. Since Pthreads on OSX reuse
the pthread_t between threads, the next created thread will have the same pthread_t and cause an assert in establishIdentifierForPthreadHandle() since the id is already in the map.

The change removes the call to isMainThread() which internally calls CurrentThread() in OSX Chromium port, by storing the result of isMainThread() in a member variable of thread-specific data during construction. Since destructor of thread-specific data is always called on its 'own' thread, the change preserves the previous behavior but fixes the aforementioned problem.

The behavior is covered by existing test LayoutTests/fast/workers/worker-terminate.html, which currently fails on OSX Chromium and will stop failing after this change.

[Michael Nordman]

> Note the non-Chromium OSX pthread builds are fine since isMainThread is not
> calling CurrentThread on those.

Why are chromium OSX builds different in this regard?

* maybe rename m_mainThread to m_isMainThread for clarity

* should some kind of debug runtime check be introduced into CurrentThread()
  (or perhaps something more specific like establishIdentifierForPthreadHandle)
  to complain about re-establishing a threadid at TLS dtor time?

[Dmitry Titov]

(In reply to comment #4)
> > Note the non-Chromium OSX pthread builds are fine since isMainThread is not
> > calling CurrentThread on those.
> 
> Why are chromium OSX builds different in this regard?

Because WTF::isMainThread() (in ThreadingPthreads.cpp) is different for Chromium (it has #ifdef CHROMIUM inside): Chromium treats the thread that first called InitializeThreading as the main one (because in a renderer process, the WebKit is running not on the main thread) rather then just use pthread_main_np() - so the Chromium codepath calls CurrentThread() inside.

> * maybe rename m_mainThread to m_isMainThread for clarity

cool idea, will do.

> * should some kind of debug runtime check be introduced into CurrentThread()
>   (or perhaps something more specific like establishIdentifierForPthreadHandle)
>   to complain about re-establishing a threadid at TLS dtor time?

I thought about it for a while, working on bug 25348. I am not sure there is a way to construct such check. Essentially, inside CurrentThread() we would need to figure out that the caller stack has a tls dtor on it. Given that such dtor is just a registered function, I'm not sure how to figure that out. We could wrap most of threading WTF API into some class and then provide a virtual that would be a 'tls dtor' so we could know when we are 'in tls dtor' but this is more work and likely should be done (if at all) as a fix for bug 25348. Or we could have something like 'dying threads list' with pthread_t stored in it but the problem is we don't know when to remove a thread from such a list.
So, I'm totally open for an idea how to make such check :-)

Meanwhile, in worker OSX Chromium case, the existing ASSERT  in establishIdentifierForPthreadHandle() fires when a new thread is created but the pthread_t is established by the previous thread (OSX pthreads reuse pthread_t), so we need this fix to enable workers on OSX Chromium.

[Michael Nordman]

This looks like a good proximate fix to the problem your bumping into. Much less ripple than revamping what ThreadIdentifiers are.

[Dmitry Titov]

Created attachment 30794 [details]
Updated according to Michael's comments

I've also added #ifndef NDEBUG around a boolean that is only used in the ASSERT.

[Darin Adler]

Comment on attachment 30794 [details]
Updated according to Michael's comments

Seems fine.

I think you want a comment in ~ThreadGlobalData explaining why you can't use isMainThread() at this point, since people working on platforms other than Chromium might try to "clean this up" and re-break it if there's no comment.

[Dmitry Titov]

Landed: http://trac.webkit.org/changeset/44327 and http://trac.webkit.org/changeset/44328