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

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[Jesse Schwartzentruber (:truber)]

Attachment: 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

Comment 1

[u459114]

Attachment: Bug 1343147 - Part 1. Do not append a nsDisplayPerspective if we already decide spipping a nsDisplayTransform.

Review commit: https://reviewboard.mozilla.org/r/177612/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/177612/

Comment 2

[u459114]

Attachment: Bug 1343147 - Part 2. A crash test of painting elements with both perspective and transform in generate text mask for background-clip:text.

Review commit: https://reviewboard.mozilla.org/r/177614/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/177614/

Comment 3

[u459114]

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/177614/diff/1-2/

Comment 4

[u459114]

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

Comment 5

[u459114]

Attachment: Bug 1343147 - Part 1. Do not double applying transform vector of the root frame in a glyph mask into the target context.

When we generate the glyph mask for a transformed frame in
GenerateAndPushTextMask, the transform vector had been applied into aContext[1],
so we should find a way to prevent applying the vector again when paiting the
glyph mask.

In bug 1299715, I tried to prevent double apply at [2], it casued two problems:

1. We only skip generating nsDisplayTransform, but we may still create a
nsDisplayPerspactive bellow. Since the parent of a nsDisplayPerspective must be
a nsDisplayTransform, which we have ignored, so we hit this assertion.

2. We skip all transfrom for all frames while paiting the glyph mask, which is
not correct. We should only skip double applying transform vector of the root
frame.

[1]
https://hg.mozilla.org/mozilla-central/file/59e5ec5729db/layout/painting/nsDisplayList.cpp#l752
[2]
https://hg.mozilla.org/mozilla-central/file/ce2c129f0a87/layout/generic/nsFrame.cpp#l2806

Review commit: https://reviewboard.mozilla.org/r/185940/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/185940/

Comment 6

[u459114]

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/177614/diff/2-3/

Comment 7

[u459114]

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/185940/diff/1-2/

Comment 8

[u459114]

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/177614/diff/3-4/

Comment 9

[u459114]

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/185940/diff/2-3/

Comment 10

[u459114]

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/177614/diff/4-5/

Comment 11

[u459114]

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/185940/diff/3-4/

Comment 12

[u459114]

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/177614/diff/5-6/

Comment 13

[Jonathan Kew [: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.

Comment 14

[Markus Stange [: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.

Comment 15

[Markus Stange [:mstange]]

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

Comment 16

[u459114]

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/185940/diff/4-5/

Comment 17

[u459114]

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/177614/diff/6-7/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
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

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/84451d723686
https://hg.mozilla.org/mozilla-central/rev/a4ae049b878e

Comment 20

[u459114]

[Tracking Requested - why for this release]:

Comment 21

[u459114]

Attachment: part2-on-beta.diff

Comment 22

[u459114]

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

Comment 23

[Marcia Knous [:marcia]]

Discussed with Ritu on irc. Not a new regression. As such we are going to won't fix this particular bug for Firefox 57.

Comment 25

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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+

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/237425440168
https://hg.mozilla.org/releases/mozilla-beta/rev/25fd3441407d

Comment 27

[Andrei Vaida [:avaida]]

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

Comment 28

[u459114]

Attachment: Part 1 for esr-52

Comment 29

[u459114]

Attachment: Part 2 for esr-52

Comment 30

[u459114]

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

Comment 31

[Liz Henry (:lizzard)]

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?

Comment 32

[Ryan VanderMeulen [:RyanVM]]

See comment 24.

Comment 33

[Liz Henry (:lizzard)]

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.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/971d6345bc3a
https://hg.mozilla.org/releases/mozilla-esr52/rev/60df7db06669