Assertion failure: mList.GetChildren()->GetTop()->GetType() == TYPE_TRANSFORM

RESOLVED FIXED in Firefox -esr52

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: truber, Assigned: u459114)

Tracking

(Blocks 1 bug, {assertion, testcase})

Trunk
mozilla58
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox-esr52 fixed, firefox56 wontfix, firefox57- fixed, firefox58 fixed)

Details

Attachments

(6 attachments, 1 obsolete attachment)

Posted image testcase.svg
The attached testcase causes an assertion failure in mozilla-central rev 5d1ebf7782df.

Assertion failure: mList.GetChildren()->GetTop()->GetType() == TYPE_TRANSFORM, at /home/worker/workspace/build/src/layout/painting/nsDisplayList.cpp:7411
#01: nsIFrame::BuildDisplayListForStackingContext at layout/generic/nsFrame.cpp:2639
#02: nsIFrame::BuildDisplayListForChild at layout/painting/nsDisplayList.h:1083
#03: nsContainerFrame::BuildDisplayListForNonBlockChildren at layout/generic/nsContainerFrame.cpp:341
#04: nsIFrame::BuildDisplayListForStackingContext at layout/generic/nsFrame.cpp:2419
#05: nsIFrame::BuildDisplayListForChild at layout/painting/nsDisplayList.h:1083
#06: nsContainerFrame::BuildDisplayListForNonBlockChildren at layout/generic/nsContainerFrame.cpp:341
#07: nsIFrame::BuildDisplayListForChild at layout/generic/nsFrame.cpp:2992
#08: nsContainerFrame::BuildDisplayListForNonBlockChildren at layout/generic/nsContainerFrame.cpp:341
#09: nsSVGOuterSVGFrame::BuildDisplayList at layout/painting/DisplayListClipState.h:155
#10: nsIFrame::BuildDisplayListForChild at layout/generic/nsFrame.cpp:2992
#11: nsCanvasFrame::BuildDisplayList at layout/generic/nsCanvasFrame.cpp:558
#12: nsIFrame::BuildDisplayListForStackingContext at layout/generic/nsFrame.cpp:2419
#13: nsLayoutUtils::PaintFrame at layout/painting/nsDisplayList.h:942
#14: GenerateAndPushTextMask at gfx/src/nsRegion.h:75
#15: nsDisplayBackgroundImage::PaintInternal at layout/painting/nsDisplayList.cpp:3533
#16: nsDisplayCanvasBackgroundImage::Paint at layout/generic/nsCanvasFrame.cpp:326
#17: mozilla::FrameLayerBuilder::PaintItems at layout/painting/FrameLayerBuilder.cpp:6031
#18: mozilla::FrameLayerBuilder::DrawPaintedLayer at layout/painting/FrameLayerBuilder.cpp:6210
#19: mozilla::layers::ClientPaintedLayer::PaintThebes at mfbt/RefPtr.h:62
#20: mozilla::layers::ClientPaintedLayer::RenderLayerWithReadback at gfx/src/nsRegion.h:75
#21: mozilla::layers::ClientContainerLayer::RenderLayer at gfx/layers/client/ClientContainerLayer.h:57
#22: mozilla::layers::ClientContainerLayer::RenderLayer at gfx/layers/client/ClientContainerLayer.h:57
#23: mozilla::layers::ClientLayerManager::EndTransactionInternal at gfx/layers/client/ClientLayerManager.cpp:358
#24: mozilla::layers::ClientLayerManager::EndTransaction at gfx/layers/client/ClientLayerManager.cpp:412
#25: nsDisplayList::PaintRoot at layout/painting/nsDisplayList.cpp:2251
#26: nsLayoutUtils::PaintFrame at mfbt/RefPtr.h:129
Flags: in-testsuite?
Component: Layout → Layout: Web Painting
Assignee: nobody → cku
Attachment #8905804 - Flags: review?(mstange)
Attachment #8905805 - Flags: review?(mstange)
Attachment #8905804 - Flags: review?(mstange)
Attachment #8905805 - Flags: review?(mstange)
1. Revert the transform of aFrame in GenerateAndPushTextMask.
2. Remove the if statement at [1]

Except hitting this assertion, the original solution will remove any transfrom inside the element with bg_clip:text
 
[1] https://hg.mozilla.org/mozilla-central/file/d5110841090c/layout/generic/nsFrame.cpp#l2794
Attachment #8905804 - Attachment is obsolete: true
Attachment #8914597 - Flags: review?(jfkthame)
Attachment #8905805 - Flags: review?(jfkthame)
Comment on attachment 8914597 [details]
Bug 1343147 - Part 1. Do not double applying transform vector of the root frame in a glyph mask into the target context.

https://reviewboard.mozilla.org/r/185940/#review190934

