1343139 - Assertion failure: !preTransformOverflows (GetVisualOverflowRect() won't return the pre-effects rect!

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect, P3)

Description

[Jesse Schwartzentruber (:truber)]

Attachment: testcase.html

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

Assertion failure: !preTransformOverflows (GetVisualOverflowRect() won't return the pre-effects rect!), at /home/worker/workspace/build/src/layout/svg/nsSVGIntegrationUtils.cpp:97
#01: PreEffectsVisualOverflowCollector::AddBox at layout/svg/nsSVGIntegrationUtils.cpp:71
#02: AddBoxesForFrame at layout/base/nsLayoutUtils.cpp:3885
#03: nsLayoutUtils::GetAllInFlowBoxes at layout/base/nsLayoutUtils.cpp:3897
#04: GetPreEffectsVisualOverflowUnion at layout/svg/nsSVGIntegrationUtils.cpp:75
#05: nsSVGIntegrationUtils::ComputePostEffectsVisualOverflowRect at layout/svg/nsSVGIntegrationUtils.cpp:264
#06: nsIFrame::FinishAndStoreOverflow at layout/generic/nsFrame.cpp:6571
#07: nsIFrame::UpdateOverflow at layout/generic/nsFrame.cpp:6736
#08: mozilla::OverflowChangedTracker::Flush at layout/base/OverflowChangedTracker.h:141
#09: mozilla::GeckoRestyleManager::EndProcessingRestyles at layout/base/GeckoRestyleManager.cpp:551
#10: mozilla::RestyleTracker::DoProcessRestyles at layout/base/RestyleTracker.cpp:128
#11: mozilla::GeckoRestyleManager::ProcessPendingRestyles at layout/base/GeckoRestyleManager.cpp:507
#12: mozilla::PresShell::DoFlushPendingNotifications at layout/base/PresShell.cpp:4137
#13: nsRefreshDriver::Tick at mfbt/RefPtr.h:283

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Hello CJKu, this is somewhat related to your work (bug 1340257), would you mind looking into this?
Of course this is also related to animation, so please feel free to ping me back if you are unsure about this.

Thanks!

Comment 2

[u459114]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)
> Hello CJKu, this is somewhat related to your work (bug 1340257), would you
> mind looking into this?
> Of course this is also related to animation, so please feel free to ping me
> back if you are unsure about this.
> 
> Thanks!
Sure, I will look into it.

Comment 3

[u459114]

Attachment: Bug 1343139 - Part 1. Implement nsStyleSVGReset::HasMask.

Implement this unil function to improve readability

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

Comment 4

[u459114]

Attachment: Bug 1343139 - Part 2. (Main) Add nsChangeHint_UpdateContainingBlock hint when HasMask state changed.

Look into the test case on bugzilla attachment:
o1.animate([{"mask": "linear-gradient(red,blue)", "transform": "none"}], 100);

If I replace mask by a filter effect, we won't hit that assertion. Then I look
back to nsStyleStruct, compared nsStyleSVGReset::CalcDifference with
nsStyleEffects::CalcDifference, and notice that we miss at this hint in
nsStyleSVGReset:
nsStyleEffects::CalcDifference(const nsStyleEffects& aNewData) const
{
  //....
  if (HasFilters() != aNewData.HasFilters()) {
    // A change from/to being a containing block for position:fixed.
    hint |= nsChangeHint_UpdateContainingBlock;
  }
}

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

Comment 5

[u459114]

Attachment: Bug 1343139 - Part 3. crash test.

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

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8844344 [details]
Bug 1343139 - Part 3. crash test.

https://reviewboard.mozilla.org/r/117830/#review119496

::: layout/base/crashtests/crashtests.list:492
(Diff revision 1)
>  load 1308793.svg
>  load 1308848-1.html
>  load 1308848-2.html
>  load 1338772-1.html
>  asserts(0-1) load 1343606.html # bug 1343948
> +load 1343139-1.html

Please add 'pref(dom.animations-api.core.enabled,true)' and 'skip-if(stylo)' just like this:

skip-if(stylo) pref(dom.animations-api.core.enabled,true) load 1343139-1.html # bug 1311257

Thank you for quick fix!

Comment 7

[u459114]

Comment on attachment 8844342 [details]
Bug 1343139 - Part 1. Implement nsStyleSVGReset::HasMask.

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

