Bug 173840 - Web Inspector: AsyncStackTrace nodes can be corrupted when truncating
Summary: Web Inspector: AsyncStackTrace nodes can be corrupted when truncating
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P1 Critical
Assignee: Matt Baker
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-06-26 10:47 PDT by Matt Baker
Modified: 2017-06-30 19:56 PDT (History)
8 users (show)

See Also:


Attachments
Patch (6.09 KB, patch)
2017-06-26 11:49 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Patch (6.50 KB, patch)
2017-06-27 17:41 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Patch (6.45 KB, patch)
2017-06-30 16:57 PDT, Matt Baker
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Baker 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__
Comment 1 Matt Baker 2017-06-26 10:47:38 PDT
<rdar://problem/30840820>
Comment 2 Matt Baker 2017-06-26 11:49:09 PDT
Created attachment 313856 [details]
Patch
Comment 3 Mark Lam 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?
Comment 4 Matt Baker 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.
Comment 5 Matt Baker 2017-06-27 17:41:06 PDT
Created attachment 313965 [details]
Patch
Comment 6 Matt Baker 2017-06-27 17:41:45 PDT
Updated change log to better explain the problem being solved.
Comment 7 Joseph Pecoraro 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.
Comment 8 Matt Baker 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.
Comment 9 Matt Baker 2017-06-30 16:57:13 PDT
Created attachment 314326 [details]
Patch
Comment 10 WebKit Commit Bot 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.
Comment 11 WebKit Commit Bot 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>
Comment 12 WebKit Commit Bot 2017-06-30 19:56:09 PDT
All reviewed patches have been landed.  Closing bug.