Closed Bug 1343139 Opened 3 years ago Closed 3 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr52 --- unaffected
firefox53 --- wontfix
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: truber, Assigned: u459114)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(7 files)

Attached file 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
Flags: in-testsuite?
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!
Flags: needinfo?(cku)
(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.
Assignee: nobody → cku
Flags: needinfo?(cku)
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 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.
Attachment #8844342 - Flags: review?(cam)
Attachment #8844343 - Flags: review?(cam)
Attachment #8844344 - Flags: review?(cam)
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/
Attachment #8844342 - Flags: review?(cam) → review+
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.
Attachment #8844343 - Flags: review?(cam) → review+
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.
Attachment #8844344 - Flags: review?(cam) → review-
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.
Pushed by cku@mozilla.com:
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
unlucky while make test case simpler. Lets check-in part 1 & 2 first, and leave-open. I will keep working on test case.
Whiteboard: leave-open
CJ, are you still planning to work on the test case for this?
Flags: needinfo?(cku)
Priority: -- → P3
(In reply to Brian Birtles (:birtles) from comment #18)
> CJ, are you still planning to work on the test case for this?
Not recently.
Flags: needinfo?(cku)
It's probably better, then, to just resolve without a test rather than leaving it open forever.
Status: NEW → RESOLVED
Closed: 3 years ago
Flags: in-testsuite? → in-testsuite-
Resolution: --- → FIXED
Whiteboard: leave-open
Target Milestone: --- → mozilla55
Should we consider backporting this to Beta54 or can it ride the 55 train?
Flags: needinfo?(cku)
Flags: needinfo?(cku)
Well... 54 is now beta since we kill aurora?
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
Attachment #8844342 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
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.
Attachment #8844342 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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.
Flags: qe-verify-
I'm still seeing this crash frequently and the testcase attached still repros in m-c b0ff0c5c0a35
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached file testcase2.html
Another testcase
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.
Attached patch follow upSplinter Review
Attachment #8866624 - Flags: review?(cam)
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.
Attachment #8866624 - Flags: review?(cam)
Attachment #8866624 - Flags: review?(cam)
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.
Attachment #8866624 - Flags: review?(cam) → review+
Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd48efef19f3
(followup) Check PreTransformOverflowAreasProperty in painting process only. r=heycam
https://hg.mozilla.org/mozilla-central/rev/fd48efef19f3
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.