Closed
Bug 1452898
Opened 7 years ago
Closed 7 years ago
TSAN reports data race calling JSRuntime::hasHelperThreadZones() on helper thread
Categories
(Core :: JavaScript Engine, defect, P3)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: jonco, Assigned: jonco)
Details
Attachments
(1 file)
899 bytes,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
WARNING: ThreadSanitizer: data race (pid=31440)
Read of size 8 at 0x7ba400000418 by thread T9:
#0 bool js::ProtectedData<js::CheckUnprotected, unsigned long>::operator><int>(int const&) const /home/jon/clone/dev/js/src/threading/ProtectedData.h:79:5 (js+0xd83ea3)
#1 JSRuntime::hasHelperThreadZones() const /home/jon/clone/dev/js/src/vm/Runtime.h:492 (js+0xd83ea3)
#2 js::AutoLockScriptData::AutoLockScriptData(JSRuntime*) /home/jon/clone/dev/js/src/vm/JSContext.h:1223 (js+0xd83ea3)
#3 JSScript::shareScriptData(JSContext*) /home/jon/clone/dev/js/src/vm/JSScript.cpp:2455 (js+0xd83ea3)
#4 JSScript::fullyInitFromEmitter(JSContext*, JS::Handle<JSScript*>, js::frontend::BytecodeEmitter*) /home/jon/clone/dev/js/src/vm/JSScript.cpp:2975:18 (js+0xd8e0be)
#5 js::frontend::BytecodeEmitter::emitScript(js::frontend::ParseNode*) /home/jon/clone/dev/js/src/frontend/BytecodeEmitter.cpp:4917:10 (js+0x10bada5)
#6 BytecodeCompiler::compileScript(JS::Handle<JSObject*>, js::frontend::SharedContext*) /home/jon/clone/dev/js/src/frontend/BytecodeCompiler.cpp:343:27 (js+0x10ba640)
#7 BytecodeCompiler::compileGlobalScript(js::ScopeKind) /home/jon/clone/dev/js/src/frontend/BytecodeCompiler.cpp:377:12 (js+0x10bcf79)
#8 js::frontend::CompileGlobalScript(JSContext*, js::LifoAlloc&, js::ScopeKind, JS::ReadOnlyCompileOptions const&, JS::SourceBufferHolder&, js::ScriptSourceObject**) /home/jon/clone/dev/js/src/frontend/BytecodeCompiler.cpp:575 (js+0x10bcf79)
#9 js::ScriptParseTask::parse(JSContext*) /home/jon/clone/dev/js/src/vm/HelperThreads.cpp:476:24 (js+0xd0d7e6)
#10 js::HelperThread::handleParseWorkload(js::AutoLockHelperThreadState&) /home/jon/clone/dev/js/src/vm/HelperThreads.cpp:1939:15 (js+0xd15a19)
#11 js::HelperThread::threadLoop() /home/jon/clone/dev/js/src/vm/HelperThreads.cpp:2249:9 (js+0xd145c3)
#12 js::HelperThread::ThreadMain(void*) /home/jon/clone/dev/js/src/vm/HelperThreads.cpp:1734:38 (js+0xd10755)
#13 void js::detail::ThreadTrampoline<void (&)(void*), js::HelperThread*>::callMain<0ul>(mozilla::IndexSequence<0ul>) /home/jon/clone/dev/js/src/threading/Thread.h:242:5 (js+0xd1c09d)
#14 js::detail::ThreadTrampoline<void (&)(void*), js::HelperThread*>::Start(void*) /home/jon/clone/dev/js/src/threading/Thread.h:235 (js+0xd1c09d)
Previous write of size 8 at 0x7ba400000418 by main thread (mutexes: write M30):
#0 js::ProtectedData<js::CheckUnprotected, unsigned long>::operator++(int) /home/jon/clone/dev/js/src/threading/ProtectedData.h:95:38 (js+0xde89ab)
#1 JSRuntime::setUsedByHelperThread(JS::Zone*) /home/jon/clone/dev/js/src/vm/Runtime.cpp:798 (js+0xde89ab)
#2 js::ParseTask::activate(JSRuntime*) /home/jon/clone/dev/js/src/vm/HelperThreads.cpp:422:9 (js+0xd0f1d6)
#3 QueueOffThreadParseTask(JSContext*, js::ParseTask*) /home/jon/clone/dev/js/src/vm/HelperThreads.cpp:740 (js+0xd0f1d6)
#4 StartOffThreadParseTask(JSContext*, js::ParseTask*, JS::ReadOnlyCompileOptions const&) /home/jon/clone/dev/js/src/vm/HelperThreads.cpp:769 (js+0xd0f1d6)
#5 js::StartOffThreadParseScript(JSContext*, JS::ReadOnlyCompileOptions const&, char16_t const*, unsigned long, void (*)(void*, void*), void*) /home/jon/clone/dev/js/src/vm/HelperThreads.cpp:783:19 (js+0xd0f4c0)
#6 JS::CompileOffThread(JSContext*, JS::ReadOnlyCompileOptions const&, char16_t const*, unsigned long, void (*)(void*, void*), void*) /home/jon/clone/dev/js/src/jsapi.cpp:4285:12 (js+0xb9c071)
#7 OffThreadCompileScript(JSContext*, unsigned int, JS::Value*) /home/jon/clone/dev/js/src/shell/js.cpp:4532:10 (js+0x4eee45)
#8 <null> <null> (0x7f7a7ef3ac00)
#9 Interpret(JSContext*, js::RunState&) /home/jon/clone/dev/js/src/vm/Interpreter.cpp:2037:28 (js+0x5fb3f9)
#10 js::RunScript(JSContext*, js::RunState&) /home/jon/clone/dev/js/src/vm/Interpreter.cpp:417:12 (js+0x5f9d86)
#11 js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) /home/jon/clone/dev/js/src/vm/Interpreter.cpp:700:15 (js+0x611214)
#12 js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) /home/jon/clone/dev/js/src/vm/Interpreter.cpp:732:12 (js+0x611430)
#13 ExecuteScript(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSScript*>, JS::Value*) /home/jon/clone/dev/js/src/jsapi.cpp:4702:12 (js+0xb9f0d9)
#14 JS_ExecuteScript(JSContext*, JS::Handle<JSScript*>) /home/jon/clone/dev/js/src/jsapi.cpp:4735:12 (js+0xb9f2b6)
#15 RunFile(JSContext*, char const*, _IO_FILE*, bool) /home/jon/clone/dev/js/src/shell/js.cpp:833:14 (js+0x4fc7ee)
#16 Process(JSContext*, char const*, bool, FileKind) /home/jon/clone/dev/js/src/shell/js.cpp:1277 (js+0x4fc7ee)
#17 ProcessArgs(JSContext*, js::cli::OptionParser*) /home/jon/clone/dev/js/src/shell/js.cpp:8249:18 (js+0x4dfe2e)
#18 Shell(JSContext*, js::cli::OptionParser*, char**) /home/jon/clone/dev/js/src/shell/js.cpp:8668 (js+0x4dfe2e)
#19 main /home/jon/clone/dev/js/src/shell/js.cpp:9133 (js+0x4dfe2e)
Assignee | ||
Comment 1•7 years ago
|
||
JSRuntime::hasHelperThreadZones() reads numActiveHelperThreadZones without locking. This can be called on helper threads while the value is updated by the main thread.
I don't think this is actually a problem because the value never becomes zero while any helper threads are running, so hasHelperThreadZones() will always return true.
Assignee | ||
Comment 2•7 years ago
|
||
Making this field atomic makes the TSAN warning go away.
Attachment #8966553 -
Flags: review?(jdemooij)
Assignee | ||
Updated•7 years ago
|
Priority: -- → P3
Comment 3•7 years ago
|
||
Comment on attachment 8966553 [details] [diff] [review]
bug1452898-helper-thread-count-race
Review of attachment 8966553 [details] [diff] [review]:
-----------------------------------------------------------------
I'm a bit worried about regressing AutoLockForExclusiveAccess because it uses this as a fast path, but maybe it's okay, also because atomization doesn't use AutoLockForExclusiveAccess when it uses the per-zone atoms table?
Attachment #8966553 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 4•7 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #3)
Yeah, that's a good point. I'll land it and back out if there are any regressions.
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b0a61c9da09
Make count of helper thread zones atomic as this can be read without locking by helper threads r=jandem
Comment 6•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•