Closed Bug 707664 Opened 8 years ago Closed 8 years ago

OOM reporting should unlock the atoms compartment and thread-unsafe usage of atomsCompartmentIsLocked

Categories

(Core :: JavaScript Engine, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla11

People

(Reporter: igor, Assigned: igor)

Details

Attachments

(1 file)

Currently our OOM handling code does not unlock the atoms compartment when reporting memory allocation errors under AtomizeInline. That can lead to a double lock if the OOM callback could be triggered to call atoms-related API.

Another problem with atoms compartment locking is the usage of JSRuntime::atomsCompartmentIsLocked in RunLastDitchGC:

    Maybe<AutoUnlockAtomsCompartment> maybeUnlockAtomsCompartment;
    if (cx->compartment == rt->atomsCompartment && rt->atomsCompartmentIsLocked)
        maybeUnlockAtomsCompartment.construct(cx);

Here atomsCompartmentIsLocked is a global variable that, as we still support JS_THREADSAFE, can be mutated on other threads. Those mutations may not yet propagate to the current thread and RunLastDitchGC may see atomsCompartmentIsLocked set when the atoms compartment is in fact unlocked.
Attached patch v1Splinter Review
To fix the thread-safety-issue and to simplify the unlocking of atoms compartment the patch moves atomsLocked flag into JSContext. This emphasizes that the flag is just a way to pass from AtomizeInline an extra information to the RunLastDitchGC and js_ReportOOM functions that they should unlock the compartment. 

As a bonus this allowed to simplify the AutoUnlockAtomsCompartment class usage and fix can-be-not-initialized warning with GCC 4.6 that comes from the class usage in jsgc.cpp without the patch. That is how I found the bug.
Attachment #579200 - Flags: review?(luke)
Attachment #579200 - Flags: review?(luke) → review+
https://hg.mozilla.org/mozilla-central/rev/931c2ba07cf1
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
You need to log in before you can comment on or make changes to this bug.