Closed
Bug 1311702
Opened 8 years ago
Closed 8 years ago
Potential deadlock resizing the nursery in FF51
Categories
(Core :: JavaScript: GC, defect)
Core
JavaScript: GC
Tracking
()
RESOLVED
FIXED
mozilla51
Tracking | Status | |
---|---|---|
firefox49 | --- | unaffected |
firefox-esr45 | --- | unaffected |
firefox50 | --- | unaffected |
firefox51 | --- | fixed |
firefox52 | --- | unaffected |
People
(Reporter: jonco, Assigned: jonco)
References
Details
Attachments
(1 file)
3.32 KB,
patch
|
sfink
:
review+
gchang
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
When I first tried to land the fix for bug 1309615 it got backed out for triggering a deadlock. I updated the patch and pushed again but the deadlock was an pre-existing issue that was caused by bug 1291292, and is still present in FF 51. I should probably have split this out into a separate bug at the time.
Assignee | ||
Comment 1•8 years ago
|
||
The problem is in Nursery::updateNumChunksLocked where we have a AutoMaybeStartBackgroundAllocation while we already hold the GC lock. The destructor of AutoMaybeStartBackgroundAllocation calls GCRuntime::startBackgroundAllocTaskIfIdle which takes the helper thread state lock. We need to ensure a consistent lock ordering between these two locks to prevent the possibility of deadlock. Elsewhere we only take the GC lock after taking the helper thread state lock so we need observe that ordering here too. This just means moving AutoMaybeStartBackgroundAllocation up into the callers until it's outside the scope of AutoLockGC.
Attachment #8802923 -
Flags: review?(sphink)
Assignee | ||
Updated•8 years ago
|
status-firefox49:
--- → unaffected
status-firefox50:
--- → unaffected
status-firefox51:
--- → affected
status-firefox52:
--- → unaffected
status-firefox-esr45:
--- → unaffected
Comment 2•8 years ago
|
||
Comment on attachment 8802923 [details] [diff] [review] bug1311702-nursery-resize-deadlock Review of attachment 8802923 [details] [diff] [review]: ----------------------------------------------------------------- Would tsan catch something like this? Does it monitor lock orderings? I really ought to fix up that build, in case it's already detecting this case.
Attachment #8802923 -
Flags: review?(sphink) → review+
Comment 3•8 years ago
|
||
Ah. It's there, but experimental (as of Aug 2015). TSAN_OPTIONS=detect_deadlocks=1
Assignee | ||
Comment 4•8 years ago
|
||
(In reply to Steve Fink [:sfink] [:s:] from comment #3) Interesting! We should try and turn this on if possible.
Assignee | ||
Comment 5•8 years ago
|
||
Comment on attachment 8802923 [details] [diff] [review] bug1311702-nursery-resize-deadlock Approval Request Comment [Feature/regressing bug #]: Bug 1291292. [User impact if declined]: Potential hang. [Describe test coverage new/current, TreeHerder]: This fix has been on m-c since 14th October as part of the fix for bug 1309615. [Risks and why]: Low. [String/UUID change made/needed]: None.
Attachment #8802923 -
Flags: approval-mozilla-aurora?
Comment 6•8 years ago
|
||
Comment on attachment 8802923 [details] [diff] [review] bug1311702-nursery-resize-deadlock Fix a potential hang issue. Take it in 51 aurora.
Attachment #8802923 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 7•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/903cce16fa75
Updated•8 years ago
|
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in
before you can comment on or make changes to this bug.
Description
•