I think mstange would be a better reviewer for display-list stuff, unless he's unavailable to take this.
Attachment #8914597 - Flags: review?(jfkthame) → review?(mstange)
Attachment #8905805 - Flags: review?(jfkthame) → review?(mstange)
Comment on attachment 8914597 [details]
Bug 1343147 - Part 1. Do not double applying transform vector of the root frame in a glyph mask into the target context.

https://reviewboard.mozilla.org/r/185940/#review191148

::: commit-message-44643:20
(Diff revision 4)
> +
> +This patch fixes both of these issues:
> +a. We will still create a nsDisplayTransform for the root frame if need. But
> +the transform matrix we apply into the target context will be an identity
> +matrix, so we fix #1 above.
> +b. In #a, we change the transform matrix to an identify matrix only for the root

"identity"

::: layout/painting/nsDisplayList.cpp:8123
(Diff revision 4)
> -  const Matrix4x4& newTransformMatrix = GetTransformForRendering();
> +  const Matrix4x4& newTransformMatrix =
> +    shouldSkipTransform ? Matrix4x4(): GetTransformForRendering();

I don't completely understand in what cases local const references prolong the lifespan of a temporary, so I would be more comfortable if this did a copy.
Attachment #8914597 - Flags: review?(mstange) → review+
Comment on attachment 8905805 [details]
Bug 1343147 - Part 2. A crash test of painting elements with both perspective and transform in generate text mask for background-clip:text.

https://reviewboard.mozilla.org/r/177614/#review191150
Attachment #8905805 - Flags: review?(mstange) → review+
Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/84451d723686
Part 1. Do not double applying transform vector of the root frame in a glyph mask into the target context. r=mstange
https://hg.mozilla.org/integration/autoland/rev/a4ae049b878e
Part 2. A crash test of painting elements with both perspective and transform in generate text mask for background-clip:text. r=mstange
https://hg.mozilla.org/mozilla-central/rev/84451d723686
https://hg.mozilla.org/mozilla-central/rev/a4ae049b878e
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Depends on: 1299715
Version: Trunk → 57 Branch
Version: 57 Branch → Trunk
[Tracking Requested - why for this release]:
Comment on attachment 8914597 [details]
Bug 1343147 - Part 1. Do not double applying transform vector of the root frame in a glyph mask into the target context.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1299715 
[User impact if declined]: The mask shape of an element with "backgound-clip:text" can be wrong if there are also transform/perspective props applied onto that element.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]:  no
[List of other uplifts needed for the feature/fix]: this patch plus attachment 8916055 [details] [diff] [review]
[Is the change risky?]: no
[Why is the change risky/not risky?]: simple change cover with an auto test.
[String changes made/needed]: NA
Attachment #8914597 - Flags: approval-mozilla-beta?
Discussed with Ritu on irc. Not a new regression. As such we are going to won't fix this particular bug for Firefox 57.
Flags: needinfo?(rkothari)
Comment on attachment 8914597 [details]
Bug 1343147 - Part 1. Do not double applying transform vector of the root frame in a glyph mask into the target context.

My mistake, this one has a severe impact, taking it, Beta57+
Flags: needinfo?(rkothari)
Attachment #8914597 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to C.J. Ku[:cjku](UTC+8) from comment #22)
> [Is this code covered by automated tests?]: yes
> [Has the fix been verified in Nightly?]: yes
> [Needs manual test from QE? If yes, steps to reproduce]:  no

Setting qe-verify- based on C.J. Ku's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
Comment on attachment 8917645 [details] [diff] [review]
Part 1 for esr-52

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: 
User impact if declined: The mask shape of an element with "backgound-clip:text" can be wrong if there are also transform/perspective props applied onto that element.
Fix Landed on Version: 58 and 57
Risk to taking this patch (and alternatives if risky): low. covered by a test case and verified on 58
String or UUID changes made by this patch: NA

Please land this patch and "Part 2 for esr-52" which contains a test case
Attachment #8917645 - Flags: approval-mozilla-esr52?
Comment on attachment 8917645 [details] [diff] [review]
Part 1 for esr-52

Normally we try to keep uplifts to ESR to critical security and stability issues. Is this something that we think is affecting many ESR users? Is it a new regression in a recent ESR version?
Flags: needinfo?(cku)
Attachment #8917645 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52-
See comment 24.
Flags: needinfo?(cku) → needinfo?(lhenry)
Comment on attachment 8917645 [details] [diff] [review]
Part 1 for esr-52

OK, reconsidered for ESR. Seems like this has fixed the issue on 57 beta. Thanks Ryan.
Attachment #8917645 - Flags: approval-mozilla-esr52- → approval-mozilla-esr52+
Flags: needinfo?(lhenry)
Attachment #8917646 - Flags: approval-mozilla-esr52+
You need to log in before you can comment on or make changes to this bug.