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

RESOLVED FIXED in mozilla11

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

unspecified
mozilla11
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 579200 [details] [diff] [review]
v1

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)

Updated

6 years ago
Attachment #579200 - Flags: review?(luke) → review+
(Assignee)

Comment 2

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/931c2ba07cf1
https://hg.mozilla.org/mozilla-central/rev/931c2ba07cf1
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
You need to log in before you can comment on or make changes to this bug.