160337 2016-07-29 04:46:23 -0700 Crash in JavaScriptCore GC when using JSC on dispatch queues (thread_get_state returns NULL stack pointer) 2017-05-10 16:44:29 -0700 1 1 1 Unclassified WebKit JavaScriptCore Other iPhone / iPad iOS 9.3 RESOLVED FIXED InRadar P2 Normal --- 1 nham mark.lam fpizlo ggaren hszhsh jfbastien keith_miller mark.lam msaboff saam webkit-bug-importer oldest_to_newest 1215537 0 nham 2016-07-29 04:46:23 -0700 We recently enabled the use of JSC APIs in our app from dispatch queues. This lead to a number of crashes in the JSC GC timer so we had to remove this feature from our app. This implies that it's not safe to use JSC on a dispatch queue. The crashing stack looks like this on iOS 8 (EXC_BAD_ADDRESS at 0x0): Thread #15 Crashed: 0 JavaScriptCore JSC::ConservativeRoots::add(void*, void*, JSC::JITStubRoutineSet&, JSC::CodeBlockSet&) + 88 1 JavaScriptCore JSC::MachineThreads::gatherFromOtherThread(JSC::ConservativeRoots&, JSC::MachineThreads::Thread*, JSC::JITStubRoutineSet&, JSC::CodeBlockSet&) + 132 2 JavaScriptCore JSC::MachineThreads::gatherFromOtherThread(JSC::ConservativeRoots&, JSC::MachineThreads::Thread*, JSC::JITStubRoutineSet&, JSC::CodeBlockSet&) + 132 3 JavaScriptCore JSC::MachineThreads::gatherConservativeRoots(JSC::ConservativeRoots&, JSC::JITStubRoutineSet&, JSC::CodeBlockSet&, void*, int (&) [48]) + 384 4 JavaScriptCore JSC::Heap::markRoots(double) + 432 5 JavaScriptCore JSC::Heap::collect(JSC::HeapOperation) + 376 6 JavaScriptCore JSC::GCActivityCallback::doWork() + 144 7 JavaScriptCore JSC::HeapTimer::timerDidFire(__CFRunLoopTimer*, void*) + 196 8 CoreFoundation __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__ + 24 9 CoreFoundation __CFRunLoopDoTimer + 884 10 CoreFoundation __CFRunLoopRun + 1368 11 CoreFoundation CFRunLoopRunSpecific + 392 12 Facebook +[RCTJSCExecutor runRunLoopThread] (RCTJSCExecutor.mm:280) 13 Foundation __NSThread__main__ + 1068 14 libsystem_pthread.dylib _pthread_body + 160 15 libsystem_pthread.dylib _pthread_start + 156 16 libsystem_pthread.dylib thread_start + 0 Basically, this is the marker segfaulting on trying to dereference a stack pointer of 0x0 for the target thread. (For this crash, we actually have a dump of the entire state of the stack in this crash, and were able to walk up the stack and find the Thread::Registers instance right on the stack clearly showing a SP of 0x0.) The crashing stack looks like this on iOS 9 (EXC_BAD_ACCESS at 0xbbadbeef): Thread #11 Crashed: 0 JavaScriptCore bmalloc::Heap::allocateXLarge(std::__1::lock_guard<bmalloc::StaticMutex>&, unsigned long) + 36 1 JavaScriptCore bmalloc::Heap::allocateXLarge(std::__1::lock_guard<bmalloc::StaticMutex>&, unsigned long) + 20 2 JavaScriptCore bmalloc::Allocator::allocateXLarge(unsigned long) + 92 3 JavaScriptCore JSC::MachineThreads::gatherConservativeRoots(JSC::ConservativeRoots&, JSC::JITStubRoutineSet&, JSC::CodeBlockSet&, void*, void*, int (&) [48]) + 176 4 JavaScriptCore JSC::Heap::markRoots(double, void*, void*, int (&) [48]) + 384 5 JavaScriptCore JSC::Heap::collectImpl(JSC::HeapOperation, void*, void*, int (&) [48]) + 612 6 JavaScriptCore JSC::Heap::collect(JSC::HeapOperation) + 92 7 JavaScriptCore JSC::GCActivityCallback::doWork() + 88 8 JavaScriptCore JSC::HeapTimer::timerDidFire(__CFRunLoopTimer*, void*) + 216 9 CoreFoundation __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__ + 24 10 CoreFoundation __CFRunLoopDoTimer + 880 11 CoreFoundation __CFRunLoopRun + 1516 12 CoreFoundation CFRunLoopRunSpecific + 380 13 Facebook +[RCTJSCExecutor runRunLoopThread] (RCTJSCExecutor.mm:280) 14 Foundation __NSThread__start__ + 996 15 libsystem_pthread.dylib _pthread_body + 152 16 libsystem_pthread.dylib _pthread_start + 152 17 libsystem_pthread.dylib thread_start + 0 Basically, this is bmalloc crashing on purpose because vm_allocate can't allocate a large enough contiguous region to hold the target thread's stack, presumably because JSC thinks the stack is ginormous due to it having a SP of 0x0. It looks like the basic issue here is that JSC uses thread_get_state to the the stack pointer of the target thread. Unfortunately, thread_get_state sometimes returns a SP of 0x0 for a thread. See <rdar://27607384> and https://openradar.appspot.com/27607384 for more details. Basically, when the target thread is a dispatch queue thread that's just about to run with this call stack: Thread #32: 0 libsystem_pthread.dylib start_wqthread + 0 then thread_get_state may return 0x0 for the SP, causing the crashes in JSC GC outlined above. It's unclear to me whether this is actually a kernel bug or a JSC bug, but it seems like it might be a good idea for JSC to be defensive against a SP of 0x0 and have it abort marking if a thread is in this state. Additionally, it seems like MachineStackMarker.cpp should do some sort of sanity check on the size of the stack and not try to allocate a ginormous buffer to hold a thread's stack. For instance, on iOS I believe the maximum size of a stack is 1 MB, so JSC should never be allocating more than that amount of space to hold a single thread's stack. 1215591 1 ggaren 2016-07-29 10:48:25 -0700 I don't think we want to set a stack size maximum because stack size is configurable. But it sounds like we do need a specific workaround for the case where thread_get_state returns a bogus value. 1215592 2 webkit-bug-importer 2016-07-29 10:48:44 -0700 <rdar://problem/27611733> 1219285 3 nham 2016-08-11 02:12:58 -0700 Core OS wrote back in <rdar://27607384>: Can you guys use thread_get_register_pointer_values() as an alternative? This seems like it would fix the issue because thread_get_register_pointer_values appears to be a wrapper function around thread_get_state that a) filters out any pointers pointing to the null page and b) properly takes into account the red zone beneath the stack pointer. 1219288 4 nham 2016-08-11 02:19:55 -0700 Actually looking at this a bit more, I still don't think thread_get_register_pointer_values will do the right thing. For instance, on x86_64, it returns SP - 128 (taking in to account the red zone underneath the stack pointer): #elif defined(__x86_64__) if (sp) *sp = state.__rsp - 128 /* redzone */; This means that thread_get_register_pointer_values would actually return -128 when state.__rsp is 0x0, which is also an invalid stack pointer. 1219291 5 nham 2016-08-11 02:23:18 -0700 Ugh, I'm commenting too fast. thread_get_register_pointer_values just leaves the sp output parameter untouched when state.__rsp (or the equivalent register on another arch) is 0x0 and also returns KERN_SUCCESS. I don't see how this behavior would fix this crash. Seems like the simplest workaround is to ignore threads with null stack pointers. 1269078 6 hszhsh 2017-01-23 09:45:58 -0800 Is anyone working on this? I have similar issue with iOS 9. 1306930 7 309621 mark.lam 2017-05-10 11:24:24 -0700 Created attachment 309621 proposed patch. Let's try this on the EWS. 1306944 8 309621 fpizlo 2017-05-10 12:21:09 -0700 Comment on attachment 309621 proposed patch. r=me. Did you mean to set r?? 1306946 9 mark.lam 2017-05-10 12:27:51 -0700 (In reply to Filip Pizlo from comment #8) > Comment on attachment 309621 [details] > proposed patch. > > r=me. Did you mean to set r?? Oops. Yes, I forgot to set the r? after the EWS results were satisfactory. 1306947 10 309621 ggaren 2017-05-10 12:30:47 -0700 Comment on attachment 309621 proposed patch. r=me 1306963 11 mark.lam 2017-05-10 13:07:40 -0700 Thanks for the reviews. Landed in r216608: <http://trac.webkit.org/r216608>. 1306975 12 309621 ggaren 2017-05-10 13:25:07 -0700 Comment on attachment 309621 proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=309621&action=review > Source/JavaScriptCore/heap/MachineStackMarker.cpp:316 > + // This is a workaround for <rdar://problem/27607384>. During thread initialization, > + // for some target platforms, thread state is momentarily set to 0 before being Thinking about this again, I don't think this explanation can be correct. JSC only scans threads after they register themselves by invoking a JSC API. So, it's impossible that JSC would try to scan a thread that had not yet initialized. Given that, I don't think you've fixed this bug. 1306990 13 309621 mark.lam 2017-05-10 13:50:28 -0700 Comment on attachment 309621 proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=309621&action=review >> Source/JavaScriptCore/heap/MachineStackMarker.cpp:316 >> + // for some target platforms, thread state is momentarily set to 0 before being > > Thinking about this again, I don't think this explanation can be correct. > > JSC only scans threads after they register themselves by invoking a JSC API. So, it's impossible that JSC would try to scan a thread that had not yet initialized. Given that, I don't think you've fixed this bug. Read on below: "This issue may manifest with workqueue threads where the OS may choose to recycle a thread for an expired task." A more conclusive fix is to ensure that workqueue threads will properly invoke the thread destructor code, but short of that, this is a workaround that should work. 1307041 14 ggaren 2017-05-10 15:17:19 -0700 > Read on below: "This issue may manifest with workqueue threads where the OS > may choose to recycle a thread for an expired task." > > A more conclusive fix is to ensure that workqueue threads will properly > invoke the thread destructor code, but short of that, this is a workaround > that should work. If we believe that we have workqueue threads that terminate without invoking their destructors, then we may still have a bug where we scan those threads after they deallocate their stacks. Seems like we need closer investigation of the conditions that produce this null. 1307064 15 309621 ggaren 2017-05-10 16:08:03 -0700 Comment on attachment 309621 proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=309621&action=review > Source/JavaScriptCore/heap/MachineStackMarker.cpp:325 > + // This is a workaround for <rdar://problem/27607384>. During thread initialization, > + // for some target platforms, thread state is momentarily set to 0 before being > + // filled in with the target thread's real register values. As a result, there's > + // a race condition that may result in us getting a null stackPointer. > + // This issue may manifest with workqueue threads where the OS may choose to recycle > + // a thread for an expired task. > + // > + // The workaround is simply to indicate that there's nothing to copy and return. > + // This is correct because we will only ever observe a null pointer during thread > + // initialization. Hence, by definition, there's nothing there that we need to scan > + // yet, and therefore, nothing that needs to be copied. I would just say this: // This is a workaround for <rdar://problem/27607384>. libdispatch recycles work queue threads without running pthread exit destructors. This can cause us to scan a thread during work queue initialization, when the stack pointer is null. 1307079 16 mark.lam 2017-05-10 16:44:29 -0700 (In reply to Geoffrey Garen from comment #15) > I would just say this: > > // This is a workaround for <rdar://problem/27607384>. libdispatch recycles > work queue threads without running pthread exit destructors. This can cause > us to scan a thread during work queue initialization, when the stack pointer > is null. Landed this comment change in r216637: <http://trac.webkit.org/r216637>. 309621 2017-05-10 11:24:24 -0700 2017-05-10 12:30:47 -0700 proposed patch. bug-160337.patch text/plain 3084 mark.lam SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMjE2NTk2KQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDI2IEBA CisyMDE3LTA1LTEwICBNYXJrIExhbSAgPG1hcmsubGFtQGFwcGxlLmNvbT4KKworICAgICAgICBD cmFzaCBpbiBKYXZhU2NyaXB0Q29yZSBHQyB3aGVuIHVzaW5nIEpTQyBvbiBkaXNwYXRjaCBxdWV1 ZXMgKHRocmVhZF9nZXRfc3RhdGUgcmV0dXJucyBOVUxMIHN0YWNrIHBvaW50ZXIpLgorICAgICAg ICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTYwMzM3CisgICAgICAg IDxyZGFyOi8vcHJvYmxlbS8yNzYxMTczMz4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkg KE9PUFMhKS4KKworICAgICAgICBUaGlzIGlzIGEgd29ya2Fyb3VuZCBmb3IgPHJkYXI6Ly9wcm9i bGVtLzI3NjA3Mzg0Pi4gRHVyaW5nIHRocmVhZCBpbml0aWFsaXphdGlvbiwKKyAgICAgICAgZm9y IHNvbWUgdGFyZ2V0IHBsYXRmb3JtcywgdGhyZWFkIHN0YXRlIGlzIG1vbWVudGFyaWx5IHNldCB0 byAwIGJlZm9yZSBiZWluZworICAgICAgICBmaWxsZWQgaW4gd2l0aCB0aGUgdGFyZ2V0IHRocmVh ZCdzIHJlYWwgcmVnaXN0ZXIgdmFsdWVzLiBBcyBhIHJlc3VsdCwgdGhlcmUncworICAgICAgICBh IHJhY2UgY29uZGl0aW9uIHRoYXQgbWF5IHJlc3VsdCBpbiB1cyBnZXR0aW5nIGEgbnVsbCBzdGFj a1BvaW50ZXIgZHVyaW5nIGEgR0Mgc2Nhbi4KKyAgICAgICAgVGhpcyBpc3N1ZSBtYXkgbWFuaWZl c3Qgd2l0aCB3b3JrcXVldWUgdGhyZWFkcyB3aGVyZSB0aGUgT1MgbWF5IGNob29zZSB0byByZWN5 Y2xlCisgICAgICAgIGEgdGhyZWFkIGZvciBhbiBleHBpcmVkIHRhc2suCisKKyAgICAgICAgVGhl IHdvcmthcm91bmQgaXMgc2ltcGx5IHRvIGluZGljYXRlIHRoYXQgdGhlcmUncyBub3RoaW5nIHRv IGNvcHkgYW5kIHJldHVybi4KKyAgICAgICAgVGhpcyBpcyBjb3JyZWN0IGJlY2F1c2Ugd2Ugd2ls bCBvbmx5IGV2ZXIgb2JzZXJ2ZSBhIG51bGwgcG9pbnRlciBkdXJpbmcgdGhyZWFkCisgICAgICAg IGluaXRpYWxpemF0aW9uLiBIZW5jZSwgYnkgZGVmaW5pdGlvbiwgdGhlcmUncyBub3RoaW5nIHRo ZXJlIHRoYXQgd2UgbmVlZCB0byBzY2FuCisgICAgICAgIHlldCwgYW5kIHRoZXJlZm9yZSwgbm90 aGluZyB0aGF0IG5lZWRzIHRvIGJlIGNvcGllZC4KKworICAgICAgICAqIGhlYXAvTWFjaGluZVN0 YWNrTWFya2VyLmNwcDoKKyAgICAgICAgKEpTQzo6TWFjaGluZVRocmVhZHM6OnRyeUNvcHlPdGhl clRocmVhZFN0YWNrKToKKwogMjAxNy0wNS0xMCAgRmlsaXAgUGl6bG8gIDxmcGl6bG9AYXBwbGUu Y29tPgogCiAgICAgICAgIE51bGwgcG9pbnRlciBkZXJlZmVyZW5jZSBpbiBXVEY6OlJlZlB0cjxX VEY6OlN0cmluZ0ltcGw+OjpvcGVyYXRvciEoKSB1bmRlciBzbG93X3BhdGhfZ2V0X2RpcmVjdF9w bmFtZQpJbmRleDogU291cmNlL0phdmFTY3JpcHRDb3JlL2hlYXAvTWFjaGluZVN0YWNrTWFya2Vy LmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvSmF2YVNjcmlwdENvcmUvaGVhcC9NYWNoaW5lU3Rh Y2tNYXJrZXIuY3BwCShyZXZpc2lvbiAyMTY1OTYpCisrKyBTb3VyY2UvSmF2YVNjcmlwdENvcmUv aGVhcC9NYWNoaW5lU3RhY2tNYXJrZXIuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0zMTEsNiArMzEx LDIzIEBAIHZvaWQgTWFjaGluZVRocmVhZHM6OnRyeUNvcHlPdGhlclRocmVhZFMKIHsKICAgICBN YWNoaW5lVGhyZWFkOjpSZWdpc3RlcnMgcmVnaXN0ZXJzOwogICAgIHNpemVfdCByZWdpc3RlcnNT aXplID0gdGhyZWFkLT5nZXRSZWdpc3RlcnMocmVnaXN0ZXJzKTsKKworICAgIC8vIFRoaXMgaXMg YSB3b3JrYXJvdW5kIGZvciA8cmRhcjovL3Byb2JsZW0vMjc2MDczODQ+LiBEdXJpbmcgdGhyZWFk IGluaXRpYWxpemF0aW9uLAorICAgIC8vIGZvciBzb21lIHRhcmdldCBwbGF0Zm9ybXMsIHRocmVh ZCBzdGF0ZSBpcyBtb21lbnRhcmlseSBzZXQgdG8gMCBiZWZvcmUgYmVpbmcKKyAgICAvLyBmaWxs ZWQgaW4gd2l0aCB0aGUgdGFyZ2V0IHRocmVhZCdzIHJlYWwgcmVnaXN0ZXIgdmFsdWVzLiBBcyBh IHJlc3VsdCwgdGhlcmUncworICAgIC8vIGEgcmFjZSBjb25kaXRpb24gdGhhdCBtYXkgcmVzdWx0 IGluIHVzIGdldHRpbmcgYSBudWxsIHN0YWNrUG9pbnRlci4KKyAgICAvLyBUaGlzIGlzc3VlIG1h eSBtYW5pZmVzdCB3aXRoIHdvcmtxdWV1ZSB0aHJlYWRzIHdoZXJlIHRoZSBPUyBtYXkgY2hvb3Nl IHRvIHJlY3ljbGUKKyAgICAvLyBhIHRocmVhZCBmb3IgYW4gZXhwaXJlZCB0YXNrLgorICAgIC8v CisgICAgLy8gVGhlIHdvcmthcm91bmQgaXMgc2ltcGx5IHRvIGluZGljYXRlIHRoYXQgdGhlcmUn cyBub3RoaW5nIHRvIGNvcHkgYW5kIHJldHVybi4KKyAgICAvLyBUaGlzIGlzIGNvcnJlY3QgYmVj YXVzZSB3ZSB3aWxsIG9ubHkgZXZlciBvYnNlcnZlIGEgbnVsbCBwb2ludGVyIGR1cmluZyB0aHJl YWQKKyAgICAvLyBpbml0aWFsaXphdGlvbi4gSGVuY2UsIGJ5IGRlZmluaXRpb24sIHRoZXJlJ3Mg bm90aGluZyB0aGVyZSB0aGF0IHdlIG5lZWQgdG8gc2NhbgorICAgIC8vIHlldCwgYW5kIHRoZXJl Zm9yZSwgbm90aGluZyB0aGF0IG5lZWRzIHRvIGJlIGNvcGllZC4KKyAgICBpZiAoVU5MSUtFTFko IXJlZ2lzdGVycy5zdGFja1BvaW50ZXIoKSkpIHsKKyAgICAgICAgKnNpemUgPSAwOworICAgICAg ICByZXR1cm47CisgICAgfQorCiAgICAgc3RkOjpwYWlyPHZvaWQqLCBzaXplX3Q+IHN0YWNrID0g dGhyZWFkLT5jYXB0dXJlU3RhY2socmVnaXN0ZXJzLnN0YWNrUG9pbnRlcigpKTsKIAogICAgIGJv b2wgY2FuQ29weSA9ICpzaXplICsgcmVnaXN0ZXJzU2l6ZSArIHN0YWNrLnNlY29uZCA8PSBjYXBh Y2l0eTsK