Summary: AsyncStackTrace nodes can be corrupted when truncating. This only occur when nodes in the path being truncated are shared by multiple traces. Steps to Reproduce: 1. Goto arstechnica.com 2. Open Inspector 3. Quickly scroll the page => Crash ASSERTION FAILED: m_parent->m_childCount /Volumes/Data/Projects/WebKit/OpenSource/Source/JavaScriptCore/inspector/AsyncStackTrace.cpp(189) : void Inspector::AsyncStackTrace::remove() 1 0x12261bc5d WTFCrash 2 0x1214a74b8 Inspector::AsyncStackTrace::remove() 3 0x1214a73c6 Inspector::AsyncStackTrace::~AsyncStackTrace() 4 0x1214a7555 Inspector::AsyncStackTrace::~AsyncStackTrace() 5 0x1214ab567 WTF::RefCounted<Inspector::AsyncStackTrace>::deref() const 6 0x1214ab511 void WTF::derefIfNotNull<Inspector::AsyncStackTrace>(Inspector::AsyncStackTrace*) 7 0x1214a815b WTF::RefPtr<Inspector::AsyncStackTrace>::operator=(std::nullptr_t) 8 0x1214a74ea Inspector::AsyncStackTrace::remove() 9 0x1214a7903 Inspector::AsyncStackTrace::didDispatchAsyncCall() 10 0x121e8f67f Inspector::InspectorDebuggerAgent::didDispatchAsyncCall() 11 0x115f4bf35 WebCore::InspectorInstrumentation::didFireTimerImpl(WebCore::InspectorInstrumentationCookie const&) 12 0x1156fff28 WebCore::InspectorInstrumentation::didFireTimer(WebCore::InspectorInstrumentationCookie const&) 13 0x1156ffba5 WebCore::DOMTimer::fired() 14 0x1179986c4 WebCore::ThreadTimers::sharedTimerFiredInternal() 15 0x117998e31 WebCore::ThreadTimers::setSharedTimer(WebCore::SharedTimer*)::$_0::operator()() const 16 0x117998de9 WTF::Function<void ()>::CallableWrapper<WebCore::ThreadTimers::setSharedTimer(WebCore::SharedTimer*)::$_0>::call() 17 0x114efbbde WTF::Function<void ()>::operator()() const 18 0x116c7a415 WebCore::MainThreadSharedTimer::fired() 19 0x116c7a7a9 WebCore::timerFired(__CFRunLoopTimer*, void*) 20 0x10ac0d074 __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__
<rdar://problem/30840820>
Created attachment 313856 [details] Patch
Comment on attachment 313856 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313856&action=review > Source/JavaScriptCore/ChangeLog:10 > + node counts to be incorrectly incremented and parent pointers to shared typo: /pointers to shared/pointers to be shared/. > Source/JavaScriptCore/inspector/AsyncStackTrace.cpp:-177 > - currentNode->remove(); Hmmm, this part is different. Previously, you invoke remove() on the oldest parent (because currentNode is updated to be currentNode->m_parent.get() in the loop). Now, you remove the lastUnlockedAncestor before traversing up the parent chain. Is this intentional?
Comment on attachment 313856 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313856&action=review >> Source/JavaScriptCore/inspector/AsyncStackTrace.cpp:-177 >> - currentNode->remove(); > > Hmmm, this part is different. Previously, you invoke remove() on the oldest parent (because currentNode is updated to be currentNode->m_parent.get() in the loop). Now, you remove the lastUnlockedAncestor before traversing up the parent chain. Is this intentional? This is intentional. The subtree at `lastUnlockedAncestor` needs to be removed from its parent, and the rest of the tree (from the first locked ancestor to the root) should be left alone. The locked portion will be cloned and become the root of a new tree, to which `lastLockedAncestor` will be appended.
Created attachment 313965 [details] Patch
Updated change log to better explain the problem being solved.
Comment on attachment 313965 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313965&action=review > LayoutTests/inspector/debugger/truncate-async-stack-trace.html:5 > +<script src="resources/log-active-stack-trace.js"></script> Is this script needed? Should the test be calling window.logActiveStackTrace to verify something? Right now it just confirms it can pause, which I assume is verifying that we aren't ASSERTing.
Comment on attachment 313965 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313965&action=review >> LayoutTests/inspector/debugger/truncate-async-stack-trace.html:5 >> +<script src="resources/log-active-stack-trace.js"></script> > > Is this script needed? Should the test be calling window.logActiveStackTrace to verify something? Right now it just confirms it can pause, which I assume is verifying that we aren't ASSERTing. Debugging leftovers. Will remove.
Created attachment 314326 [details] Patch
The commit-queue encountered the following flaky tests while processing attachment 314326 [details]: fast/workers/worker-exception-during-navigation.html bug 174058 The commit-queue is continuing to process your patch.
Comment on attachment 314326 [details] Patch Clearing flags on attachment: 314326 Committed r219035: <http://trac.webkit.org/changeset/219035>
All reviewed patches have been landed. Closing bug.