1310351 - GCRuntime::requestInterruptCallback() is not threadsafe

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Andrew McCreight [:mccr8]]

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.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 2

[Andrew McCreight [:mccr8]]

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...

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Andrew also pointed out that we never unset interruptCallbackRequested, which is really bad.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: Unset interruptCallbackRequested

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: Locking fixes

This fixes the race on currentBudget.

Comment 6

[Andrew McCreight [:mccr8]]

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...

Comment 7

[Andrew McCreight [:mccr8]]

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d18605c870f9
https://hg.mozilla.org/mozilla-central/rev/43d55ee0e09e