Closed Bug 1311702 Opened 5 years ago Closed 5 years ago

Potential deadlock resizing the nursery in FF51

Categories

(Core :: JavaScript: GC, defect)

defect
Not set
normal

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)

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.
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)
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+
Ah. It's there, but experimental (as of Aug 2015).

TSAN_OPTIONS=detect_deadlocks=1
(In reply to Steve Fink [:sfink] [:s:] from comment #3)
Interesting!  We should try and turn this on if possible.
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 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+
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.