[WTF] Make isMainThread more reliable

[Yusuke Suzuki]

[WTF] Make isMainThread more reliable

[Yusuke Suzuki]

Created attachment 350501 [details]
Patch

[Yusuke Suzuki]

Created attachment 350503 [details]
Patch

[Don Olmstead]

Comment on attachment 350503 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=350503&action=review

Informal r+.

I'm just wondering about the additional symbol check. Its probably good to have those sorts of checks to be absolutely certain. I'm not sure how standardized that header is.

> Source/WTF/wtf/generic/MainThreadGeneric.cpp:33
> +#include <pthread.h>

Is this really generic anymore if it relies on pthread.h being present?

> Source/cmake/OptionsCommon.cmake:137
> +WEBKIT_CHECK_HAVE_SYMBOL(HAVE_PTHREAD_MAIN_NP pthread_main_np pthread_np.h)

There's already a check for whether pthread_np.h is present. Is there a case where pthread_main_np would not be present in that scenario?

[Yusuke Suzuki]

Comment on attachment 350503 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=350503&action=review

>> Source/WTF/wtf/generic/MainThreadGeneric.cpp:33
>> +#include <pthread.h>
> 
> Is this really generic anymore if it relies on pthread.h being present?

We only support two types of thresding models: Windows and Pthreads. Since MainThreadWin.cpp is used for Windows, we can say this requires pthread.h.

>> Source/cmake/OptionsCommon.cmake:137
>> +WEBKIT_CHECK_HAVE_SYMBOL(HAVE_PTHREAD_MAIN_NP pthread_main_np pthread_np.h)
> 
> There's already a check for whether pthread_np.h is present. Is there a case where pthread_main_np would not be present in that scenario?

This code checks pthread_main_np carefully since pthread_np just meams pthread NonPortable. It is not guaranteed that pthread_main_np is included in pthread_np.h when pthread_np.h exists.

[Ross Kirsling]

Thank you for doing this, Yusuke!

Unfortunately, it seems that WinCairo JSC tests still get stuck in Release mode, with all eight subprocesses sitting on this line:
https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/jsc.cpp#L172

The stack trace before this patch looked like:
> [External Code]
> [Inline Frame] WTF.dll!std::_Cnd_waitX(_Cnd_internal_imp_t *)
> [Inline Frame] WTF.dll!std::condition_variable::wait(std::unique_lock<std::mutex> &)
> WTF.dll!WTF::WordLock::lockSlow()
> [Inline Frame] WTF.dll!WTF::WordLock::lock()
> [Inline Frame] WTF.dll!WTF::Locker<WTF::WordLock>::lock()
> [Inline Frame] WTF.dll!WTF::Locker<WTF::WordLock>::{ctor}(WTF::WordLock &)
> [Inline Frame] WTF.dll!WTF::holdLock(WTF::WordLock &)
> WTF.dll!WTF::Thread::didExit()
> WTF.dll!WTF::Thread::destructTLS(void * data)
> [External Code]
> jscLib.dll!`anonymous namespace'::jscExit(int status)
> jscLib.dll!main(int argc, char * * argv)
> jscLib.dll!dllLauncherEntryPoint(int argc, const char * * argv)
> jsc.exe!main(int argc, const char * * argv)
> [External Code]

After this patch, it's just the last six lines (i.e. nothing WTF-related appearing).

I'll see whether Debug is different...

[Ross Kirsling]

Hmm, Debug mode completed successfully (for anyone curious: ~8.5 hours, 141 failures).

Will retry Release once more...

[Yusuke Suzuki]

(In reply to Ross Kirsling from comment #6)
> Hmm, Debug mode completed successfully (for anyone curious: ~8.5 hours, 141
> failures).
> 
> Will retry Release once more...

My guess is that std::mutex implementation in Windowsis broken if it is used in TLS destructor. We can avoid this problem if we use spinlock here instead.

Anyway, this patch also touch TLS in TLS destructor. So in addition to this patch, spinlock patch would be requied.

[Yusuke Suzuki]

(In reply to Ross Kirsling from comment #5)
> Thank you for doing this, Yusuke!
> 
> Unfortunately, it seems that WinCairo JSC tests still get stuck in Release
> mode, with all eight subprocesses sitting on this line:
> https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/jsc.
> cpp#L172
> 
> The stack trace before this patch looked like:
> > [External Code]
> > [Inline Frame] WTF.dll!std::_Cnd_waitX(_Cnd_internal_imp_t *)
> > [Inline Frame] WTF.dll!std::condition_variable::wait(std::unique_lock<std::mutex> &)
> > WTF.dll!WTF::WordLock::lockSlow()
> > [Inline Frame] WTF.dll!WTF::WordLock::lock()
> > [Inline Frame] WTF.dll!WTF::Locker<WTF::WordLock>::lock()
> > [Inline Frame] WTF.dll!WTF::Locker<WTF::WordLock>::{ctor}(WTF::WordLock &)
> > [Inline Frame] WTF.dll!WTF::holdLock(WTF::WordLock &)
> > WTF.dll!WTF::Thread::didExit()
> > WTF.dll!WTF::Thread::destructTLS(void * data)
> > [External Code]
> > jscLib.dll!`anonymous namespace'::jscExit(int status)
> > jscLib.dll!main(int argc, char * * argv)
> > jscLib.dll!dllLauncherEntryPoint(int argc, const char * * argv)
> > jsc.exe!main(int argc, const char * * argv)
> > [External Code]
> 
> After this patch, it's just the last six lines (i.e. nothing WTF-related
> appearing).
> 
> I'll see whether Debug is different...

