1373750 - Assertion failure: aClipState->IsValid()

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[Jesse Schwartzentruber (:truber)]

Attachment: testcase.html

The attached testcase fails the below assertion in mozilla-central rev fe809f57bf22.

Assertion failure: aClipState->IsValid(), at /home/worker/workspace/build/src/layout/painting/nsCSSRendering.cpp:2195
#01: nsCSSRendering::PaintStyleImageLayerWithSC at layout/painting/nsCSSRendering.cpp:2682
#02: PaintMaskSurface at layout/svg/nsSVGIntegrationUtils.cpp:478
#03: nsSVGIntegrationUtils::PaintMaskAndClipPath at layout/svg/nsSVGIntegrationUtils.cpp:557
#04: nsDisplayMask::PaintAsLayer at layout/painting/nsDisplayList.cpp:8553
#05: mozilla::FrameLayerBuilder::PaintItems at layout/painting/FrameLayerBuilder.cpp:3704
#06: mozilla::FrameLayerBuilder::DrawPaintedLayer at layout/painting/FrameLayerBuilder.cpp:6247
#07: mozilla::layers::ClientPaintedLayer::PaintThebes at mfbt/RefPtr.h:40
#08: mozilla::layers::ClientPaintedLayer::RenderLayerWithReadback at gfx/src/nsRegion.h:75
#09: mozilla::layers::ClientContainerLayer::RenderLayer at gfx/layers/client/ClientContainerLayer.h:57
#10: mozilla::layers::ClientContainerLayer::RenderLayer at gfx/layers/client/ClientContainerLayer.h:57
#11: mozilla::layers::ClientLayerManager::EndTransactionInternal at gfx/layers/client/ClientLayerManager.cpp:375
#12: mozilla::layers::ClientLayerManager::EndTransaction at gfx/layers/client/ClientLayerManager.cpp:434
#13: nsDisplayList::PaintRoot at layout/painting/nsDisplayList.cpp:2293
#14: nsLayoutUtils::PaintFrame at mfbt/RefPtr.h:129
#15: mozilla::PresShell::Paint at layout/base/PresShell.cpp:6444

Comment 1

[Ryan VanderMeulen [:RyanVM]]

INFO: Last good revision: c992c7e903ce1409aa3ad34a97ee2920ca0e45a9
INFO: First bad revision: abcf45c9ad660b35e892cd3736c28d11528bdc64
INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=c992c7e903ce1409aa3ad34a97ee2920ca0e45a9&tochange=abcf45c9ad660b35e892cd3736c28d11528bdc64

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

This is unrelated to bug 1188074. That bug just happens to make the given syntax valid. This assertion can be triggered with a slightly modified version of the testcase which uses valid syntax for earlier version.

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

Attachment: modified testcase for earlier version

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

With the modified testcase, the updated regression range is: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=9113f64dea5785aa0f75c4ddab2d2b00c4cf28b0&tochange=3ea48d34848247c964df18c4d9582f4dff6a71dd

Comment 5

[u459114]

I will check it next week

Comment 6

[u459114]

Attachment: Bug 1373750 - Part 1. Except the bottom layer, recompute clip state of each mask layer by a clear clipState object.

In nsCSSRendering::PaintStyleImageLayerWithSC, we reuse the same clip-state
object,  clipState, for all bg/mask layers. We hit this assertion if the
border-rarius of prevoius one is not 0, but the the border-radius of the next one
is 0. In this condition, clipState.mHasRoundedCorners is false, but
clipState.mClippedRadii still stores border-radius information of previous layer.

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

Comment 7

[u459114]

Attachment: Bug 1373750 - Part 2. Add a crash test consists of two mask layers.

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

Comment 8

[Jan Keromnes [:janx]]

Comment on attachment 8911690 [details]
Bug 1373750 - Part 1. Except the bottom layer, recompute clip state of each mask layer by a clear clipState object.

https://reviewboard.mozilla.org/r/183082/#review188320

