Closed Bug 622691 Opened 9 years ago Closed 9 years ago
data race on JSContext::default
Compartment Is Locked
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)
blocking2.0: ? → betaN+
Assignee: general → dmandelin
Status: NEW → ASSIGNED
Attachment #501529 - Flags: review?(gal)
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
Whooops. Good catch. So this one should work as a followup patch?
Attachment #501739 - Flags: review?(jseward)
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Attachment #501739 - Flags: review?(jseward) → review?(gal)
You need to log in before you can comment on or make changes to this bug.