And I want to know that whether this `isMainThread()` is working well.
Is this `holdLock()` called inside `if (shouldRemoveThreadFromThreadGroup())` clause?
Or just before `m_didExit = true;`? If the above problem is caused in the latter place, we can say that updated `isMainThread()` seems working as we expected.

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #7)
> (In reply to Ross Kirsling from comment #6)
> > Hmm, Debug mode completed successfully (for anyone curious: ~8.5 hours, 141
> > failures).
> > 
> > Will retry Release once more...
> 
> My guess is that std::mutex implementation in Windowsis broken if it is used
> in TLS destructor. We can avoid this problem if we use spinlock here instead.
> 
> Anyway, this patch also touch TLS in TLS destructor. So in addition to this
> patch, spinlock patch would be requied.

Maybe, the sampling profiler or the other thread taking the mutex, and destroyed by the system. So waiting forever happens.
The way to avoid this is simple, we should move this holdLock code inside `if (shouldRemoveThreadFromThreadGroup())`.

[Yusuke Suzuki]

Created attachment 350560 [details]
Patch

[Ross Kirsling]

Oops, I had dropped the line numbers from that stack trace. This is the holdLock() call:
https://github.com/WebKit/webkit/blob/master/Source/WTF/wtf/Threading.cpp#L200
So then shouldRemoveThreadFromThreadGroup() definitely seems to be returning `true` for the main thread in a certain scenario (without your patch).

I confirmed that the first patch here doesn't work in Release mode, but I'll try the new one now. :)

[Ross Kirsling]

(In reply to Ross Kirsling from comment #11)
> Oops, I had dropped the line numbers from that stack trace. This is the
> holdLock() call:
> https://github.com/WebKit/webkit/blob/master/Source/WTF/wtf/Threading.
> cpp#L200
> So then shouldRemoveThreadFromThreadGroup() definitely seems to be returning
> `true` for the main thread in a certain scenario (without your patch).
> 
> I confirmed that the first patch here doesn't work in Release mode, but I'll
> try the new one now. :)

Ugh, I can't believe it. It managed to start running every single test, but at the very end, two subprocesses are hanging in the usual location (jsc.cpp line 172) and so the runner can't exit.

