Closed
Bug 998880
Opened 10 years ago
Closed 10 years ago
nsThread shouldn't busy-loop in NS_ProcessNextEvent waiting for a thread to shutdown
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: jesup, Assigned: jesup)
References
Details
(Keywords: perf)
Attachments
(1 file)
1.80 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
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...
Assignee | ||
Comment 1•10 years ago
|
||
Try: https://tbpl.mozilla.org/?tree=Try&rev=c70d5ce66801
Assignee | ||
Comment 2•10 years ago
|
||
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)
Assignee | ||
Comment 3•10 years ago
|
||
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....
Assignee | ||
Comment 4•10 years ago
|
||
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)
Assignee | ||
Comment 5•10 years ago
|
||
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
Assignee | ||
Comment 6•10 years ago
|
||
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.
Assignee | ||
Comment 7•10 years ago
|
||
Updated Try, expect ASAN failure still https://tbpl.mozilla.org/?tree=Try&rev=9847c1eb3bf0
Assignee | ||
Comment 8•10 years ago
|
||
Hmm, no ASAN error. retriggering. Maybe I just picked a poor base
Assignee | ||
Comment 9•10 years ago
|
||
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•10 years ago
|
||
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+
Assignee | ||
Comment 11•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e2566a604f1
Target Milestone: --- → mozilla31
https://hg.mozilla.org/mozilla-central/rev/2e2566a604f1
Assignee: nobody → rjesup
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•