Bug 180373 - Web Inspector: Crashes seen under Inspector::ScriptCallFrame::~ScriptCallFrame
Summary: Web Inspector: Crashes seen under Inspector::ScriptCallFrame::~ScriptCallFrame
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
: 179998 (view as bug list)
Depends on:
Blocks:
 
Reported: 2017-12-04 13:12 PST by Joseph Pecoraro
Modified: 2018-12-21 15:51 PST (History)
13 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (1.65 KB, patch)
2018-12-21 14:47 PST, Joseph Pecoraro
drousso: review+
Details | Formatted Diff | Diff
[PATCH] For Landing (1.94 KB, patch)
2018-12-21 15:03 PST, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2017-12-04 13:12:41 PST
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
Comment 1 Joseph Pecoraro 2017-12-04 13:12:53 PST
<rdar://problem/33894170>
Comment 2 Joseph Pecoraro 2018-12-21 14:39:12 PST
*** Bug 179998 has been marked as a duplicate of this bug. ***
Comment 3 Joseph Pecoraro 2018-12-21 14:39:23 PST
<rdar://problem/33726908>
Comment 4 Joseph Pecoraro 2018-12-21 14:41:36 PST
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.
Comment 5 Joseph Pecoraro 2018-12-21 14:47:19 PST
Created attachment 357982 [details]
[PATCH] Proposed Fix
Comment 6 Devin Rousso 2018-12-21 14:56:26 PST
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 😛
Comment 7 Simon Fraser (smfr) 2018-12-21 14:59:23 PST
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; ?
Comment 8 Joseph Pecoraro 2018-12-21 15:02:47 PST
(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.
Comment 9 Joseph Pecoraro 2018-12-21 15:03:54 PST
Created attachment 357987 [details]
[PATCH] For Landing
Comment 10 Matt Baker 2018-12-21 15:25:03 PST
(In reply to Joseph Pecoraro from comment #9)
> Created attachment 357987 [details]
> [PATCH] For Landing

Nice work!
Comment 11 Joseph Pecoraro 2018-12-21 15:49:48 PST
<https://trac.webkit.org/r239525>
Comment 12 WebKit Commit Bot 2018-12-21 15:51:49 PST
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: dino@apple.com, fred.wang@free.fr, and graouts@apple.com)
The commit-queue is continuing to process your patch.