Comment 8

[u459114]

Comment on attachment 8844343 [details]
Bug 1343139 - Part 2. (Main) Add nsChangeHint_UpdateContainingBlock hint when HasMask state changed.

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

Comment 9

[u459114]

Comment on attachment 8844344 [details]
Bug 1343139 - Part 3. crash test.

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

Comment 10

[u459114]

Comment on attachment 8844344 [details]
Bug 1343139 - Part 3. crash test.

https://reviewboard.mozilla.org/r/117830/#review119496

> Please add 'pref(dom.animations-api.core.enabled,true)' and 'skip-if(stylo)' just like this:
> 
> skip-if(stylo) pref(dom.animations-api.core.enabled,true) load 1343139-1.html # bug 1311257
> 
> Thank you for quick fix!

Fixed.
You are wellcome.

Comment 11

[Cameron McCormack (:heycam)]

Comment on attachment 8844342 [details]
Bug 1343139 - Part 1. Implement nsStyleSVGReset::HasMask.

https://reviewboard.mozilla.org/r/117826/#review120314

::: commit-message-b7e42:3
(Diff revision 2)
> +Bug 1343139 - Part 1. Implement nsStyleSVGReset::HasMask.
> +
> +Implement this unil function to improve readability

s/unil/util/

Comment 12

[Cameron McCormack (:heycam)]

Comment on attachment 8844343 [details]
Bug 1343139 - Part 2. (Main) Add nsChangeHint_UpdateContainingBlock hint when HasMask state changed.

https://reviewboard.mozilla.org/r/117828/#review120322

::: layout/style/nsStyleStruct.cpp:1209
(Diff revision 2)
> +  if (HasMask() != aNewData.HasMask()) {
> +    hint |= nsChangeHint_UpdateContainingBlock;
> +  }

May as well duplicate the comment about position:fixed in nsStyleEffects::CalcDifference.

Comment 13

[Cameron McCormack (:heycam)]

Comment on attachment 8844344 [details]
Bug 1343139 - Part 3. crash test.

https://reviewboard.mozilla.org/r/117830/#review120324

::: layout/base/crashtests/1343139-1.html:6
(Diff revision 2)
> +<!DOCTYPE html>
> +<html>
> +<head>
> +<meta charset="UTF-8">
> +<script>
> +setTimeout(function(){ window.close(); }, 500);

I don't think we should window.close() from inside a test.  Also you would need a class=reftest-wait on the root element if you wanted a test that waited until animations are done.  (For animation tests like this, it is better to use a mochitest, and take control of the refresh driver, so we can skip to specific times without having to actually wait.)

But instead I think you should reduce the test a bit further, so that we don't need to run the animations, with something like:

o0.style.filter = "invert(96%)";
o1.style.mask = "linear-gradient(red,blue)";
document.body.offsetWidth;  // flush

and hopefully that is enough to cause the assertion without your part 2 patch.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8844343 [details]
Bug 1343139 - Part 2. (Main) Add nsChangeHint_UpdateContainingBlock hint when HasMask state changed.

https://reviewboard.mozilla.org/r/117828/#review120328

