Closed Bug 707664 Opened 8 years ago Closed 8 years ago
OOM reporting should unlock the atoms compartment and thread-unsafe usage of atoms
Compartment Is Locked
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.
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)
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.