Bug 173840 – Web Inspector: AsyncStackTrace nodes can be corrupted when truncating

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Matt Baker]

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__

[Matt Baker]

<rdar://problem/30840820>

[Matt Baker]

Created attachment 313856 [details]
Patch

[Mark Lam]

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 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]

Created attachment 313965 [details]
Patch

[Matt Baker]

Updated change log to better explain the problem being solved.

[Joseph Pecoraro]

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 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]

Created attachment 314326 [details]
Patch

[WebKit Commit Bot]

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 on attachment 314326 [details]
Patch

Clearing flags on attachment: 314326

Committed r219035: <http://trac.webkit.org/changeset/219035>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.