Last Comment Bug 707664 - OOM reporting should unlock the atoms compartment and thread-unsafe usage of atomsCompartmentIsLocked
: OOM reporting should unlock the atoms compartment and thread-unsafe usage of ...
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Linux
-- normal (vote)
: mozilla11
Assigned To: Igor Bukanov
: Jason Orendorff [:jorendorff]
Depends on:
  Show dependency treegraph
Reported: 2011-12-05 07:06 PST by Igor Bukanov
Modified: 2012-02-01 13:59 PST (History)
1 user (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

v1 (8.35 KB, patch)
2011-12-05 19:53 PST, Igor Bukanov
luke: review+
Details | Diff | Splinter Review

Description User image Igor Bukanov 2011-12-05 07:06:23 PST
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)

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.
Comment 1 User image Igor Bukanov 2011-12-05 19:53:47 PST
Created attachment 579200 [details] [diff] [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.
Comment 3 User image Ed Morley [:emorley] 2011-12-07 02:33:09 PST

Note You need to log in before you can comment on or make changes to this bug.