Closed Bug 998880 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: jesup, Assigned: jesup)

References

Details

(Keywords: perf)

Attachments

(1 file)

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 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
Attachment #8409575 - Flags: review?(benjamin)
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....
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)
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
Blocks: 999016
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.
Updated Try, expect ASAN failure still
https://tbpl.mozilla.org/?tree=Try&rev=9847c1eb3bf0
Hmm, no ASAN error.  retriggering.  Maybe I just picked a poor base
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 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.
Attachment #8409575 - Flags: review?(benjamin) → review+
https://hg.mozilla.org/mozilla-central/rev/2e2566a604f1
Assignee: nobody → rjesup
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.