Bug 132032 – DFG::Worklist should acquire the m_lock before iterating DFG plans

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

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

[Radar WebKit Bug Importer]

<rdar://problem/16693833>

[Filip Pizlo]

Interesting!  That's a good catch. Looks like Worklist::visitChildren needs to lock m_lock. Is that what you were thinking?

[Mark Lam]

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

[Filip Pizlo]

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

[Mark Lam]

Created attachment 229928 [details]
the patch

[Filip Pizlo]

Comment on attachment 229928 [details]
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.

[Mark Lam]

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