GCRuntime::requestInterruptCallback() is not threadsafe

RESOLVED FIXED in Firefox 52

Status

()

Core
JavaScript: GC
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: mccr8, Assigned: billm)

Tracking

(Blocks: 1 bug)

unspecified
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

a year ago
It is defined like this (and runs off the main thread):
    if (currentBudget) {
        interruptCallbackRequested = true;
        currentBudget->requestFullCheck();
    }

currentBudget is just a pointer, and gets written to on the main thread, so that's the first thread safety issue. Secondly, even if it was Atomic<>, it could get destroyed in between when we do the check and when we call requestFullCheck().

One fix might be to make the budget a field on some runtime data structure. Then there would be no pointer, and we would not have to worry about it being destroyed when we're in this callback.
(Assignee)

Comment 1

a year ago
I talked to Andrew about this. I think it should be safe to take the GC lock when we read or write currentBudget. That happens pretty rarely.
(Reporter)

Comment 2

a year ago
Another thing I was thinking about is that HangMonitorChild holds a pointer to the JSContext, but I guess we're pretty sure HangMonitorChild will die before the JS engine does...
(Assignee)

Updated

a year ago
Flags: needinfo?(wmccloskey)
(Assignee)

Comment 3

a year ago
Andrew also pointed out that we never unset interruptCallbackRequested, which is really bad.
(Assignee)

Comment 4

a year ago
Created attachment 8802238 [details] [diff] [review]
Unset interruptCallbackRequested
Flags: needinfo?(wmccloskey)
Attachment #8802238 - Flags: review?(jcoppeard)
(Assignee)

Comment 5

a year ago
Created attachment 8802239 [details] [diff] [review]
Locking fixes

This fixes the race on currentBudget.
Assignee: nobody → wmccloskey
Status: NEW → ASSIGNED
Attachment #8802239 - Flags: review?(jcoppeard)
Attachment #8802238 - Flags: review?(jcoppeard) → review+
Attachment #8802239 - Flags: review?(jcoppeard) → review+
(Reporter)

Comment 6

a year ago
interruptCallbackRequested should probably also be cleared at the same time that currentBudget is. You could have a budget, then request an interrupt callback (setting the flag to true), then I assume possibly finish out the GC without checking the budget (like maybe really near the end in some noninterruptible phase), and then the flag would be true when exit the GC, so when the next GC starts, the interrupt flag is still set. Sorry for noticing these in a piecemeal fashion...
(Reporter)

Comment 7

a year ago
Though clearing the interrupt flag at the same time you initially set currentBudget would be more robust.

Comment 8

a year ago
Pushed by wmccloskey@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d18605c870f9
Unset interruptCallbackRequested (r=jonco)
https://hg.mozilla.org/integration/mozilla-inbound/rev/43d55ee0e09e
Locking fixes for currentBudget (r=jonco)

Comment 9

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d18605c870f9
https://hg.mozilla.org/mozilla-central/rev/43d55ee0e09e
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.