::: layout/style/nsStyleStruct.cpp:1210
(Diff revision 2)
>               mMaskType      != aNewData.mMaskType) {
>      hint |= nsChangeHint_RepaintFrame;
>    }
>  
> +  if (HasMask() != aNewData.HasMask()) {
> +    hint |= nsChangeHint_UpdateContainingBlock;

Yould you also please update the comment in https://searchfox.org/mozilla-central/rev/78ac0ceba97bd2deed847a8d0ae86ccf7a8887bf/layout/style/nsStyleContext.cpp#1254 to include nsStyleSVGReset.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/21f98c44d942
Part 1. Implement nsStyleSVGReset::HasMask. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/2eadd7ac6cc2
Part 2. (Main) Add nsChangeHint_UpdateContainingBlock hint when HasMask state changed. r=heycam

Comment 16

[u459114]

unlucky while make test case simpler. Lets check-in part 1 & 2 first, and leave-open. I will keep working on test case.

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/21f98c44d942
https://hg.mozilla.org/mozilla-central/rev/2eadd7ac6cc2

Comment 18

[Brian Birtles (:birtles, Feb 26~ parental leave)]

CJ, are you still planning to work on the test case for this?

Comment 19

[u459114]

(In reply to Brian Birtles (:birtles) from comment #18)
> CJ, are you still planning to work on the test case for this?
Not recently.

Comment 20

[Brian Birtles (:birtles, Feb 26~ parental leave)]

It's probably better, then, to just resolve without a test rather than leaving it open forever.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Should we consider backporting this to Beta54 or can it ride the 55 train?

Comment 22

[u459114]

Comment on attachment 8844342 [details]
Bug 1343139 - Part 1. Implement nsStyleSVGReset::HasMask.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1340257
[User impact if declined]: Hit assertion failure in debug build
[Is this code covered by automated tests?]: No
[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]: Part 1 and Part 2
[Is the change risky?]:no
[Why is the change risky/not risky?]: manual test pass
[String changes made/needed]: N/A

Please considerate to update part 1 and part 2 onto 54

Comment 23

[u459114]

Well... 54 is now beta since we kill aurora?

Comment 24

[u459114]

Comment on attachment 8844342 [details]
Bug 1343139 - Part 1. Implement nsStyleSVGReset::HasMask.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1340257
[User impact if declined]: Hit assertion failure in debug build
[Is this code covered by automated tests?]: No
[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]: Part 1 and Part 2
[Is the change risky?]:no
[Why is the change risky/not risky?]: manual test pass
[String changes made/needed]: N/A

Please considerate to uplift part 1 and part 2 onto 54

Comment 25

[Gerry Chang [:gchang]]

Comment on attachment 8844342 [details]
Bug 1343139 - Part 1. Implement nsStyleSVGReset::HasMask.

Fix an assertion failure and it's been in Nightly for a while. Beta54+. Should be in 54 beta 4.

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0dc3a0278058
https://hg.mozilla.org/releases/mozilla-beta/rev/33a78b0d2190

Comment 27

[u459114]

Attachment: follow up for beta

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ffbf38eccd5e

Comment 29

[Andrei Vaida [:avaida]]

(In reply to C.J. Ku[:cjku](UTC+8) from comment #24)
> [User impact if declined]: Hit assertion failure in debug build
> [Is this code covered by automated tests?]: No
> [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.

Comment 30

[Jesse Schwartzentruber (:truber)]

I'm still seeing this crash frequently and the testcase attached still repros in m-c b0ff0c5c0a35

Comment 31

[Jesse Schwartzentruber (:truber)]

Attachment: testcase2.html

Another testcase

Comment 32

[u459114]

Hit the same assertion but the root casue is different. This time hit how we update hint in nsStyleEffects::CalcDifference.(Last time was nsStyleSVGReset::CalcDifference)

I probably should change MOZ_ASSERT back to NS_ASSERTION since the condition of hitting this assertion is more complicate than I thought.

Comment 33

[u459114]

Attachment: follow up

Comment 34

[u459114]

Comment on attachment 8866624 [details] [diff] [review]
follow up

Review of attachment 8866624 [details] [diff] [review]:
-----------------------------------------------------------------

Let me look deeper into FinishAndStoreOverflow to see if I can compute overflow of continuation before hitting that assertion first.

Comment 35

[Cameron McCormack (:heycam)]

Comment on attachment 8866624 [details] [diff] [review]
follow up

Review of attachment 8866624 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/svg/nsSVGIntegrationUtils.cpp
@@ +93,5 @@
>      // not matter if there is no SVG effect on this frame, since no effect
>      // means post-effect rect matches pre-effect rect.
> +    //
> +    // This function may be called during reflow or painting. We should only
> +    // do this check in paiting process since the PreEffectsBBoxProperty of

painting

@@ +94,5 @@
>      // means post-effect rect matches pre-effect rect.
> +    //
> +    // This function may be called during reflow or painting. We should only
> +    // do this check in paiting process since the PreEffectsBBoxProperty of
> +    // continuations are not set correctly while reflowing.

Is there already a bug on file for the problem with old PreEffectsBBoxProperty values found on continuations during reflow?  If not, please file one.  Then reference that bug in the comment here.

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd48efef19f3
(followup) Check PreTransformOverflowAreasProperty in painting process only. r=heycam

Comment 37

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

https://hg.mozilla.org/mozilla-central/rev/fd48efef19f3