Bug 25973 - Avoid calling WTF::CurrentThread() from thread-local destructors in Chromium on OSX
: Avoid calling WTF::CurrentThread() from thread-local destructors in Chromium ...
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: Macintosh Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-05-22 18:14 PST by
Modified: 2009-06-01 12:58 PST (History)


Attachments
Proposed patch (1.26 KB, patch)
2009-05-22 18:19 PST, Dmitry Titov
eric: review-
Review Patch | Details | Formatted Diff | Diff
Updated patch. (3.10 KB, patch)
2009-05-26 16:37 PST, Dmitry Titov
no flags Review Patch | Details | Formatted Diff | Diff
Updated according to Michael's comments (3.08 KB, patch)
2009-05-29 16:03 PST, Dmitry Titov
darin: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-05-22 18:14:09 PST
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.
------- Comment #1 From 2009-05-22 18:19:03 PST -------
Created an attachment (id=30601) [details]
Proposed patch

Note the non-Chromium OSX pthread builds are fine since isMainThread is not calling CurrentThread on those.
------- Comment #2 From 2009-05-22 19:46:55 PST -------
(From update of attachment 30601 [details])
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.
------- Comment #3 From 2009-05-26 16:37:52 PST -------
Created an attachment (id=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.
------- Comment #4 From 2009-05-29 13:03:28 PST -------
> 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?
------- Comment #5 From 2009-05-29 13:57:06 PST -------
(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.
------- Comment #6 From 2009-05-29 15:21:00 PST -------
This looks like a good proximate fix to the problem your bumping into. Much less ripple than revamping what ThreadIdentifiers are.
------- Comment #7 From 2009-05-29 16:03:22 PST -------
Created an attachment (id=30794) [details]
Updated according to Michael's comments

I've also added #ifndef NDEBUG around a boolean that is only used in the ASSERT.
------- Comment #8 From 2009-05-30 14:10:57 PST -------
(From update of attachment 30794 [details])
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.
------- Comment #9 From 2009-06-01 12:58:03 PST -------
Landed: http://trac.webkit.org/changeset/44327 and http://trac.webkit.org/changeset/44328