Bug 218577 - Debug assertion failure in RenderGeometryMap::mapToContainer
Summary: Debug assertion failure in RenderGeometryMap::mapToContainer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-11-04 11:24 PST by Ryosuke Niwa
Modified: 2021-04-27 02:56 PDT (History)
14 users (show)

See Also:


Attachments
Reduced test case (399 bytes, text/html)
2020-11-04 11:27 PST, Ryosuke Niwa
no flags Details
Patch (4.39 KB, patch)
2020-11-27 12:36 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Another test hitting the assert (54 bytes, text/html)
2021-02-11 00:40 PST, Frédéric Wang (:fredw)
no flags Details
Patch (4.09 KB, patch)
2021-04-20 11:33 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (4.40 KB, patch)
2021-04-21 07:51 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (5.00 KB, patch)
2021-04-26 03:14 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (4.53 KB, patch)
2021-04-26 07:30 PDT, Rob Buis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2020-11-04 11:24:56 PST
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>
Comment 1 Ryosuke Niwa 2020-11-04 11:27:41 PST
Created attachment 413189 [details]
Reduced test case
Comment 2 Rob Buis 2020-11-23 14:00:37 PST
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.
Comment 3 Rob Buis 2020-11-27 12:36:02 PST
Created attachment 414966 [details]
Patch
Comment 4 Ryosuke Niwa 2020-11-30 14:26:04 PST
Is there any security implication here? If not, we can move it to non-security component.
Comment 5 Alex Christensen 2020-11-30 14:26:35 PST
Rob and I think this is not a security bug.
Comment 6 zalan 2020-11-30 14:30:59 PST
Could you explain in the changelog entry why changing behavior here is ok?
Comment 7 Ryosuke Niwa 2020-12-18 20:01:35 PST
Any update on this?
Comment 8 Frédéric Wang (:fredw) 2021-02-04 09:43:49 PST
(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].
Comment 9 Frédéric Wang (:fredw) 2021-02-11 00:40:38 PST
Created attachment 419957 [details]
Another test hitting the assert

Hi Rob. I'm attaching my other testcase here.
Comment 10 Rob Buis 2021-04-20 07:04:46 PDT
(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?
Comment 11 Simon Fraser (smfr) 2021-04-20 09:26:22 PDT
(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.
Comment 12 Rob Buis 2021-04-20 09:32:04 PDT
(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.
Comment 13 Rob Buis 2021-04-20 11:33:20 PDT
Created attachment 426574 [details]
Patch
Comment 14 Rob Buis 2021-04-21 07:51:56 PDT
Created attachment 426685 [details]
Patch
Comment 15 Simon Fraser (smfr) 2021-04-21 15:56:09 PDT
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?
Comment 16 Rob Buis 2021-04-26 03:14:56 PDT
Created attachment 427033 [details]
Patch
Comment 17 Rob Buis 2021-04-26 07:30:24 PDT
Created attachment 427041 [details]
Patch
Comment 18 EWS 2021-04-26 23:19:09 PDT
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].