622691 - data race on JSContext::defaultCompartmentIsLocked

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Julian Seward [:jseward]]

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)

Comment 1

[Andreas Gal :gal]

++helgrind

Comment 2

[David Mandelin [:dmandelin]]

Attachment: Patch

Comment 3

[David Mandelin [:dmandelin]]

http://hg.mozilla.org/tracemonkey/rev/fdfd99101d10

Comment 4

[Julian Seward [:jseward]]

Attachment: fix handling for AutoUnlockDefaultCompartment

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.

Comment 5

[David Mandelin [:dmandelin]]

Attachment: Followup patch

Whooops. Good catch. So this one should work as a followup patch?

Comment 6

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/fdfd99101d10