Bug 218577 – Debug assertion failure in RenderGeometryMap::mapToContainer

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Ryosuke Niwa]

e.g.

ASSERTION FAILED: m_accumulatedOffsetMightBeSaturated || roundedIntPoint(LayoutPoint(rendererMappedResult)) == result
./rendering/RenderGeometryMap.cpp(112) : WebCore::FloatPoint WebCore::RenderGeometryMap::mapToContainer(const WebCore::FloatPoint &, const WebCore::RenderLayerModelObject *) const
1   0x21e8072a9 WTFCrash
2   0x200005b8b WTFCrashWithInfo(int, char const*, char const*, int)
3   0x2045d7225 WebCore::RenderGeometryMap::mapToContainer(WebCore::FloatPoint const&, WebCore::RenderLayerModelObject const*) const
4   0x20460e6e1 WebCore::RenderGeometryMap::absolutePoint(WebCore::FloatPoint const&) const
5   0x20460cf7e WebCore::RenderLayer::updateLayerPositions(WebCore::RenderGeometryMap*, WTF::OptionSet<WebCore::RenderLayer::UpdateLayerPositionsFlag>)
6   0x20460d68f WebCore::RenderLayer::updateLayerPositions(WebCore::RenderGeometryMap*, WTF::OptionSet<WebCore::RenderLayer::UpdateLayerPositionsFlag>)
7   0x20460d68f WebCore::RenderLayer::updateLayerPositions(WebCore::RenderGeometryMap*, WTF::OptionSet<WebCore::RenderLayer::UpdateLayerPositionsFlag>)
8   0x20460d9d9 WebCore::RenderLayer::updateLayerPositionsAfterLayout(bool, bool)
9   0x203ceebee WebCore::FrameView::didLayout(WTF::WeakPtr<WebCore::RenderElement, WTF::EmptyCounter>)
10  0x203ce5425 WebCore::FrameViewLayoutContext::layout()
11  0x202f26b76 WebCore::Document::implicitClose()
12  0x203ae439b WebCore::FrameLoader::checkCallImplicitClose()
13  0x203ae3dfa WebCore::FrameLoader::checkCompleted()
14  0x203ae21a7 WebCore::FrameLoader::finishedParsing()
15  0x202f3ae36 WebCore::Document::finishedParsing()
16  0x2036639c8 WebCore::HTMLConstructionSite::finishedParsing()
17  0x2036a94e7 WebCore::HTMLTreeBuilder::finished()
18  0x20366ae68 WebCore::HTMLDocumentParser::end()
19  0x203668cd8 WebCore::HTMLDocumentParser::attemptToRunDeferredScriptsAndEnd()
20  0x203668a07 WebCore::HTMLDocumentParser::prepareToStopParsing()
21  0x20366aed2 WebCore::HTMLDocumentParser::attemptToEnd()
22  0x20366afa9 WebCore::HTMLDocumentParser::finish()
23  0x203a7db82 WebCore::DocumentWriter::end()
24  0x203a7cb84 WebCore::DocumentLoader::finishedLoading()
25  0x203a7c581 WebCore::DocumentLoader::notifyFinished(WebCore::CachedResource&, WebCore::NetworkLoadMetrics const&)
26  0x203bf054a WebCore::CachedResource::checkNotify(WebCore::NetworkLoadMetrics const&)
27  0x203bec0bc WebCore::CachedResource::finishLoading(WebCore::SharedBuffer*, WebCore::NetworkLoadMetrics const&)
28  0x203bed5ac WebCore::CachedRawResource::finishLoading(WebCore::SharedBuffer*, WebCore::NetworkLoadMetrics const&)
29  0x203b73c44 WebCore::SubresourceLoader::didFinishLoading(WebCore::NetworkLoadMetrics const&)
30  0x10454f0fa WebKit::WebResourceLoader::didFinishResourceLoad(WebCore::NetworkLoadMetrics const&)
31  0x104b08380 void IPC::callMemberFunctionImpl<WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&), std::__1::tuple<WebCore::NetworkLoadMetrics>, 0ul>(WebKit::WebResourceLoader*, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&), std::__1::tuple<WebCore::NetworkLoadMetrics>&&, std::__1::integer_sequence<unsigned long, 0ul>)
#CRASHED - com.apple.WebKit.WebContent.Development (pid 95170)

<rdar://problem/69574636>

[Ryosuke Niwa]

