Bug 180373

Summary: Web Inspector: Crashes seen under Inspector::ScriptCallFrame::~ScriptCallFrame
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: Web InspectorAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ews-watchlist, hi, inspector-bugzilla-changes, joepeck, keith_miller, mark.lam, mattbaker, msaboff, saam, simon.fraser, webkit-bug-importer, xpeng1984
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
[PATCH] Proposed Fix
hi: review+
[PATCH] For Landing none

Joseph Pecoraro
Reported 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
Attachments
[PATCH] Proposed Fix (1.65 KB, patch)
2018-12-21 14:47 PST, Joseph Pecoraro 		hi: review+
DetailsFormatted DiffDiff
[PATCH] For Landing (1.94 KB, patch)
2018-12-21 15:03 PST, Joseph Pecoraro 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Joseph Pecoraro
Comment 1 2017-12-04 13:12:53 PST
<rdar://problem/33894170>
Joseph Pecoraro
Comment 2 2018-12-21 14:39:12 PST
*** Bug 179998 has been marked as a duplicate of this bug. ***
Joseph Pecoraro
Comment 3 2018-12-21 14:39:23 PST
<rdar://problem/33726908>
Joseph Pecoraro
Comment 4 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.
Joseph Pecoraro
Comment 5 2018-12-21 14:47:19 PST
Created attachment 357982 [details] [PATCH] Proposed Fix
Devin Rousso
Comment 6 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 😛
Simon Fraser (smfr)
Comment 7 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; ?
Joseph Pecoraro
Comment 8 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.
Joseph Pecoraro
Comment 9 2018-12-21 15:03:54 PST
Created attachment 357987 [details] [PATCH] For Landing
Matt Baker
Comment 10 2018-12-21 15:25:03 PST
(In reply to Joseph Pecoraro from comment #9) > Created attachment 357987 [details] > [PATCH] For Landing Nice work!
Joseph Pecoraro
Comment 11 2018-12-21 15:49:48 PST
<https://trac.webkit.org/r239525>
WebKit Commit Bot
Comment 12 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.
Note You need to log in before you can comment on or make changes to this bug.