Bug 180373 – Web Inspector: Crashes seen under Inspector::ScriptCallFrame::~ScriptCallFrame

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

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

[Joseph Pecoraro]

<rdar://problem/33894170>

[Joseph Pecoraro]

*** Bug 179998 has been marked as a duplicate of this bug. ***

[Joseph Pecoraro]

<rdar://problem/33726908>

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

Created attachment 357982 [details]
[PATCH] Proposed Fix

[Devin Rousso]

Comment on attachment 357982 [details]
[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 😛

[Simon Fraser (smfr)]

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; ?

[Joseph Pecoraro]

(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.

[Joseph Pecoraro]

Created attachment 357987 [details]
[PATCH] For Landing

[Matt Baker]

(In reply to Joseph Pecoraro from comment #9)
> Created attachment 357987 [details]
> [PATCH] For Landing

Nice work!

[Joseph Pecoraro]

<https://trac.webkit.org/r239525>

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 357987 [details]:

http/wpt/css/css-animations/start-animation-001.html bug 190903 (authors: [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.