1452898 - TSAN reports data race calling JSRuntime::hasHelperThreadZones() on helper thread

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Jon Coppeard (:jonco)]

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)

Comment 1

[Jon Coppeard (:jonco)]

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.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: bug1452898-helper-thread-count-race

Making this field atomic makes the TSAN warning go away.

Comment 3

[Jan de Mooij [:jandem]]

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?

Comment 4

[Jon Coppeard (:jonco)]

(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.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
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

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/1b0a61c9da09