Closed Bug 1457249 Opened 2 years ago Closed 2 years ago

Assertion failure: transform->mTransform.mTransformInDevSpace.FuzzyEqualsMultiplicative( (layer->AsHostLayer()->GetShadowBaseTransform())), at gfx/layers/composite/AsyncCompositionManager.cpp:696

Categories

(Core :: DOM: Animation, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox59 --- unaffected
firefox60 --- unaffected
firefox61 --- fixed

People

(Reporter: aosmond, Assigned: hiro)

References

Details

(Keywords: assertion, regression)

Attachments

(4 files)

Assertion failure: transform->mTransform.mTransformInDevSpace.FuzzyEqualsMultiplicative( (layer->AsHostLayer()->GetShadowBaseTransform())), at /builds/worker/workspace/build/src/gfx/layers/composite/AsyncCompositionManager.cpp:696

STR:

Using a linux64 debug build, open http://giphy.com/. Wait for images to load/animate. Scroll down. Should not assert.

I reproduced off recent mozilla-central debug builds (local and off treeherder, ccfd7b716a91). Looks like the assert in question was added by bug 1454324.
Blocks: 1454324
Flags: needinfo?(hikezoe)
I could reproduce the assertion locally and am debugging it now, but it's weird.  The transform value in question that was used to SetShadowBaseTransform()[1] in ApplyAnimatedValue actually equals to the transform value in the animation storage.  I did also confirmed GetShadowTransformSetByAnimation value is true.  So, I suspect there is another place that we call SetShadowBaseTransform() without calling SetShadowTransformSetByAnimation() and I guess we should call SetShadowTransformSetByAnimation(false) in the place.  But if there is really such place, it means we can't use the shadow transform value in the assertion.  sigh.

[1] https://hg.mozilla.org/mozilla-central/file/63a0e2f626fe/gfx/layers/composite/AsyncCompositionManager.cpp#l632
Oh yeah, maybe it's ApplyAsyncTransformToScrollbarForContent.  ApplyAsyncTransformToScrollbarForContent calls SetShadowTransform and SetShadowTransform calls SetShadowBaseTransform without calling SetShadowTransformSetByAnimation().  And it seems to me that it's correct.  So we have to figure out another way for the assertion,  I might give up adding the assertion there.

https://hg.mozilla.org/mozilla-central/file/63a0e2f626fe/gfx/layers/composite/AsyncCompositionManager.cpp#l1134
Oh well, ApplyAsyncTransformToScrollbarForContent is only for scroll bar.  Other call sites of SetShadowTransform change the transform value anyway.
It seems to be hard to reverse-calculate the transform changes accumulated by APZ.  So I've decided to not skip the calculation on debug build and use the assertion.  This is the most reliable way to check the animation value is unchanged.  Note that even if we don't skip the calculation, we don't update animation storage with the calculated values, so the optimization should work as it is.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c7348def93077d28b00791d49afc2b541f08296
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Flags: needinfo?(hikezoe)
There was a garbage line that I should have removed; :/  (the value was used for the tolerance for FuzzyEquals?)
https://hg.mozilla.org/try/rev/2d2f4741d3fc0ef6d138c3e49769ae4cf1fd7904#l2.13

A new try;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3caac9b2238b621688bbe7f39ab4d05b2fc396f
Comment on attachment 8971469 [details]
Bug 1457249 - Assert that there is an animation value set by animations when we skip calculation for newly animation value.

https://reviewboard.mozilla.org/r/240220/#review246102
Attachment #8971469 - Flags: review?(bugmail) → review+
Comment on attachment 8971470 [details]
Bug 1457249 - Factor out the function that converts servo's animation value to matrix.

https://reviewboard.mozilla.org/r/240222/#review246104
Attachment #8971470 - Flags: review?(bugmail) → review+
Comment on attachment 8971471 [details]
Bug 1457249 - Factor out the function that converts transform into device space.

https://reviewboard.mozilla.org/r/240224/#review246106
Attachment #8971471 - Flags: review?(bugmail) → review+
Comment on attachment 8971472 [details]
Bug 1457249 - Use actually calculated value for the assertion that checks animation value is unchanged when we decided to skip calculation for the animation on debug build.

https://reviewboard.mozilla.org/r/240226/#review246110

::: gfx/layers/AnimationHelper.cpp:160
(Diff revision 1)
>  {
>    MOZ_ASSERT(!aAnimations.IsEmpty(), "Should be called with animations");
>  
>    bool hasInEffectAnimations = false;
> +#ifdef DEBUG
> +  bool shouldBeSkipped = false;

It would be worth adding a code comment here explaining a little bit what's going on. Something like this:

In cases where this function returns a SampleResult::Skipped, we actually do populate aAnimationValue in debug mode, so that we can MOZ_ASSERT at the call site that the value that would have been computed matches the stored value that we end up using. This flag is used to ensure we populate aAnimationValue in this scenario.
Attachment #8971472 - Flags: review?(bugmail) → review+
Thank you, kats!  I will add the comment there.
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1794c5b65d03
Assert that there is an animation value set by animations when we skip calculation for newly animation value. r=kats
https://hg.mozilla.org/integration/autoland/rev/ac3a8c1f0776
Factor out the function that converts servo's animation value to matrix. r=kats
https://hg.mozilla.org/integration/autoland/rev/053966aefdb3
Factor out the function that converts transform into device space. r=kats
https://hg.mozilla.org/integration/autoland/rev/ccef67dba025
Use actually calculated value for the assertion that checks animation value is unchanged when we decided to skip calculation for the animation on debug build. r=kats
You need to log in before you can comment on or make changes to this bug.