132032 2014-04-22 17:13:40 -0700 DFG::Worklist should acquire the m_lock before iterating DFG plans 2014-04-22 17:38:23 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 mark.lam mark.lam fpizlo ggaren mhahnenberg mmirman msaboff oliver webkit-bug-importer oldest_to_newest 1003112 0 mark.lam 2014-04-22 17:13:40 -0700 Currently, there's a rightToRun mechanism that ensures that no compilation threads are running when the GC is iterating through the DFG worklists. However, this does not prevent a Worker thread from doing a DFG compilation and modifying the plans in the worklists thereby invalidating the plan the iterator that the GC is using. This issue can be reproduced by enabling COLLECT_ON_EVERY_ALLOCATION and running the fast/workers layout tests. Here are the backtraces of 2 threads which show this issue manifesting. The test is running code which I have instrumented to assert that: 1. No other thread is holding the DFG worklist m_lock when the GC is about to iterate the worklist. 2. We're not in the midst of iterating the worklist when right after we acquire the m_lock. The follow backtraces failed assertion 2 above and found that a worker thread is trying to remove plans from the worklist while the GC is iterating the worklist in the main thread. Thread 0:: Dispatch queue: com.apple.main-thread 0 dyld 0x00007fff6b49c6e0 ImageLoaderMachOCompressed::findClosestSymbol(void const*, void const**) const + 288 1 dyld 0x00007fff6b48ea64 dladdr + 133 2 libdyld.dylib 0x00007fff88f0174c dladdr + 72 3 com.apple.JavaScriptCore 0x0000000101c16f97 WTFPrintBacktrace + 71 (Assertions.cpp:295) 4 com.apple.JavaScriptCore 0x0000000101c16f29 WTFReportBacktrace + 89 (Assertions.cpp:267) 5 com.apple.JavaScriptCore 0x0000000101c17100 WTFCrash + 32 (Assertions.cpp:332) 6 com.apple.JavaScriptCore 0x0000000101800348 JSC::DFG::Worklist::visitChildren(JSC::SlotVisitor&, JSC::CodeBlockSet&) + 728 (DFGWorklist.cpp:239) 7 com.apple.JavaScriptCore 0x000000010182fcc8 JSC::Heap::visitCompilerWorklists() + 120 (Heap.cpp:632) 8 com.apple.JavaScriptCore 0x000000010182f8d6 JSC::Heap::markRoots(double) + 566 (Heap.cpp:519) 9 com.apple.JavaScriptCore 0x0000000101831253 JSC::Heap::collect(JSC::HeapOperation) + 515 (Heap.cpp:996) 10 com.apple.JavaScriptCore 0x0000000101830ff4 JSC::Heap::collectAllGarbage() + 52 (Heap.cpp:952) 11 com.apple.JavaScriptCore 0x0000000101a367ac JSC::MarkedAllocator::allocateSlowCase(unsigned long) + 124 (MarkedAllocator.cpp:153) 12 com.apple.WebCore 0x00000001035897df JSC::MarkedAllocator::allocate(unsigned long) + 79 (MarkedAllocator.h:94) 13 com.apple.WebCore 0x0000000103589679 JSC::MarkedSpace::allocateWithImmortalStructureDestructor(unsigned long) + 41 (MarkedSpace.h:237) 14 com.apple.WebCore 0x00000001035895f6 JSC::Heap::allocateWithImmortalStructureDestructor(unsigned long) + 118 (HeapInlines.h:196) 15 com.apple.WebCore 0x00000001035894c7 void* JSC::allocateCell<JSC::Structure>(JSC::Heap&, unsigned long) + 151 (JSCellInlines.h:120) 16 com.apple.WebCore 0x00000001035890bf void* JSC::allocateCell<JSC::Structure>(JSC::Heap&) + 31 (JSCellInlines.h:136) 17 com.apple.WebCore 0x0000000103588d8f JSC::Structure::create(JSC::VM&, JSC::JSGlobalObject*, JSC::JSValue, JSC::TypeInfo const&, JSC::ClassInfo const*, unsigned char, unsigned int) + 191 (StructureInlines.h:39) 18 com.apple.WebCore 0x0000000104326720 WebCore::JSEventPrototype::createStructure(JSC::VM&, JSC::JSGlobalObject*, JSC::JSValue) + 112 (JSEvent.h:119) 19 com.apple.WebCore 0x0000000104324409 WebCore::JSEvent::createPrototype(JSC::VM&, JSC::JSGlobalObject*) + 89 (JSEvent.cpp:462) 20 com.apple.WebCore 0x00000001043271e0 JSC::Structure* WebCore::getDOMStructure<WebCore::JSEvent>(JSC::VM&, WebCore::JSDOMGlobalObject*) + 112 (JSDOMBinding.h:115) 21 com.apple.WebCore 0x0000000104326480 JSC::JSObject* WebCore::getDOMPrototype<WebCore::JSEvent>(JSC::VM&, JSC::JSGlobalObject*) + 48 (JSDOMBinding.h:126) 22 com.apple.WebCore 0x000000010432409d WebCore::JSEventPrototype::self(JSC::VM&, JSC::JSGlobalObject*) + 29 (JSEvent.cpp:438) 23 com.apple.WebCore 0x000000010449f5f3 WebCore::JSMessageEvent::createPrototype(JSC::VM&, JSC::JSGlobalObject*) + 67 (JSMessageEvent.cpp:215) 24 com.apple.WebCore 0x0000000104334450 JSC::Structure* WebCore::getDOMStructure<WebCore::JSMessageEvent>(JSC::VM&, WebCore::JSDOMGlobalObject*) + 112 (JSDOMBinding.h:115) 25 com.apple.WebCore 0x00000001043288b9 WebCore::JSDOMWrapper* WebCore::createWrapper<WebCore::JSMessageEvent, WebCore::MessageEvent>(WebCore::JSDOMGlobalObject*, WebCore::MessageEvent*) + 169 (JSDOMBinding.h:217) 26 com.apple.WebCore 0x0000000104327795 WebCore::toJS(JSC::ExecState*, WebCore::JSDOMGlobalObject*, WebCore::Event*) + 613 (JSEventCustom.cpp:67) 27 com.apple.WebCore 0x000000010433aeac WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext*, WebCore::Event*) + 732 (JSEventListener.cpp:114) 28 com.apple.WebCore 0x0000000103b0c66f WebCore::EventTarget::fireEventListeners(WebCore::Event*, WebCore::EventTargetData*, WTF::Vector<WebCore::RegisteredEventListener, 1ul, WTF::CrashOnOverflow>&) + 1503 (EventTarget.cpp:247) 29 com.apple.WebCore 0x0000000103b0bf3e WebCore::EventTarget::fireEventListeners(WebCore::Event*) + 334 (EventTarget.cpp:197) 30 com.apple.WebCore 0x0000000103b0bdb7 WebCore::EventTarget::dispatchEvent(WTF::PassRefPtr<WebCore::Event>) + 135 (EventTarget.cpp:160) 31 com.apple.WebCore 0x000000010539f591 WebCore::MessageWorkerTask::performTask(WebCore::ScriptExecutionContext*) + 1345 (WorkerMessagingProxy.cpp:106) 32 com.apple.WebCore 0x0000000103936def WebCore::Document::didReceiveTask(void*) + 463 (Document.cpp:4930) 33 com.apple.JavaScriptCore 0x0000000101c3d19e WTF::dispatchFunctionsFromMainThread() + 414 (MainThread.cpp:171) 34 com.apple.JavaScriptCore 0x0000000101c3fab5 -[JSWTFMainThreadCaller call] + 21 (MainThreadMac.mm:53) 35 com.apple.Foundation 0x00007fff812cc13e __NSThreadPerformPerform + 229 36 com.apple.CoreFoundation 0x00007fff8849e731 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17 37 com.apple.CoreFoundation 0x00007fff8848fea2 __CFRunLoopDoSources0 + 242 38 com.apple.CoreFoundation 0x00007fff8848f62f __CFRunLoopRun + 831 39 com.apple.CoreFoundation 0x00007fff8848f0b5 CFRunLoopRunSpecific + 309 40 DumpRenderTree 0x00000001011c7295 runTest(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) + 4853 (DumpRenderTree.mm:1822) 41 DumpRenderTree 0x00000001011c5f2a runTestingServerLoop() + 282 (DumpRenderTree.mm:1072) 42 DumpRenderTree 0x00000001011c5785 dumpRenderTree(int, char const**) + 405 (DumpRenderTree.mm:1156) 43 DumpRenderTree 0x00000001011c7b27 DumpRenderTreeMain(int, char const**) + 103 (DumpRenderTree.mm:1264) 44 DumpRenderTree 0x00000001012173e2 main + 34 (DumpRenderTreeMain.mm:30) 45 libdyld.dylib 0x00007fff88f025fd start + 1 Thread 14 Crashed:: WebCore: Worker 0 com.apple.JavaScriptCore 0x0000000101c1710a WTFCrash + 42 (Assertions.cpp:333) 1 com.apple.JavaScriptCore 0x00000001017ffa8b JSC::DFG::Worklist::removeAllReadyPlansForVM(JSC::VM&, WTF::Vector<WTF::RefPtr<JSC::DFG::Plan>, 8ul, WTF::CrashOnOverflow>&) + 139 (DFGWorklist.cpp:156) 2 com.apple.JavaScriptCore 0x00000001017ffce9 JSC::DFG::Worklist::completeAllReadyPlansForVM(JSC::VM&, JSC::DFG::CompilationKey) + 89 (DFGWorklist.cpp:183) 3 com.apple.JavaScriptCore 0x00000001018c907e operationOptimize + 974 (JITOperations.cpp:1116) 4 ??? 0x000046d317005439 0 + 77872437941305 5 com.apple.JavaScriptCore 0x0000000101a1ed84 callToJavaScript + 356 6 com.apple.JavaScriptCore 0x00000001018b861d JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) + 45 (JITCode.cpp:47) 7 com.apple.JavaScriptCore 0x000000010189cf89 JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 1241 (Interpreter.cpp:994) 8 com.apple.JavaScriptCore 0x0000000101570f0e JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 190 (CallData.cpp:39) 9 com.apple.JavaScriptCore 0x0000000101570f73 JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, JSC::JSValue*) + 83 (CallData.cpp:44) 10 com.apple.WebCore 0x000000010433b11d WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext*, WebCore::Event*) + 1357 (JSEventListener.cpp:128) 11 com.apple.WebCore 0x0000000103b0c66f WebCore::EventTarget::fireEventListeners(WebCore::Event*, WebCore::EventTargetData*, WTF::Vector<WebCore::RegisteredEventListener, 1ul, WTF::CrashOnOverflow>&) + 1503 (EventTarget.cpp:247) 12 com.apple.WebCore 0x0000000103b0bf3e WebCore::EventTarget::fireEventListeners(WebCore::Event*) + 334 (EventTarget.cpp:197) 13 com.apple.WebCore 0x0000000103b0bdb7 WebCore::EventTarget::dispatchEvent(WTF::PassRefPtr<WebCore::Event>) + 135 (EventTarget.cpp:160) 14 com.apple.WebCore 0x000000010539e9ed WebCore::MessageWorkerGlobalScopeTask::performTask(WebCore::ScriptExecutionContext*) + 1389 (WorkerMessagingProxy.cpp:75) 15 com.apple.WebCore 0x00000001053a0a03 WebCore::WorkerRunLoop::Task::performTask(WebCore::WorkerRunLoop const&, WebCore::ScriptExecutionContext*) + 147 (WorkerRunLoop.cpp:219) 16 com.apple.WebCore 0x00000001053a0521 WebCore::WorkerRunLoop::runInMode(WebCore::WorkerGlobalScope*, WebCore::ModePredicate const&, WebCore::WorkerRunLoop::WaitMode) + 545 (WorkerRunLoop.cpp:165) 17 com.apple.WebCore 0x00000001053a02c9 WebCore::WorkerRunLoop::run(WebCore::WorkerGlobalScope*) + 89 (WorkerRunLoop.cpp:133) 18 com.apple.WebCore 0x00000001053aa0d5 WebCore::WorkerThread::runEventLoop() + 53 (WorkerThread.cpp:206) 19 com.apple.WebCore 0x0000000103884239 WebCore::DedicatedWorkerThread::runEventLoop() + 89 (DedicatedWorkerThread.cpp:66) 20 com.apple.WebCore 0x00000001053a9ff1 WebCore::WorkerThread::workerThread() + 1393 (WorkerThread.cpp:187) 21 com.apple.WebCore 0x00000001053a9a75 WebCore::WorkerThread::workerThreadStart(void*) + 21 (WorkerThread.cpp:155) 22 com.apple.JavaScriptCore 0x0000000101c69350 WTF::threadEntryPoint(void*) + 144 (Threading.cpp:68) 23 com.apple.JavaScriptCore 0x0000000101c6a038 WTF::wtfThreadEntryPoint(void*) + 296 (ThreadingPthreads.cpp:168) 24 libsystem_pthread.dylib 0x00007fff863fe899 _pthread_body + 138 25 libsystem_pthread.dylib 0x00007fff863fe72a _pthread_start + 137 26 libsystem_pthread.dylib 0x00007fff86402fc9 thread_start + 13 1003113 1 webkit-bug-importer 2014-04-22 17:14:10 -0700 <rdar://problem/16693833> 1003114 2 fpizlo 2014-04-22 17:22:03 -0700 Interesting! That's a good catch. Looks like Worklist::visitChildren needs to lock m_lock. Is that what you were thinking? 1003116 3 mark.lam 2014-04-22 17:22:40 -0700 (In reply to comment #2) > Interesting! That's a good catch. Looks like Worklist::visitChildren needs to lock m_lock. Is that what you were thinking? Yes. And also Worklist::isActiveForVM(). 1003118 4 fpizlo 2014-04-22 17:25:39 -0700 (In reply to comment #3) > (In reply to comment #2) > > Interesting! That's a good catch. Looks like Worklist::visitChildren needs to lock m_lock. Is that what you were thinking? > > Yes. And also Worklist::isActiveForVM(). Oh, right! Agreed. 1003120 5 229928 mark.lam 2014-04-22 17:26:50 -0700 Created attachment 229928 the patch 1003121 6 229928 fpizlo 2014-04-22 17:30:41 -0700 Comment on attachment 229928 the patch View in context: https://bugs.webkit.org/attachment.cgi?id=229928&action=review R=me. > Source/JavaScriptCore/dfg/DFGWorklist.cpp:237 > for (unsigned i = m_threads.size(); i--;) { Can you do us a favor and add a comment on top of this loop that says that it doesn't need further locking because (1) no new threads can be added to m_threads so that data structure is immutable and needs no locks, and (2) ThreadDatea::m_safepoint is protected by that thread's rightToRun which we must be holding here because of a prior call to suspendAllThreads(). Or something to that effect. 1003128 7 mark.lam 2014-04-22 17:38:23 -0700 (In reply to comment #6) > Can you do us a favor and add a comment on top of this loop that says that it doesn't need further locking because (1) no new threads can be added to m_threads so that data structure is immutable and needs no locks, and (2) ThreadDatea::m_safepoint is protected by that thread's rightToRun which we must be holding here because of a prior call to suspendAllThreads(). Or something to that effect. Thanks. I love comments. =) It's done. Landed in r167692: <http://trac.webkit.org/r167692>. 229928 2014-04-22 17:26:50 -0700 2014-04-22 17:30:41 -0700 the patch bug-132032.patch text/plain 2692 mark.lam SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTY3NjkxKQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDI0IEBA CisyMDE0LTA0LTIyICBNYXJrIExhbSAgPG1hcmsubGFtQGFwcGxlLmNvbT4KKworICAgICAgICBE Rkc6OldvcmtsaXN0IHNob3VsZCBhY3F1aXJlIHRoZSBtX2xvY2sgYmVmb3JlIGl0ZXJhdGluZyBE RkcgcGxhbnMuCisgICAgICAgIDxodHRwczovL3dlYmtpdC5vcmcvYi8xMzIwMzI+CisKKyAgICAg ICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgQ3VycmVudGx5LCB0aGVy ZSdzIGEgcmlnaHRUb1J1biBtZWNoYW5pc20gdGhhdCBlbnN1cmVzIHRoYXQgbm8gY29tcGlsYXRp b24KKyAgICAgICAgdGhyZWFkcyBhcmUgcnVubmluZyB3aGVuIHRoZSBHQyBpcyBpdGVyYXRpbmcg dGhyb3VnaCB0aGUgREZHIHdvcmtsaXN0cy4KKyAgICAgICAgSG93ZXZlciwgdGhpcyBkb2VzIG5v dCBwcmV2ZW50IGEgV29ya2VyIHRocmVhZCBmcm9tIGRvaW5nIGEgREZHIGNvbXBpbGF0aW9uCisg ICAgICAgIGFuZCBtb2RpZnlpbmcgdGhlIHBsYW5zIGluIHRoZSB3b3JrbGlzdHMgdGhlcmVieSBp bnZhbGlkYXRpbmcgdGhlIHBsYW4KKyAgICAgICAgaXRlcmF0b3IgdGhhdCB0aGUgR0MgaXMgdXNp bmcuICBUaGlzIHBhdGNoIGZpeGVzIHRoZSBpc3N1ZSBieSBhY3F1aXJpbmcKKyAgICAgICAgdGhl IHdvcmtsaXN0IG1fbG9jayBiZWZvcmUgaXRlcmF0aW5nIHRoZSB3b3JrbGlzdCBwbGFucy4KKwor ICAgICAgICBUaGlzIGlzc3VlIHdhcyB1bmNvdmVyZWQgYnkgcnVubmluZyB0aGUgZmFzdC93b3Jr ZXJzIGxheW91dCB0ZXN0cyB3aXRoCisgICAgICAgIENPTExFQ1RfT05fRVZFUllfQUxMT0NBVElP TiBlbmFibGVkLgorCisgICAgICAgICogZGZnL0RGR1dvcmtsaXN0LmNwcDoKKyAgICAgICAgKEpT Qzo6REZHOjpXb3JrbGlzdDo6aXNBY3RpdmVGb3JWTSk6CisgICAgICAgIChKU0M6OkRGRzo6V29y a2xpc3Q6OnZpc2l0Q2hpbGRyZW4pOgorCiAyMDE0LTA0LTIyICBCcmVudCBGdWxnaGFtICA8YmZ1 bGdoYW1AYXBwbGUuY29tPgogCiAgICAgICAgIFtXaW5dIFN1cHBvcnQgUHl0aG9uIDIuNyBpbiBD eWd3aW4KSW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9kZmcvREZHV29ya2xpc3QuY3BwCj09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT0KLS0tIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9kZmcvREZHV29ya2xpc3QuY3BwCShy ZXZpc2lvbiAxNjc2NzUpCisrKyBTb3VyY2UvSmF2YVNjcmlwdENvcmUvZGZnL0RGR1dvcmtsaXN0 LmNwcAkod29ya2luZyBjb3B5KQpAQCAtNzYsNiArNzYsNyBAQCBQYXNzUmVmUHRyPFdvcmtsaXN0 PiBXb3JrbGlzdDo6Y3JlYXRlKHVuCiAKIGJvb2wgV29ya2xpc3Q6OmlzQWN0aXZlRm9yVk0oVk0m IHZtKSBjb25zdAogeworICAgIE11dGV4TG9ja2VyIGxvY2tlcihtX2xvY2spOwogICAgIFBsYW5N YXA6OmNvbnN0X2l0ZXJhdG9yIGVuZCA9IG1fcGxhbnMuZW5kKCk7CiAgICAgZm9yIChQbGFuTWFw Ojpjb25zdF9pdGVyYXRvciBpdGVyID0gbV9wbGFucy5iZWdpbigpOyBpdGVyICE9IGVuZDsgKytp dGVyKSB7CiAgICAgICAgIGlmICgmaXRlci0+dmFsdWUtPnZtID09ICZ2bSkKQEAgLTIyMiwxMiAr MjIzLDE1IEBAIHZvaWQgV29ya2xpc3Q6OnJlc3VtZUFsbFRocmVhZHMoKQogdm9pZCBXb3JrbGlz dDo6dmlzaXRDaGlsZHJlbihTbG90VmlzaXRvciYgdmlzaXRvciwgQ29kZUJsb2NrU2V0JiBjb2Rl QmxvY2tzKQogewogICAgIFZNKiB2bSA9IHZpc2l0b3IuaGVhcCgpLT52bSgpOwotICAgIGZvciAo UGxhbk1hcDo6aXRlcmF0b3IgaXRlciA9IG1fcGxhbnMuYmVnaW4oKTsgaXRlciAhPSBtX3BsYW5z LmVuZCgpOyArK2l0ZXIpIHsKLSAgICAgICAgUGxhbiogcGxhbiA9IGl0ZXItPnZhbHVlLmdldCgp OwotICAgICAgICBpZiAoJnBsYW4tPnZtICE9IHZtKQotICAgICAgICAgICAgY29udGludWU7Ci0g ICAgICAgIGl0ZXItPmtleS52aXNpdENoaWxkcmVuKGNvZGVCbG9ja3MpOwotICAgICAgICBpdGVy LT52YWx1ZS0+dmlzaXRDaGlsZHJlbih2aXNpdG9yLCBjb2RlQmxvY2tzKTsKKyAgICB7CisgICAg ICAgIE11dGV4TG9ja2VyIGxvY2tlcihtX2xvY2spOworICAgICAgICBmb3IgKFBsYW5NYXA6Oml0 ZXJhdG9yIGl0ZXIgPSBtX3BsYW5zLmJlZ2luKCk7IGl0ZXIgIT0gbV9wbGFucy5lbmQoKTsgKytp dGVyKSB7CisgICAgICAgICAgICBQbGFuKiBwbGFuID0gaXRlci0+dmFsdWUuZ2V0KCk7CisgICAg ICAgICAgICBpZiAoJnBsYW4tPnZtICE9IHZtKQorICAgICAgICAgICAgICAgIGNvbnRpbnVlOwor ICAgICAgICAgICAgaXRlci0+a2V5LnZpc2l0Q2hpbGRyZW4oY29kZUJsb2Nrcyk7CisgICAgICAg ICAgICBpdGVyLT52YWx1ZS0+dmlzaXRDaGlsZHJlbih2aXNpdG9yLCBjb2RlQmxvY2tzKTsKKyAg ICAgICAgfQogICAgIH0KICAgICAKICAgICBmb3IgKHVuc2lnbmVkIGkgPSBtX3RocmVhZHMuc2l6 ZSgpOyBpLS07KSB7Cg==