Closed Bug 622691 Opened 9 years ago Closed 9 years ago

data race on JSContext::defaultCompartmentIsLocked

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: jseward, Assigned: dmandelin)

Details

(Whiteboard: softblocker, fixed-in-tracemonkey)

Attachments

(2 files, 1 obsolete file)

Using t-m of today:

Running pbiggar's threaded benchmark (bug 619595) on a
--enable-threadsafe build of jsengine, w/ suitable markup, and race
checking on helgrind.  The race shown below is reported many times.

Race is being reported on cx->runtime->defaultCompartmentIsLocked in
AutoLockDefaultCompartment::AutoLockDefaultCompartment vs
AutoLockDefaultCompartment::~AutoLockDefaultCompartment

The constructor acquires cx->runtime->atomState.lock and then writes
to cx->runtime->defaultCompartmentIsLocked (fine).

The destructor drops the lock, then writes to
cx->runtime->defaultCompartmentIsLocked :-(, so it is not consistently
protected by the lock.

Inverting the order of statements in ~AutoLockDefaultCompartment gets
rid of the reported races.

It looks like class AutoUnlockDefaultCompartment has the same problem,
but Helgrind doesn't appear to report any races.  Maybe the bug 619595
test doesn't exercise it enough.

----------------------

Possible data race during write of size 1 at 0x5fca268 by thread #3
   at 0x42D329: js_AtomizeString(JSContext*, JSString*, unsigned int)
                (jscntxt.h:2649)
   by 0x42D763: js_AtomizeChars(JSContext*, unsigned short const*, un
   by 0x4CF6F1: js::TokenStream::getTokenInternal() (jsscan.cpp:807)
   by 0x4ADA20: js::Parser::variables(bool) (jsscan.h:401)
   by 0x4AECFE: js::Parser::statement() (jsparse.cpp:6015)
   by 0x4B16A4: js::Compiler::compileScript(JSContext*, JSObject*, JS
   by 0x41EA8E: CompileFileHelper(JSContext*, JSObject*, JSPrincipals
   by 0x41ED86: JS_CompileFile (jsapi.cpp:4665)
   by 0x40E4DA: js::workers::InitEvent::process(JSContext*) (jsworker
   by 0x40D37E: js::workers::Worker::processOneEvent() (jsworkers.cpp
   by 0x40DAB9: js::workers::WorkerQueue::work() (jsworkers.cpp:1005)
   by 0x41006D: js::workers::ThreadPool::start(void*) (jsworkers.cpp:
 This conflicts with a previous write of size 1 by thread #1
   at 0x42D4F1: js_AtomizeString(JSContext*, JSString*, unsigned int)
                (jscntxt.h:2655)
   by 0x42D7D1: js_Atomize(JSContext*, char const*, unsigned long, un
   by 0x4142BF: JS_DefineFunction (jsapi.cpp:4411)
   by 0x4143C1: JS_DefineFunctions (jsapi.cpp:4397)
   by 0x48A0E9: js_InitClass(JSContext*, JSObject*, JSObject*, js::Cl
   by 0x41618F: JS_InitClass (jsapi.cpp:2879)
   by 0x40EBDF: js::workers::Worker::init(JSContext*, js::workers::Wo
   by 0x40C44E: js::workers::Worker::create(JSContext*, js::workers::
 Address 0x5fca268 is 8 bytes inside a block of size 2360 alloc'd
   at 0x4C26E2C: calloc (vg_replace_malloc.c:467)
   by 0x412F01: js_calloc (jsutil.h:213)
   by 0x418FD2: JS_Init (jsapi.cpp:764)
   by 0x40A1F0: main (js.cpp:5565)
++helgrind
blocking2.0: --- → ?
blocking2.0: ? → betaN+
Whiteboard: softblocker
Attached patch Patch (obsolete) — Splinter Review
Assignee: general → dmandelin
Status: NEW → ASSIGNED
Attachment #501529 - Flags: review?(gal)
Attachment #501529 - Flags: review?(gal) → review+
http://hg.mozilla.org/tracemonkey/rev/fdfd99101d10
Whiteboard: softblocker → softblocker, fixed-in-tracemonkey
Ehm, I think that's not entirely right.  It fixes class
AutoLockDefaultCompartment ok, but because the sense of locking vs
unlocking is inverted in class AutoUnlockDefaultCompartment, it
causes the assignments to be done entirely outside of the lock.
Revised patch attached.
Attachment #501529 - Attachment is obsolete: true
Attached patch Followup patchSplinter Review
Whooops. Good catch. So this one should work as a followup patch?
Attachment #501739 - Flags: review?(jseward)
http://hg.mozilla.org/mozilla-central/rev/fdfd99101d10
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Attachment #501739 - Flags: review?(jseward) → review?(gal)
Attachment #501739 - Flags: review?(gal) → review+
You need to log in before you can comment on or make changes to this bug.