Bug 204648 - Crash in RenderLayerBacking::updateCompositedBounds from using cleared WeakPtr from m_backingSharingLayers
Summary: Crash in RenderLayerBacking::updateCompositedBounds from using cleared WeakPt...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Compositing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ali Juma
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-11-27 11:25 PST by Ali Juma
Modified: 2019-12-13 10:16 PST (History)
11 users (show)

See Also:


Attachments
Minimized test case (1.51 KB, text/html)
2019-12-12 13:42 PST, Ali Juma
no flags Details
Patch (6.38 KB, patch)
2019-12-12 13:50 PST, Ali Juma
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ali Juma 2019-11-27 11:25:22 PST
Steps to reproduce:
1. Visit google.com on MobileMiniBrowser or Chrome for iOS (this bug doesn't seem to reproduce on the version of google.com served to Safari)
2. Search for "word stacks"
3. Scroll down to the bottom, scroll back up, repeat.

Result:
WebProcess crash during step 3 (usually during the first scroll down or up).

The problem is that in RenderLayerBacking::updateCompositedBounds, in the "for (auto& layerWeakPtr : m_backingSharingLayers)" loop, |layerWeakPtr| is null but we use it without null-checking. In Debug builds, we crash in |ASSERT(layerWeakPtr->isDescendantOf(m_owningLayer));|, and in Release builds we crash inside the call to |layerWeakPtr->calculateLayerBounds|. 

Is a null check the right fix here, or is this a symptom of a deeper problem?

The crash stack on Debug builds is:
#0	0x0000000305e28a2e in ::WTFCrash() at /Users/ajuma/syncWebKit/Source/WTF/wtf/Assertions.cpp:305
#1	0x000000030aa22c1b in WTFCrashWithInfo(int, char const*, char const*, int) at /Users/ajuma/syncWebKit/WebKitBuild/Debug-iphonesimulator/usr/local/include/wtf/Assertions.h:622
#2	0x000000030e49149f in WebCore::RenderLayerBacking::updateCompositedBounds() at /Users/ajuma/syncWebKit/Source/WebCore/rendering/RenderLayerBacking.cpp:728
#3	0x000000030e469add in WebCore::RenderLayerBacking::updateAfterLayout(bool, bool) at /Users/ajuma/syncWebKit/Source/WebCore/rendering/RenderLayerBacking.cpp:797
#4	0x000000030e468439 in WebCore::RenderLayer::updateLayerPositions(WebCore::RenderGeometryMap*, WTF::OptionSet<WebCore::RenderLayer::UpdateLayerPositionsFlag>) at /Users/ajuma/syncWebKit/Source/WebCore/rendering/RenderLayer.cpp:1054
#5	0x000000030e46823b in WebCore::RenderLayer::updateLayerPositions(WebCore::RenderGeometryMap*, WTF::OptionSet<WebCore::RenderLayer::UpdateLayerPositionsFlag>) at /Users/ajuma/syncWebKit/Source/WebCore/rendering/RenderLayer.cpp:1034
#6	0x000000030e46823b in WebCore::RenderLayer::updateLayerPositions(WebCore::RenderGeometryMap*, WTF::OptionSet<WebCore::RenderLayer::UpdateLayerPositionsFlag>) at /Users/ajuma/syncWebKit/Source/WebCore/rendering/RenderLayer.cpp:1034
#7	0x000000030e46823b in WebCore::RenderLayer::updateLayerPositions(WebCore::RenderGeometryMap*, WTF::OptionSet<WebCore::RenderLayer::UpdateLayerPositionsFlag>) at /Users/ajuma/syncWebKit/Source/WebCore/rendering/RenderLayer.cpp:1034
#8	0x000000030e46856f in WebCore::RenderLayer::updateLayerPositionsAfterLayout(bool, bool) at /Users/ajuma/syncWebKit/Source/WebCore/rendering/RenderLayer.cpp:937
#9	0x000000030db7c78b in WebCore::FrameView::didLayout(WTF::WeakPtr<WebCore::RenderElement>) at /Users/ajuma/syncWebKit/Source/WebCore/page/FrameView.cpp:1252
#10	0x000000030dbbdd2b in WebCore::FrameViewLayoutContext::layout() at /Users/ajuma/syncWebKit/Source/WebCore/page/FrameViewLayoutContext.cpp:277
#11	0x000000030ce558f6 in WebCore::Document::updateLayout() at /Users/ajuma/syncWebKit/Source/WebCore/dom/Document.cpp:2099
#12	0x000000030ce56dfe in WebCore::Document::updateLayoutIgnorePendingStylesheets(WebCore::Document::RunPostLayoutTasks) at /Users/ajuma/syncWebKit/Source/WebCore/dom/Document.cpp:2113
#13	0x000000030cf13709 in WebCore::Element::scrollTop() at /Users/ajuma/syncWebKit/Source/WebCore/dom/Element.cpp:1273
#14	0x000000030b31d731 in WebCore::jsElementScrollTopGetter(JSC::JSGlobalObject&, WebCore::JSElement&, JSC::ThrowScope&) at /Users/ajuma/syncWebKit/WebKitBuild/Debug-iphonesimulator/DerivedSources/WebCore/JSElement.cpp:1325
#15	0x000000030b29c9b0 in long long WebCore::IDLAttribute<WebCore::JSElement>::get<&(WebCore::jsElementScrollTopGetter(JSC::JSGlobalObject&, WebCore::JSElement&, JSC::ThrowScope&)), (WebCore::CastedThisErrorBehavior)3>(JSC::JSGlobalObject&, long long, char const*) at /Users/ajuma/syncWebKit/Source/WebCore/bindings/js/JSDOMAttribute.h:69
#16	0x000000030b29c898 in WebCore::jsElementScrollTop(JSC::JSGlobalObject*, long long, JSC::PropertyName) at /Users/ajuma/syncWebKit/WebKitBuild/Debug-iphonesimulator/DerivedSources/WebCore/JSElement.cpp:1331
#17	0x00000003071b309f in JSC::PropertySlot::customGetter(JSC::JSGlobalObject*, JSC::PropertyName) const at /Users/ajuma/syncWebKit/Source/JavaScriptCore/runtime/PropertySlot.cpp:50
#18	0x00000003070dff31 in JSC::PropertySlot::getValue(JSC::JSGlobalObject*, JSC::PropertyName) const at /Users/ajuma/syncWebKit/Source/JavaScriptCore/runtime/PropertySlot.h:414
#19	0x0000000306ed6fe5 in JSC::JSValue::get(JSC::JSGlobalObject*, JSC::PropertyName, JSC::PropertySlot&) const at /Users/ajuma/syncWebKit/Source/JavaScriptCore/runtime/JSCJSValueInlines.h:870
#20	0x0000000306d12053 in ::llint_slow_path_get_by_id(JSC::CallFrame *, const JSC::Instruction *) at /Users/ajuma/syncWebKit/Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:770
Comment 1 Ali Juma 2019-11-28 08:45:36 PST
Turns out this reproduces on all non-Safari browsers on iOS that I've tested (Chrome, Firefox, Edge, MobileMiniBrowser).

It also reproduces on multiple search queries, such as "things that run", "things you can lose", "a form of communication" (all of which are apparently related to Word Stacks).
Comment 2 Simon Fraser (smfr) 2019-12-02 11:45:42 PST
> Is a null check the right fix here, or is this a symptom of a deeper problem?

It's a symptom of a deeper problem.
Comment 3 Radar WebKit Bug Importer 2019-12-02 11:45:58 PST
<rdar://problem/57565420>
Comment 4 Ali Juma 2019-12-04 13:02:56 PST
(In reply to Simon Fraser (smfr) from comment #2)
> > Is a null check the right fix here, or is this a symptom of a deeper problem?
> 
> It's a symptom of a deeper problem.

Debugging this, a RenderLayer's m_backingProviderLayer is getting cleared in RenderLayerCompositor::BackingSharingState::updateBeforeDescendantTraversal without ever getting removed from the corresponding RenderLayerBacking's m_backingSharingLayers.

At the point where this happens in RenderLayerCompositor::BackingSharingState::updateBeforeDescendantTraversal, the RenderLayer's m_backingProviderLayer is different from the current m_backingProviderCandidate. For some reason (not clear yet) there's no other point where that m_backingProviderLayer has its backingSharingLayers cleared.
Comment 5 Simon Fraser (smfr) 2019-12-04 13:27:18 PST
A reduced test case would really help here.
Comment 6 Ali Juma 2019-12-12 13:42:08 PST
Created attachment 385544 [details]
Minimized test case

Here's a minimized test case.

This bug happens when a RenderLayer X moves from one backingProviderLayer Y to another backingProviderLayer Z that's a descendant of the old backingProviderLayer. When this happens, the old backingProviderLayer (Y) clears out the layer's backingProviderLayer pointer in the call to clearBackingSharingLayerProviders that happens during RenderLayerCompositor::BackingSharingState::updateAfterDescendantTraversal.

So then RenderLayer X appears in Z's m_backingSharingLayers, but X's backingProviderLayer is null. This means that when X is destroyed, it doesn't know that it needs to tell Z about this. The next time we try to use the weak pointer to X in Z's m_backingSharingLayers, we crash.

The fix is to make clearBackingSharingLayerProviders only clear a layer's backingProviderLayer if that backingProviderLayer is the current backing's owner.
Comment 7 Ali Juma 2019-12-12 13:43:32 PST
The minimized test case crashes on Mac too.
Comment 8 Ali Juma 2019-12-12 13:50:35 PST
Created attachment 385546 [details]
Patch
Comment 9 WebKit Commit Bot 2019-12-13 06:33:02 PST
Comment on attachment 385546 [details]
Patch

Clearing flags on attachment: 385546

Committed r253469: <https://trac.webkit.org/changeset/253469>
Comment 10 WebKit Commit Bot 2019-12-13 06:33:04 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Simon Fraser (smfr) 2019-12-13 10:16:10 PST
Thank you for the fix, BTW!