Bug 204648 – Crash in RenderLayerBacking::updateCompositedBounds from using cleared WeakPtr from m_backingSharingLayers

• Component: Compositing
• Status: RESOLVED FIXED

[Ali Juma]

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

[Ali Juma]

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

[Simon Fraser (smfr)]

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

[Radar WebKit Bug Importer]

<rdar://problem/57565420>

[Ali Juma]

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

[Simon Fraser (smfr)]

A reduced test case would really help here.

[Ali Juma]

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.

[Ali Juma]

The minimized test case crashes on Mac too.

[Ali Juma]

Created attachment 385546 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 385546 [details]
Patch

Clearing flags on attachment: 385546

Committed r253469: <https://trac.webkit.org/changeset/253469>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

Thank you for the fix, BTW!