Closed Bug 1263218 Opened 4 years ago Closed 4 years ago

OOMTest can race with background threads

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: jandem, Assigned: jonco)

References

Details

Attachments

(1 file)

I'm hitting this with a local (testing) patch. 

In OOMTest, the code looks like this:

    HelperThreadState().waitForAllThreads();
    ...
    do {
        ...
        js::oom::SimulateOOMAfter(allocation, thread, false);
        ...
        JS_CallFunction(...);
        ...
    } while (handledOOM);

JS_CallFunction can end up starting new background jobs, and the next loop iteration SimulateOOMAfter will modify oom::maxAllocations. This can race when the background thread is in an AutoEnterOOMUnsafeRegion section (we will fail the assert in ~AutoEnterOOMUnsafeRegion).
Adding a call to waitForAllThreads after JS_CallFunction seems to fix this for me, but not sure if that's the best fix.
Flags: needinfo?(jcoppeard)
Nice catch.  This could be what's causing bug 1235677.
Flags: needinfo?(jcoppeard)
See Also: → 1235677
This mainly waits for the helper threads in ResetSimulatedOOM if we were simulating OOM on a background thread.  I tested that this didn't noticeably slow down the tests.
Assignee: nobody → jcoppeard
Attachment #8740067 - Flags: review?(terrence)
Comment on attachment 8740067 [details] [diff] [review]
bug1263218-oom-test-race

Review of attachment 8740067 [details] [diff] [review]:
-----------------------------------------------------------------

Great find!
Attachment #8740067 - Flags: review?(terrence) → review+
https://hg.mozilla.org/mozilla-central/rev/fd676a6ad20b
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Duplicate of this bug: 1235677
You need to log in before you can comment on or make changes to this bug.