Closed
Bug 622691
Opened 14 years ago
Closed 14 years ago
data race on JSContext::defaultCompartmentIsLocked
Categories
(Core :: JavaScript Engine, defect)
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)
1.12 KB,
patch
|
Details | Diff | Splinter Review | |
1.02 KB,
patch
|
gal
:
review+
|
Details | Diff | Splinter Review |
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•14 years ago
|
||
++helgrind
Updated•14 years ago
|
blocking2.0: --- → ?
Assignee | ||
Updated•14 years ago
|
blocking2.0: ? → betaN+
Whiteboard: softblocker
Assignee | ||
Comment 2•14 years ago
|
||
Updated•14 years ago
|
Attachment #501529 -
Flags: review?(gal) → review+
Assignee | ||
Comment 3•14 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/fdfd99101d10
Whiteboard: softblocker → softblocker, fixed-in-tracemonkey
Reporter | ||
Comment 4•14 years ago
|
||
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
Assignee | ||
Comment 5•14 years ago
|
||
Whooops. Good catch. So this one should work as a followup patch?
Attachment #501739 -
Flags: review?(jseward)
Comment 6•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/fdfd99101d10
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•14 years ago
|
Attachment #501739 -
Flags: review?(jseward) → review?(gal)
Updated•14 years ago
|
Attachment #501739 -
Flags: review?(gal) → review+
You need to log in
before you can comment on or make changes to this bug.
Description
•