180373 2017-12-04 13:12:41 -0800 Web Inspector: Crashes seen under Inspector::ScriptCallFrame::~ScriptCallFrame 2018-12-21 15:51:49 -0800 1 1 1 Unclassified WebKit Web Inspector WebKit Nightly Build All All RESOLVED FIXED InRadar P2 Normal --- 1 joepeck webkit-unassigned commit-queue ews-watchlist hi inspector-bugzilla-changes joepeck keith_miller mark.lam mattbaker msaboff saam simon.fraser webkit-bug-importer xpeng1984 oldest_to_newest 1377846 0 joepeck 2017-12-04 13:12:41 -0800 Crashes seen under ScriptCallFrame::~ScriptCallFrame: > Responsible: Safari Technology Preview [9088] > OS Version: Mac OS X 10.12.6 (16G29) > Crashed Thread: 0 Dispatch queue: com.apple.main-thread > > Exception Type: EXC_BAD_ACCESS (SIGSEGV) > Exception Codes: KERN_INVALID_ADDRESS at 0x0000000000000008 > Exception Note: EXC_CORPSE_NOTIFY > > Termination Signal: Segmentation fault: 11 > Termination Reason: Namespace SIGNAL, Code 0xb > Terminating Process: exc handler [0] > > > Thread 0 Crashed:: Dispatch queue: com.apple.main-thread > 0 com.apple.JavaScriptCore 0x0000000111bfa5f9 Inspector::ScriptCallFrame::~ScriptCallFrame() + 9 > 1 com.apple.JavaScriptCore 0x0000000111bfaaa8 Inspector::ScriptCallStack::~ScriptCallStack() + 40 > 2 com.apple.JavaScriptCore 0x00000001114c463b Inspector::AsyncStackTrace::~AsyncStackTrace() + 139 > 3 com.apple.JavaScriptCore 0x00000001114c4848 Inspector::AsyncStackTrace::truncate(unsigned long) + 424 > 4 com.apple.JavaScriptCore 0x0000000111946f68 Inspector::InspectorDebuggerAgent::willDispatchAsyncCall(int, int) + 136 > 5 com.apple.WebCore 0x000000010f241fad WebCore::InspectorInstrumentation::willFireTimerImpl(WebCore::InstrumentingAgents&, int, WebCore::ScriptExecutionContext&) + 125 > 6 com.apple.WebCore 0x000000010eb76e7f WebCore::DOMTimer::fired() + 927 > 7 com.apple.WebCore 0x000000010eaa7100 WebCore::ThreadTimers::sharedTimerFiredInternal() + 176 > 8 com.apple.WebCore 0x000000010eaa703f WebCore::timerFired(__CFRunLoopTimer*, void*) + 31 > 9 com.apple.CoreFoundation 0x00007fff96592c54 __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__ + 20 > 10 com.apple.CoreFoundation 0x00007fff965928df __CFRunLoopDoTimer + 1071 > 11 com.apple.CoreFoundation 0x00007fff9659243a __CFRunLoopDoTimers + 298 > 12 com.apple.CoreFoundation 0x00007fff96589b81 __CFRunLoopRun + 2065 > 13 com.apple.CoreFoundation 0x00007fff96589114 CFRunLoopRunSpecific + 420 > 14 com.apple.HIToolbox 0x00007fff95ae9ebc RunCurrentEventLoopInMode + 240 > 15 com.apple.HIToolbox 0x00007fff95ae9cf1 ReceiveNextEventCommon + 432 > 16 com.apple.HIToolbox 0x00007fff95ae9b26 _BlockUntilNextEventMatchingListInModeWithFilter + 71 > 17 com.apple.AppKit 0x00007fff94082a54 _DPSNextEvent + 1120 > 18 com.apple.AppKit 0x00007fff947fe7ee -[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:] + 2796 > 19 com.apple.AppKit 0x00007fff940773db -[NSApplication run] + 926 > 20 com.apple.AppKit 0x00007fff94041e0e NSApplicationMain + 1237 > 21 libxpc.dylib 0x00007fffabf628c7 _xpc_objc_main + 775 > 22 libxpc.dylib 0x00007fffabf612e4 xpc_main + 494 > 23 com.apple.WebKit.WebContent 0x10df0b695 main + 492 > 24 libdyld.dylib 0x00007fffabd09235 start + 1 1377847 1 joepeck 2017-12-04 13:12:53 -0800 <rdar://problem/33894170> 1490490 2 joepeck 2018-12-21 14:39:12 -0800 *** Bug 179998 has been marked as a duplicate of this bug. *** 1490491 3 joepeck 2018-12-21 14:39:23 -0800 <rdar://problem/33726908> 1490496 4 joepeck 2018-12-21 14:41:36 -0800 Was able to reproduce this with LayoutTests: $ run-webkit-tests --debug --no-retry-failures --no-sample-on-timeout --additional-env-var=JSC_slowPathAllocsBetweenGCs=10 --force --verbose --iterations=100 --exit-after-n-failures=1 --guard-malloc -1 --time-out-ms=50000 inspector/debugger/truncate-async-stack-trace.html Seems the relevant code is: ---- auto* previousNode = lastUnlockedAncestor; // The subtree being truncated must be removed from it's parent before // updating its parent pointer chain. auto* sourceNode = lastUnlockedAncestor->m_parent.get(); lastUnlockedAncestor->remove(); while (sourceNode) { previousNode->m_parent = AsyncStackTrace::create(sourceNode->m_callStack.copyRef(), true, nullptr); ... } ---- The `lastUnlockedAncestor->remove()` releases the last reference on `lastUnlockedAncestor->m_parent` so it gets destroyed and sourceNode is invalid. We should protect it or use RefPtr throughout this code. It looks like we can get away with protecting just that top level parent. 1490502 5 357982 joepeck 2018-12-21 14:47:19 -0800 Created attachment 357982 [PATCH] Proposed Fix 1490505 6 357982 hi 2018-12-21 14:56:26 -0800 Comment on attachment 357982 [PATCH] Proposed Fix rs=me, I think it's "safer" to just not call `.get()`, but as you pointed out, there's nothing really happening in the loop, so I guess it's fine. Hopefully our future selves don't mess it up 😛 1490506 7 357982 simon.fraser 2018-12-21 14:59:23 -0800 Comment on attachment 357982 [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=357982&action=review > Source/JavaScriptCore/inspector/AsyncStackTrace.cpp:172 > + RefPtr<AsyncStackTrace> protect(lastUnlockedAncestor->m_parent); > auto* sourceNode = lastUnlockedAncestor->m_parent.get(); Why not just RefPtr<AsyncStackTrace> sourceNode = lastUnlockedAncestor->m_parent; ? 1490512 8 joepeck 2018-12-21 15:02:47 -0800 (In reply to Simon Fraser (smfr) from comment #7) > Comment on attachment 357982 [details] > [PATCH] Proposed Fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=357982&action=review > > > Source/JavaScriptCore/inspector/AsyncStackTrace.cpp:172 > > + RefPtr<AsyncStackTrace> protect(lastUnlockedAncestor->m_parent); > > auto* sourceNode = lastUnlockedAncestor->m_parent.get(); > > Why not just > RefPtr<AsyncStackTrace> sourceNode = lastUnlockedAncestor->m_parent; ? Okay will do. It has a bit more ref churn in the loop but is ultimately safer. 1490513 9 357987 joepeck 2018-12-21 15:03:54 -0800 Created attachment 357987 [PATCH] For Landing 1490520 10 mattbaker 2018-12-21 15:25:03 -0800 (In reply to Joseph Pecoraro from comment #9) > Created attachment 357987 [details] > [PATCH] For Landing Nice work! 1490533 11 joepeck 2018-12-21 15:49:48 -0800 <https://trac.webkit.org/r239525> 1490536 12 commit-queue 2018-12-21 15:51:49 -0800 The commit-queue encountered the following flaky tests while processing attachment 357987: http/wpt/css/css-animations/start-animation-001.html bug 190903 (authors: dino@apple.com, fred.wang@free.fr, and graouts@apple.com) The commit-queue is continuing to process your patch. 357982 2018-12-21 14:47:19 -0800 2018-12-21 15:03:07 -0800 [PATCH] Proposed Fix protect-1.patch text/plain 1685 joepeck ZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cgYi9Tb3VyY2UvSmF2 YVNjcmlwdENvcmUvQ2hhbmdlTG9nCmluZGV4IDJlN2MzZWExMzFhLi5mNmYwZDE5ZmQ0OCAxMDA2 NDQKLS0tIGEvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZworKysgYi9Tb3VyY2UvSmF2 YVNjcmlwdENvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTcgQEAKKzIwMTgtMTItMjEgIEpvc2Vw aCBQZWNvcmFybyAgPHBlY29yYXJvQGFwcGxlLmNvbT4KKworICAgICAgICBXZWIgSW5zcGVjdG9y OiBDcmFzaGVzIHNlZW4gdW5kZXIgSW5zcGVjdG9yOjpTY3JpcHRDYWxsRnJhbWU6On5TY3JpcHRD YWxsRnJhbWUKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lk PTE4MDM3MworICAgICAgICA8cmRhcjovL3Byb2JsZW0vMzM4OTQxNzA+CisKKyAgICAgICAgUmV2 aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgKiBpbnNwZWN0b3IvQXN5bmNTdGFj a1RyYWNlLmNwcDoKKyAgICAgICAgKEluc3BlY3Rvcjo6QXN5bmNTdGFja1RyYWNlOjp0cnVuY2F0 ZSk6CisgICAgICAgIFRoZSBgbGFzdFVubG9ja2VkQW5jZXN0b3ItPnJlbW92ZSgpYCBtYXkgcmVs ZWFzZSB0aGUgb25seSByZWZlcmVuY2UgdG8gaXQncworICAgICAgICBwYXJlbnQgd2hpY2ggd2Ug aW50ZW5kIHRvIHVzZSBsYXRlciBidXQgZG9uJ3QgaG9sZCBhIFJlZlB0ciB0by4gS2VlcCB0aGUK KyAgICAgICAgcGFyZW50IGFsaXZlIGV4cGxpY2l0bHkgYnkgcHJvdGVjdGluZyBpdC4KKwogMjAx OC0xMi0yMCAgS2VpdGggTWlsbGVyICA8a2VpdGhfbWlsbGVyQGFwcGxlLmNvbT4KIAogICAgICAg ICBBZGQgc3VwcG9ydCBmb3IgZ2xvYmFsVGhpcwpkaWZmIC0tZ2l0IGEvU291cmNlL0phdmFTY3Jp cHRDb3JlL2luc3BlY3Rvci9Bc3luY1N0YWNrVHJhY2UuY3BwIGIvU291cmNlL0phdmFTY3JpcHRD b3JlL2luc3BlY3Rvci9Bc3luY1N0YWNrVHJhY2UuY3BwCmluZGV4IDhjYjA5Nzk2ZjEyLi5iNTZl ZjFkNTdiNyAxMDA2NDQKLS0tIGEvU291cmNlL0phdmFTY3JpcHRDb3JlL2luc3BlY3Rvci9Bc3lu Y1N0YWNrVHJhY2UuY3BwCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9pbnNwZWN0b3IvQXN5 bmNTdGFja1RyYWNlLmNwcApAQCAtMTY2LDcgKzE2Niw5IEBAIHZvaWQgQXN5bmNTdGFja1RyYWNl Ojp0cnVuY2F0ZShzaXplX3QgbWF4RGVwdGgpCiAgICAgYXV0byogcHJldmlvdXNOb2RlID0gbGFz dFVubG9ja2VkQW5jZXN0b3I7CiAKICAgICAvLyBUaGUgc3VidHJlZSBiZWluZyB0cnVuY2F0ZWQg bXVzdCBiZSByZW1vdmVkIGZyb20gaXQncyBwYXJlbnQgYmVmb3JlCi0gICAgLy8gdXBkYXRpbmcg aXRzIHBhcmVudCBwb2ludGVyIGNoYWluLgorICAgIC8vIHVwZGF0aW5nIGl0cyBwYXJlbnQgcG9p bnRlciBjaGFpbi4gUHJvdGVjdCB0aGUgcGFyZW50IG5vZGUgaW4gY2FzZQorICAgIC8vIHRoaXMg aG9sZHMgdGhlIG9ubHkgcmVmZXJlbmNlIHRvIGl0LgorICAgIFJlZlB0cjxBc3luY1N0YWNrVHJh Y2U+IHByb3RlY3QobGFzdFVubG9ja2VkQW5jZXN0b3ItPm1fcGFyZW50KTsKICAgICBhdXRvKiBz b3VyY2VOb2RlID0gbGFzdFVubG9ja2VkQW5jZXN0b3ItPm1fcGFyZW50LmdldCgpOwogICAgIGxh c3RVbmxvY2tlZEFuY2VzdG9yLT5yZW1vdmUoKTsKIAo= 357987 2018-12-21 15:03:54 -0800 2018-12-21 15:48:50 -0800 [PATCH] For Landing for-landing-1.patch text/plain 1990 joepeck ZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cgYi9Tb3VyY2UvSmF2 YVNjcmlwdENvcmUvQ2hhbmdlTG9nCmluZGV4IDJlN2MzZWExMzFhLi5hNzYwZTM5MTdlNiAxMDA2 NDQKLS0tIGEvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZworKysgYi9Tb3VyY2UvSmF2 YVNjcmlwdENvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTcgQEAKKzIwMTgtMTItMjEgIEpvc2Vw aCBQZWNvcmFybyAgPHBlY29yYXJvQGFwcGxlLmNvbT4KKworICAgICAgICBXZWIgSW5zcGVjdG9y OiBDcmFzaGVzIHNlZW4gdW5kZXIgSW5zcGVjdG9yOjpTY3JpcHRDYWxsRnJhbWU6On5TY3JpcHRD YWxsRnJhbWUKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lk PTE4MDM3MworICAgICAgICA8cmRhcjovL3Byb2JsZW0vMzM4OTQxNzA+CisKKyAgICAgICAgUnVi YmVyLXN0YW1wZWQgYnkgRGV2aW4gUm91c3NvLgorCisgICAgICAgICogaW5zcGVjdG9yL0FzeW5j U3RhY2tUcmFjZS5jcHA6CisgICAgICAgIChJbnNwZWN0b3I6OkFzeW5jU3RhY2tUcmFjZTo6dHJ1 bmNhdGUpOgorICAgICAgICBUaGUgYGxhc3RVbmxvY2tlZEFuY2VzdG9yLT5yZW1vdmUoKWAgbWF5 IHJlbGVhc2UgdGhlIG9ubHkgcmVmZXJlbmNlIHRvIGl0J3MKKyAgICAgICAgcGFyZW50IHdoaWNo IHdlIGludGVuZCB0byB1c2UgbGF0ZXIgYnV0IGRvbid0IGhvbGQgYSBSZWZQdHIgdG8uIEtlZXAg dGhlCisgICAgICAgIHBhcmVudCBhbGl2ZSBleHBsaWNpdGx5IGJ5IHByb3RlY3RpbmcgaXQuCisK IDIwMTgtMTItMjAgIEtlaXRoIE1pbGxlciAgPGtlaXRoX21pbGxlckBhcHBsZS5jb20+CiAKICAg ICAgICAgQWRkIHN1cHBvcnQgZm9yIGdsb2JhbFRoaXMKZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZh U2NyaXB0Q29yZS9pbnNwZWN0b3IvQXN5bmNTdGFja1RyYWNlLmNwcCBiL1NvdXJjZS9KYXZhU2Ny aXB0Q29yZS9pbnNwZWN0b3IvQXN5bmNTdGFja1RyYWNlLmNwcAppbmRleCA4Y2IwOTc5NmYxMi4u NTQ1MGUyZjNjMDAgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9pbnNwZWN0b3Iv QXN5bmNTdGFja1RyYWNlLmNwcAorKysgYi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvaW5zcGVjdG9y L0FzeW5jU3RhY2tUcmFjZS5jcHAKQEAgLTE2Nyw3ICsxNjcsNyBAQCB2b2lkIEFzeW5jU3RhY2tU cmFjZTo6dHJ1bmNhdGUoc2l6ZV90IG1heERlcHRoKQogCiAgICAgLy8gVGhlIHN1YnRyZWUgYmVp bmcgdHJ1bmNhdGVkIG11c3QgYmUgcmVtb3ZlZCBmcm9tIGl0J3MgcGFyZW50IGJlZm9yZQogICAg IC8vIHVwZGF0aW5nIGl0cyBwYXJlbnQgcG9pbnRlciBjaGFpbi4KLSAgICBhdXRvKiBzb3VyY2VO b2RlID0gbGFzdFVubG9ja2VkQW5jZXN0b3ItPm1fcGFyZW50LmdldCgpOworICAgIFJlZlB0cjxB c3luY1N0YWNrVHJhY2U+IHNvdXJjZU5vZGUgPSBsYXN0VW5sb2NrZWRBbmNlc3Rvci0+bV9wYXJl bnQ7CiAgICAgbGFzdFVubG9ja2VkQW5jZXN0b3ItPnJlbW92ZSgpOwogCiAgICAgd2hpbGUgKHNv dXJjZU5vZGUpIHsKQEAgLTE3NSwxMCArMTc1LDEwIEBAIHZvaWQgQXN5bmNTdGFja1RyYWNlOjp0 cnVuY2F0ZShzaXplX3QgbWF4RGVwdGgpCiAgICAgICAgIHByZXZpb3VzTm9kZS0+bV9wYXJlbnQt Pm1fY2hpbGRDb3VudCA9IDE7CiAgICAgICAgIHByZXZpb3VzTm9kZSA9IHByZXZpb3VzTm9kZS0+ bV9wYXJlbnQuZ2V0KCk7CiAKLSAgICAgICAgaWYgKHNvdXJjZU5vZGUgPT0gbmV3U3RhY2tUcmFj ZVJvb3QpCisgICAgICAgIGlmIChzb3VyY2VOb2RlLmdldCgpID09IG5ld1N0YWNrVHJhY2VSb290 KQogICAgICAgICAgICAgYnJlYWs7CiAKLSAgICAgICAgc291cmNlTm9kZSA9IHNvdXJjZU5vZGUt Pm1fcGFyZW50LmdldCgpOworICAgICAgICBzb3VyY2VOb2RlID0gc291cmNlTm9kZS0+bV9wYXJl bnQ7CiAgICAgfQogCiAgICAgcHJldmlvdXNOb2RlLT5tX3RydW5jYXRlZCA9IHRydWU7Cg==