[Yusuke Suzuki]

(In reply to Ross Kirsling from comment #12)
> (In reply to Ross Kirsling from comment #11)
> > Oops, I had dropped the line numbers from that stack trace. This is the
> > holdLock() call:
> > https://github.com/WebKit/webkit/blob/master/Source/WTF/wtf/Threading.
> > cpp#L200
> > So then shouldRemoveThreadFromThreadGroup() definitely seems to be returning
> > `true` for the main thread in a certain scenario (without your patch).
> > 
> > I confirmed that the first patch here doesn't work in Release mode, but I'll
> > try the new one now. :)
> 
> Ugh, I can't believe it. It managed to start running every single test, but
> at the very end, two subprocesses are hanging in the usual location (jsc.cpp
> line 172) and so the runner can't exit.

Where is the hanging location? And does that mean isMainThread is not working well even with this patch?
Also, I would like to know,

1. what is the value stored in `mainThread` variable in MainThreadWin.cpp
2. what is the returned value from Thread::currentID() in such situation?
3. Is GetCurrentThreadId broken in TLS destructor?
4. Is TLS destructor called in the main thread?

[Ross Kirsling]

(In reply to Yusuke Suzuki from comment #13)
> (In reply to Ross Kirsling from comment #12)
> > (In reply to Ross Kirsling from comment #11)
> > > Oops, I had dropped the line numbers from that stack trace. This is the
> > > holdLock() call:
> > > https://github.com/WebKit/webkit/blob/master/Source/WTF/wtf/Threading.
> > > cpp#L200
> > > So then shouldRemoveThreadFromThreadGroup() definitely seems to be returning
> > > `true` for the main thread in a certain scenario (without your patch).
> > > 
> > > I confirmed that the first patch here doesn't work in Release mode, but I'll
> > > try the new one now. :)
> > 
> > Ugh, I can't believe it. It managed to start running every single test, but
> > at the very end, two subprocesses are hanging in the usual location (jsc.cpp
> > line 172) and so the runner can't exit.
> 
> Where is the hanging location? And does that mean isMainThread is not
> working well even with this patch?
> Also, I would like to know,
> 
> 1. what is the value stored in `mainThread` variable in MainThreadWin.cpp
> 2. what is the returned value from Thread::currentID() in such situation?
> 3. Is GetCurrentThreadId broken in TLS destructor?
> 4. Is TLS destructor called in the main thread?

Both subprocesses are hung in the location mentioned above:
> [External Code] 
> jscLib.dll!`anonymous namespace'::jscExit(int status) Line 172
> jscLib.dll!main(int argc, char * * argv) Line 2270
> jscLib.dll!dllLauncherEntryPoint(int argc, const char * * argv) Line 2850
> jsc.exe!main(int argc, const char * * argv) Line 222
> [External Code] 

So jscExit is calling `exit(status);` (with status 0, FWIW).

It seems clear that isMainThread is not working *before* the patch, but it's unclear with the patch.

I'm not sure if any of the other questions can be answered by simply attaching to process, without rebuilding and rerunning...? The call stack is so short and uninformative, and using "WTF::isMainThread()" as a watch expression doesn't seem to work.

[Don Olmstead]

(In reply to Ross Kirsling from comment #14)
> (In reply to Yusuke Suzuki from comment #13)
> > (In reply to Ross Kirsling from comment #12)
> > > (In reply to Ross Kirsling from comment #11)
> > > > Oops, I had dropped the line numbers from that stack trace. This is the
> > > > holdLock() call:
> > > > https://github.com/WebKit/webkit/blob/master/Source/WTF/wtf/Threading.
> > > > cpp#L200
> > > > So then shouldRemoveThreadFromThreadGroup() definitely seems to be returning
> > > > `true` for the main thread in a certain scenario (without your patch).
> > > > 
> > > > I confirmed that the first patch here doesn't work in Release mode, but I'll
> > > > try the new one now. :)
> > > 
> > > Ugh, I can't believe it. It managed to start running every single test, but
> > > at the very end, two subprocesses are hanging in the usual location (jsc.cpp
> > > line 172) and so the runner can't exit.
> > 
> > Where is the hanging location? And does that mean isMainThread is not
> > working well even with this patch?
> > Also, I would like to know,
> > 
> > 1. what is the value stored in `mainThread` variable in MainThreadWin.cpp
> > 2. what is the returned value from Thread::currentID() in such situation?
> > 3. Is GetCurrentThreadId broken in TLS destructor?
> > 4. Is TLS destructor called in the main thread?
> 
> Both subprocesses are hung in the location mentioned above:
> > [External Code] 
> > jscLib.dll!`anonymous namespace'::jscExit(int status) Line 172
> > jscLib.dll!main(int argc, char * * argv) Line 2270
> > jscLib.dll!dllLauncherEntryPoint(int argc, const char * * argv) Line 2850
> > jsc.exe!main(int argc, const char * * argv) Line 222
> > [External Code] 
> 
> So jscExit is calling `exit(status);` (with status 0, FWIW).
> 
> It seems clear that isMainThread is not working *before* the patch, but it's
> unclear with the patch.
> 
> I'm not sure if any of the other questions can be answered by simply
> attaching to process, without rebuilding and rerunning...? The call stack is
> so short and uninformative, and using "WTF::isMainThread()" as a watch
> expression doesn't seem to work.

Ok so its dying at exit.

So according to MSDN https://msdn.microsoft.com/en-us/library/6wdz5232.aspx#Anchor_1

> The exit function calls destructors for thread-local objects, then calls—in last-in-first-out (LIFO) order—the functions that are registered by atexit and _onexit, and then flushes all file buffers before it terminates the process.

You could try and register a function and see if it ever gets to it. Possibly some destructor is getting called and stalling.

[Ross Kirsling]

Oops, looking at the outstanding questions again...

> 4. Is TLS destructor called in the main thread?

This is surely false given the shortened call stack which seems not to enter WTF.

What's also interesting is that before the patch, I tried adding a printf to isMainThread to see the comparison it was making when called from shouldRemoveThreadFromThreadGroup. (This printf was enough to cause Debug mode to succeed for some strange reason, but Release mode failed in the original way, i.e. eight hanging subprocesses with the main thread waiting for the thread group lock.) The only comparisons that were printed out were for child threads -- I never saw a comparison of the main thread ID against itself for subprocesses that exited successfully, and the hanging subprocesses (or at least, the very last one) printed nothing at all.

[Ross Kirsling]

(In reply to Ross Kirsling from comment #16)
> Oops, looking at the outstanding questions again...
> 
> > 4. Is TLS destructor called in the main thread?
> 
> This is surely false given the shortened call stack which seems not to enter
> WTF.
> 
> What's also interesting is that before the patch, I tried adding a printf to
> isMainThread to see the comparison it was making when called from
> shouldRemoveThreadFromThreadGroup. (This printf was enough to cause Debug
> mode to succeed for some strange reason, but Release mode failed in the
> original way, i.e. eight hanging subprocesses with the main thread waiting
> for the thread group lock.) The only comparisons that were printed out were
> for child threads -- I never saw a comparison of the main thread ID against
> itself for subprocesses that exited successfully, and the hanging
> subprocesses (or at least, the very last one) printed nothing at all.

With the current patch, if I add a print statement to isMainThread when called from shouldRemoveThreadFromThreadGroup, I see exactly the desired output when a subprocess exits properly:
> Running stress/activation-sink-osrexit-default-value.js.dfg-eager
> stress/activation-sink-osrexit-default-value.js.dfg-eager: *isMainThread* 0 (main: 8252, current: 10992)
> stress/activation-sink-osrexit-default-value.js.dfg-eager: *isMainThread* 0 (main: 8252, current: 7460)
> stress/activation-sink-osrexit-default-value.js.dfg-eager: *isMainThread* 0 (main: 8252, current: 2756)
> stress/activation-sink-osrexit-default-value.js.dfg-eager: *isMainThread* 1 (main: 8252, current: 8252)
> PASS: stress/activation-sink-osrexit-default-value.js.dfg-eager

...while, as before, no output at all when a subprocess hangs:
> Running stress/try-catch-getter-as-get-by-id-register-restoration.js.no-cjit-collect-continuously

[Ross Kirsling]

(In reply to Ross Kirsling from comment #17)
> With the current patch, if I add a print statement to isMainThread when
> called from shouldRemoveThreadFromThreadGroup, I see exactly the desired
> output when a subprocess exits properly:
> > Running stress/activation-sink-osrexit-default-value.js.dfg-eager
> > stress/activation-sink-osrexit-default-value.js.dfg-eager: *isMainThread* 0 (main: 8252, current: 10992)
> > stress/activation-sink-osrexit-default-value.js.dfg-eager: *isMainThread* 0 (main: 8252, current: 7460)
> > stress/activation-sink-osrexit-default-value.js.dfg-eager: *isMainThread* 0 (main: 8252, current: 2756)
> > stress/activation-sink-osrexit-default-value.js.dfg-eager: *isMainThread* 1 (main: 8252, current: 8252)
> > PASS: stress/activation-sink-osrexit-default-value.js.dfg-eager
> 
> ...while, as before, no output at all when a subprocess hangs:
> > Running stress/try-catch-getter-as-get-by-id-register-restoration.js.no-cjit-collect-continuously

Other things I managed to confirm:
- There is no sampling profiler thread interfering (ENABLE_SAMPLING_PROFILER is true but Options::useSamplingProfiler is false).
- I ensured that the buffer is flushed after printing, but still no output in the hanging case.
- I attached an atexit() from initializeMainThreadPlatform() -- it fires as expected in the normal case and does not fire in the hanging case.

I think all of this suffices to answer the questions you asked and I think that the most recent change related to m_didExit shouldn't be necessary (since I believe your earlier patch fixes the isMainThread invalidity?)...but I'm still very confused as to what could possibly be missing.

[Yusuke Suzuki]

(In reply to Ross Kirsling from comment #18)
> (In reply to Ross Kirsling from comment #17)
> > With the current patch, if I add a print statement to isMainThread when
> > called from shouldRemoveThreadFromThreadGroup, I see exactly the desired
> > output when a subprocess exits properly:
> > > Running stress/activation-sink-osrexit-default-value.js.dfg-eager
> > > stress/activation-sink-osrexit-default-value.js.dfg-eager: *isMainThread* 0 (main: 8252, current: 10992)
> > > stress/activation-sink-osrexit-default-value.js.dfg-eager: *isMainThread* 0 (main: 8252, current: 7460)
> > > stress/activation-sink-osrexit-default-value.js.dfg-eager: *isMainThread* 0 (main: 8252, current: 2756)
> > > stress/activation-sink-osrexit-default-value.js.dfg-eager: *isMainThread* 1 (main: 8252, current: 8252)
> > > PASS: stress/activation-sink-osrexit-default-value.js.dfg-eager
> > 
> > ...while, as before, no output at all when a subprocess hangs:
> > > Running stress/try-catch-getter-as-get-by-id-register-restoration.js.no-cjit-collect-continuously

Does this mean `shouldRemoveThreadFromThreadGroup` is not called when hanging?
In that case, what is the backtrace of the hanging thread? Is `Thread::didExit` still included?

> Other things I managed to confirm:
> - There is no sampling profiler thread interfering (ENABLE_SAMPLING_PROFILER
> is true but Options::useSamplingProfiler is false).
> - I ensured that the buffer is flushed after printing, but still no output
> in the hanging case.
> - I attached an atexit() from initializeMainThreadPlatform() -- it fires as
> expected in the normal case and does not fire in the hanging case.
> 
> I think all of this suffices to answer the questions you asked and I think
> that the most recent change related to m_didExit shouldn't be necessary
> (since I believe your earlier patch fixes the isMainThread
> invalidity?)...but I'm still very confused as to what could possibly be
> missing.

I think the recent change for m_didExit is required anyway since it touches Thread::m_mutex anyway.
For example, concurrent GC thread can take this mutex, and be destroyed earlier.

[Ross Kirsling]

(In reply to Yusuke Suzuki from comment #19)
> (In reply to Ross Kirsling from comment #18)
> > (In reply to Ross Kirsling from comment #17)
> > > With the current patch, if I add a print statement to isMainThread when
> > > called from shouldRemoveThreadFromThreadGroup, I see exactly the desired
> > > output when a subprocess exits properly:
> > > > Running stress/activation-sink-osrexit-default-value.js.dfg-eager
> > > > stress/activation-sink-osrexit-default-value.js.dfg-eager: *isMainThread* 0 (main: 8252, current: 10992)
> > > > stress/activation-sink-osrexit-default-value.js.dfg-eager: *isMainThread* 0 (main: 8252, current: 7460)
> > > > stress/activation-sink-osrexit-default-value.js.dfg-eager: *isMainThread* 0 (main: 8252, current: 2756)
> > > > stress/activation-sink-osrexit-default-value.js.dfg-eager: *isMainThread* 1 (main: 8252, current: 8252)
> > > > PASS: stress/activation-sink-osrexit-default-value.js.dfg-eager
> > > 
> > > ...while, as before, no output at all when a subprocess hangs:
> > > > Running stress/try-catch-getter-as-get-by-id-register-restoration.js.no-cjit-collect-continuously
> 
> Does this mean `shouldRemoveThreadFromThreadGroup` is not called when
> hanging?

Correct.

> In that case, what is the backtrace of the hanging thread? Is
> `Thread::didExit` still included?

No, both versions of the patch produce the same backtrace that I pasted in comment 14 above -- exit() goes into [External Code] and nothing beyond that can be seen. In particular, WTF no longer appears at all.

> > Other things I managed to confirm:
> > - There is no sampling profiler thread interfering (ENABLE_SAMPLING_PROFILER
> > is true but Options::useSamplingProfiler is false).
> > - I ensured that the buffer is flushed after printing, but still no output
> > in the hanging case.
> > - I attached an atexit() from initializeMainThreadPlatform() -- it fires as
> > expected in the normal case and does not fire in the hanging case.
> > 
> > I think all of this suffices to answer the questions you asked and I think
> > that the most recent change related to m_didExit shouldn't be necessary
> > (since I believe your earlier patch fixes the isMainThread
> > invalidity?)...but I'm still very confused as to what could possibly be
> > missing.
> 
> I think the recent change for m_didExit is required anyway since it touches
> Thread::m_mutex anyway.
> For example, concurrent GC thread can take this mutex, and be destroyed
> earlier.

Okay -- I just didn't know whether that could cause a problem with hasExited() elsewhere. :)

[Yusuke Suzuki]

OK! So I think this patch fixes the `isMainThread()` issue.
And now, the other issue appears in Windows JSC tests b/c isMainThread issue is fixed.

[Ross Kirsling]

(In reply to Yusuke Suzuki from comment #21)
> OK! So I think this patch fixes the `isMainThread()` issue.
> And now, the other issue appears in Windows JSC tests b/c isMainThread issue
> is fixed.

Sounds good. It does seem that after this patch, WinCairo JSC tests are able to run to completion in Debug mode, and with --quick and some good luck, it is possible for Release mode to reach completion too. Hopefully we can solve the remaining Release issue in a follow-up.

[Mark Lam]

Comment on attachment 350560 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 350560 [details]
Patch

Clearing flags on attachment: 350560

Committed r236617: <https://trac.webkit.org/changeset/236617>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/44877012>