998880 - nsThread shouldn't busy-loop in NS_ProcessNextEvent waiting for a thread to shutdown

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Randell Jesup [:jesup] (needinfo me)]

This bit in nsThread.cpp:
  // Process events on the current thread until we receive a shutdown ACK.
  while (!context.shutdownAck)
    NS_ProcessNextEvent(context.joiningThread);

This busy-loops, since it doesn't let NS_ProcessNextEvent wait. (add a ', true' to allow waiting)

The same issue applies to DISPATCH_SYNC

Busy-looping here just slows down what we want to happen...

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Attachment: don't busy-wait in nsThread waiting for shutdown or DISPATCH_SYNC

Try: https://tbpl.mozilla.org/?tree=Try&rev=c70d5ce66801

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8409575 [details] [diff] [review]
don't busy-wait in nsThread waiting for shutdown or DISPATCH_SYNC

did some basic local smoketests before submitting Try

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

So it looks like this triggers an issue in js::ctypes::ConvertToJS() and a repeatable test failure in content/chrome/js/xpconnect/tests/chrome/test_bug853571.xul

Interesting....

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

The ConvertToJS() failure in content/chrome/dom/ipc/tests/test_process_error.xul looks very interesting (since I can't think of much that *should* be sensitive to changing from busy-wait to wait)

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Looks like https://tbpl.mozilla.org/?tree=Try&rev=5ee6e8a11a7e implies the base I used simply has a perma-orange (off inbound) for the xpconnect orange

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

The ConvertToJs() failure is an intentional ctypes test crash in http://mxr.mozilla.org/mozilla-central/source/dom/ipc/tests/process_error_contentscript.js

Per IRC with bent, we need to find out how to annotate this to ASAN to avoid it kicking on this.

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Updated Try, expect ASAN failure still
https://tbpl.mozilla.org/?tree=Try&rev=9847c1eb3bf0

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

Hmm, no ASAN error.  retriggering.  Maybe I just picked a poor base

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Ok, it's green in the Try.  However:  the ASAN oth run still hits the asan failure - but it does on inbound, as well.  So it has nothing to do with my patch.

Comment 10

[Benjamin Smedberg]

Comment on attachment 8409575 [details] [diff] [review]
don't busy-wait in nsThread waiting for shutdown or DISPATCH_SYNC

I checked both of these cases and they dispatch back to the main thread, so I think this is safe from deadlock. But boy is it grimy.

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2e2566a604f1

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2e2566a604f1