| Summary: | Debug assertion failure in RenderGeometryMap::mapToContainer | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||||||||||
| Component: | Layout and Rendering | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||
| Severity: | Normal | CC: | achristensen, bfulgham, cgarcia, ews-feeder, fred.wang, gpoo, graouts, product-security, rbuis, rwlbuis, simon.fraser, svillar, webkit-bug-importer, zalan | ||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Ryosuke Niwa
2020-11-04 11:24:56 PST
Created attachment 413189 [details]
Reduced test case
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.
Created attachment 414966 [details]
Patch
Is there any security implication here? If not, we can move it to non-security component. Rob and I think this is not a security bug. Could you explain in the changelog entry why changing behavior here is ok? Any update on this? (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]. Created attachment 419957 [details]
Another test hitting the assert
Hi Rob. I'm attaching my other testcase here.
(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? (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. (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. Created attachment 426574 [details]
Patch
Created attachment 426685 [details]
Patch
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? Created attachment 427033 [details]
Patch
Created attachment 427041 [details]
Patch
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]. |