::: layout/painting/nsCSSRendering.cpp:2156
(Diff revision 1)
> +{
> +  mBGClipArea.SetEmpty();
> +  mAdditionalBGClipArea.SetEmpty();
> +  mDirtyRectInAppUnits.SetEmpty();
> +  mDirtyRectInDevPx.SetEmpty();
> +  for (int i = 0; i < 8; i++) {

Quick note: Our static analysis bot found a potential improvement for this line:

Warning: Use range-based for loop instead [clang-tidy: modernize-loop-convert]

::: layout/painting/nsCSSRendering.cpp:2160
(Diff revision 1)
> +  mDirtyRectInDevPx.SetEmpty();
> +  for (int i = 0; i < 8; i++) {
> +    mRadii[i] = 0;
> +  }
> +
> +  for (int i = 0; i < eCornerCount; i++) {

Same quick note as above:

Warning: Use range-based for loop instead [clang-tidy: modernize-loop-convert]

Comment 9

[u459114]

Comment on attachment 8911690 [details]
Bug 1373750 - Part 1. Except the bottom layer, recompute clip state of each mask layer by a clear clipState object.

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

Comment 10

[u459114]

Comment on attachment 8911691 [details]
Bug 1373750 - Part 2. Add a crash test consists of two mask layers.

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

Comment 11

[Markus Stange [:mstange]]

Comment on attachment 8911690 [details]
Bug 1373750 - Part 1. Except the bottom layer, recompute clip state of each mask layer by a clear clipState object.

https://reviewboard.mozilla.org/r/183082/#review189300

I'd prefer this be fixed in the caller. You could do something like this:

ImageLayerClipState currentLayerClipState;
if (isBottomLayer) {
  currentLayerClipState = clipState;
} else {
  GetImageLayerClip(..., &currentLayerClipState);
}
// use currentLayerClipState here

If you'd prefer to keep the current approach, would "void Clear() { *this = ImageLayerClipState(); }"  work?

::: commit-message-7e962:5
(Diff revision 2)
> +Bug 1373750 - Part 1. Implement ImageLayerClipState::Clear
> +
> +In nsCSSRendering::PaintStyleImageLayerWithSC, we reuse the same clip-state
> +object,  clipState, for all bg/mask layers. We hit this assertion if the
> +border-rarius of prevoius one is not 0, but the the border-radius of the next one

typos: "radius" and "previous"

Comment 12

[Markus Stange [:mstange]]

Comment on attachment 8911691 [details]
Bug 1373750 - Part 2. Add a crash test consists of two mask layers.

https://reviewboard.mozilla.org/r/183084/#review189302

Comment 13

[u459114]

Comment on attachment 8911690 [details]
Bug 1373750 - Part 1. Except the bottom layer, recompute clip state of each mask layer by a clear clipState object.

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

Comment 14

[u459114]

Comment on attachment 8911691 [details]
Bug 1373750 - Part 2. Add a crash test consists of two mask layers.

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

Comment 15

[u459114]

Comment on attachment 8911690 [details]
Bug 1373750 - Part 1. Except the bottom layer, recompute clip state of each mask layer by a clear clipState object.

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

Comment 16

[u459114]

Comment on attachment 8911691 [details]
Bug 1373750 - Part 2. Add a crash test consists of two mask layers.

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

Comment 17

[Markus Stange [:mstange]]

Comment on attachment 8911690 [details]
Bug 1373750 - Part 1. Except the bottom layer, recompute clip state of each mask layer by a clear clipState object.

https://reviewboard.mozilla.org/r/183082/#review189848

Thanks!

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/029918001381
Part 1. Except the bottom layer, recompute clip state of each mask layer by a clear clipState object. r=mstange
https://hg.mozilla.org/integration/autoland/rev/772f7c02ef19
Part 2. Add a crash test consists of two mask layers. r=mstange

Comment 19

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

https://hg.mozilla.org/mozilla-central/rev/029918001381
https://hg.mozilla.org/mozilla-central/rev/772f7c02ef19

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Is there a user impact here that warrants Beta backport consideration or can this ride the 58 train?

Comment 21

[u459114]

It's an assertion failure, and the reason to cause this failure is because the assertion itself is not correct. There is no user impact at release version, so I think we can just ride 58 train.