Bug 173840

Summary: Web Inspector: AsyncStackTrace nodes can be corrupted when truncating
Product: WebKit Reporter: Matt Baker <mattbaker>
Component: Web InspectorAssignee: Matt Baker <mattbaker>
Status: RESOLVED FIXED    
Severity: Critical CC: buildbot, commit-queue, inspector-bugzilla-changes, joepeck, keith_miller, mark.lam, msaboff, saam
Priority: P1 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Matt Baker
Reported 2017-06-26 10:47:15 PDT
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__
Attachments
Patch (6.09 KB, patch)
2017-06-26 11:49 PDT, Matt Baker 		no flags
DetailsFormatted DiffDiff
Patch (6.50 KB, patch)
2017-06-27 17:41 PDT, Matt Baker 		no flags
DetailsFormatted DiffDiff
Patch (6.45 KB, patch)
2017-06-30 16:57 PDT, Matt Baker 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Matt Baker
Comment 1 2017-06-26 10:47:38 PDT
<rdar://problem/30840820>
Matt Baker
Comment 2 2017-06-26 11:49:09 PDT
Created attachment 313856 [details] Patch
Mark Lam
Comment 3 2017-06-27 14:02:46 PDT
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?
Matt Baker
Comment 4 2017-06-27 17:07:10 PDT
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.
Matt Baker
Comment 5 2017-06-27 17:41:06 PDT
Created attachment 313965 [details] Patch
Matt Baker
Comment 6 2017-06-27 17:41:45 PDT
Updated change log to better explain the problem being solved.
Joseph Pecoraro
Comment 7 2017-06-30 14:05:16 PDT
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.
Matt Baker
Comment 8 2017-06-30 16:54:21 PDT
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.
Matt Baker
Comment 9 2017-06-30 16:57:13 PDT
Created attachment 314326 [details] Patch
WebKit Commit Bot
Comment 10 2017-06-30 17:46:01 PDT
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.
WebKit Commit Bot
Comment 11 2017-06-30 19:56:08 PDT
Comment on attachment 314326 [details] Patch Clearing flags on attachment: 314326 Committed r219035: <http://trac.webkit.org/changeset/219035>
WebKit Commit Bot
Comment 12 2017-06-30 19:56:09 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.