Created attachment 413189 [details]
Reduced test case

[Rob Buis]

I think we can just compare FloatPoints and do this (fixes the test case but not sure whether the buildbots run):
@@ -107,9 +107,9 @@ FloatPoint RenderGeometryMap::mapToContainer(const FloatPoint& p, const RenderLa
 #endif
     
     if (!hasFixedPositionStep() && !hasTransformStep() && !hasNonUniformStep() && (!container || (m_mapping.size() && container == m_mapping[0].m_renderer))) {
-        result = p + roundedIntSize(m_accumulatedOffset);
-        // Should convert to a LayoutPoint because of the uniqueness of LayoutUnit::round
-        ASSERT(m_accumulatedOffsetMightBeSaturated || roundedIntPoint(LayoutPoint(rendererMappedResult)) == result);
+        result = p;
+        result.move(m_accumulatedOffset);
+        ASSERT(m_accumulatedOffsetMightBeSaturated || areEssentiallyEqual(rendererMappedResult, result));
     } else {
         TransformState transformState(TransformState::ApplyTransformDirection, p);
         mapToContainer(transformState, container);

I also think this is not a security bug since the ASSERT is only hit in Debug and more of a sanity check.

[Rob Buis]

Created attachment 414966 [details]
Patch

[Ryosuke Niwa]

Is there any security implication here? If not, we can move it to non-security component.

[Alex Christensen]

Rob and I think this is not a security bug.

[zalan]

Could you explain in the changelog entry why changing behavior here is ok?

[Ryosuke Niwa]

Any update on this?

[Frédéric Wang (:fredw)]

(In reply to Rob Buis from comment #3)
> Created attachment 414966 [details]
> Patch

I'm able to reproduce the assertion with that patch applied using attachment 419280 [details].

[Frédéric Wang (:fredw)]

Created attachment 419957 [details]
Another test hitting the assert

Hi Rob. I'm attaching my other testcase here.

[Rob Buis]

(In reply to Frédéric Wang (:fredw) from comment #8)
> (In reply to Rob Buis from comment #3)
> > Created attachment 414966 [details]
> > Patch
> 
> I'm able to reproduce the assertion with that patch applied using attachment
> 419280 [details].

I looked at this and it seems to be a different problem, for sure not rounding.

In RenderGeometryMap::pushMappingsToAncestor canMapBetweenRenderersViaLayers will be called. This return true for Fred's testcase, but this code path does not take into account the translate property. The resulting accumulated offset will be compared against localToAbsolute (which does take translate into account) and is bound to fail for values of translate not 0px 0px.

Simon, should canMapBetweenRenderersViaLayers return false for translate?

[Simon Fraser (smfr)]

(In reply to Rob Buis from comment #10)
> (In reply to Frédéric Wang (:fredw) from comment #8)
> > (In reply to Rob Buis from comment #3)
> > > Created attachment 414966 [details]
> > > Patch
> > 
> > I'm able to reproduce the assertion with that patch applied using attachment
> > 419280 [details].
> 
> I looked at this and it seems to be a different problem, for sure not
> rounding.
> 
> In RenderGeometryMap::pushMappingsToAncestor canMapBetweenRenderersViaLayers
> will be called. This return true for Fred's testcase, but this code path
> does not take into account the translate property. The resulting accumulated
> offset will be compared against localToAbsolute (which does take translate
> into account) and is bound to fail for values of translate not 0px 0px.
> 
> Simon, should canMapBetweenRenderersViaLayers return false for translate?

Looks like that code was never updated for individual transform properties. It should test translate, rotate, scale properties.

[Rob Buis]

(In reply to Simon Fraser (smfr) from comment #11)
> Looks like that code was never updated for individual transform properties.
> It should test translate, rotate, scale properties.

Ah, I guess those are reasonably recent.
Do you know of a bug or can you create one? I would like to work on it, but it is a different problem than bug 218577.

[Rob Buis]

Created attachment 426574 [details]
Patch

[Rob Buis]

Created attachment 426685 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 426685 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=426685&action=review

> Source/WebCore/ChangeLog:8
> +        Do not use LayoutUnit rounding in RenderGeometryMap::mapToContainer.

Why not?

[Rob Buis]

Created attachment 427033 [details]
Patch

[Rob Buis]

Created attachment 427041 [details]
Patch

[EWS]

Committed r276629 (237057@main): <https://commits.webkit.org/237057@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 427041 [details].