| Summary: | Web Inspector: Crashes seen under Inspector::ScriptCallFrame::~ScriptCallFrame | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Joseph Pecoraro <joepeck> | ||||||
| Component: | Web Inspector | Assignee: | Nobody <webkit-unassigned> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | commit-queue, drousso, ews-watchlist, inspector-bugzilla-changes, joepeck, keith_miller, mark.lam, mattbaker, msaboff, sbarati, simon.fraser, webkit-bug-importer, xpeng1984 | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Attachments: |
|
||||||||
|
Description
Joseph Pecoraro
2017-12-04 13:12:41 PST
*** Bug 179998 has been marked as a duplicate of this bug. *** 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.
Created attachment 357982 [details]
[PATCH] Proposed Fix
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 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; ? (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. Created attachment 357987 [details]
[PATCH] For Landing
(In reply to Joseph Pecoraro from comment #9) > Created attachment 357987 [details] > [PATCH] For Landing Nice work! 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. |