1091296 - ASSERTION: overwriting animated transform! 'layerComposite->GetShadowTransformSetByAnimation()'

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

STR:

Set up two email accounts on b2g
Click the drop down selector in the hamburger panel to switch between them

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

Hmm, I'm thinking this is either a recent regression or the assertion doesn't matter, I've been running with two e-mail accounts for a while.  Rob, is this assertion "something weird is going on", or "something dangerous is going on"?

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It looks like an APZC bug to me.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(I don't know what causes the bug, it's just that that assertion definitely indicates a bug.)

Comment 4

[Kartikaya Gupta (email:[e-mail])]

I'm not able to reproduce this on Flame master. When I follow the STR in comment 0 I do see this:

11-06 14:50:36.326 I/Gecko   ( 9346): Performance warning: Async animation disabled because frame size (320, 799) is bigger than the viewport (360, 607) or the visual rectangle (320, 799) is larger than the max allowable value (17895698) [div]

so maybe in Kyle's case he is getting OMTA whereas for me it is disabled, and so he sees the assertion whereas I do not.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I'm on a Nexus 5, so my viewport is bigger ;)

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Ok, I reproduced it by returning true at http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp?rev=0af9b31d714c#5017.

Will investigate.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

And actually I can't reproduce this on tip.  Not sure if that's because Gecko changed or Gaia changed.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

From what I can tell it's a legitimate scenario now to have both APZ and OMTA transforms on a layer and we just never had the code before to handle it. It should be straightforward enough to combine the two transforms properly and get rid of the warning.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 - Refactoring

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 - When compositing start each frame with a clear shadowtransform

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 3 - Combine APZ transform with OMTA transform instead of clobbering it

Comment 12

[Kartikaya Gupta (email:[e-mail])]

I didn't bother fixing up the analogous situation in TransformScrollableLayer which would only trigger on Fennec. The reason I didn't is because (a) since Fennec doesn't have subframe async scrolling, it's not likely to hit a scenario where a layer has both OMTA and JPZC (Java PZC) transforms. And (b) that function is going to go away entirely once we port Fennec over to the C++ APZC. I would have done it anyway if it was trivial but there's some weirdness with the overscroll handling codepath there that I don't want to mess with.

Try push with above three patches:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=99e3b8967dce

Comment 13

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8518922 [details] [diff] [review]
Part 2 - When compositing start each frame with a clear shadowtransform

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

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +962,5 @@
>  
> +  // Clear any async transforms set in previous frames. This is necessary
> +  // because some things called by ApplyAsyncContentTransformToTree add to the
> +  // shadow transform rather than overwriting it.
> +  ClearAsyncTransforms(root);

I don't think we need to do this at all, we call SetShadowProperties() in CompositorParent before this.

Comment 14

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8518923 [details] [diff] [review]
Part 3 - Combine APZ transform with OMTA transform instead of clobbering it

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

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +636,5 @@
>      oldTransform.PreScale(resolution.scale, resolution.scale, 1);
>  
>      // For the purpose of aligning fixed and sticky layers, we disregard
> +    // the overscroll transform as well as any OMTA transform when computing the
> +    // 'aCurrentTransformForRoot' parameter. This ensures that the overscroll

Maybe explicitly mention that the use of GetTransform() is what skips those additions. The naming of GetTransform vs GetLocalTransform isn't overly helpful.

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 - Remove redundant code to reset shadow transform (v2)

Comment 16

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 3 - Combine APZ transform with OMTA transform instead of clobbering it (v2)

Updated comment; carrying r+

Comment 17

[Kartikaya Gupta (email:[e-mail])]

I did a new try push because the one above has a suspicious-looking orange but I don't know if it's real or just because the trees are collapsing. Also this one does Fennec instead of B2G for wider try coverage.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d64134a0d665

Comment 18

[Kartikaya Gupta (email:[e-mail])]

Try push was green, so landed:

remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/b9ebaad549c3
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/357845594db4
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/6fd76ef0601f

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b9ebaad549c3
https://hg.mozilla.org/mozilla-central/rev/357845594db4
https://hg.mozilla.org/mozilla-central/rev/6fd76ef0601f

Comment 20

[Kartikaya Gupta (email:[e-mail])]

Assuming this affects 2.1 as well since it would have resulted from multi-layer-apz and the introduction of containerless scrolling. I'll let this bake a bit and then request uplift.

Comment 21

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8518921 [details] [diff] [review]
Part 1 - Refactoring

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 967844 probably
User impact if declined: scrollable layers which have CSS animations might not animate and instead would just 'jump' to the final position.
Testing completed: on m-c for a while now
Risk to taking this patch (and alternatives if risky): should be pretty safe at this point given the bake time. the patches might need a bit of rebasing to uplift.
String or UUID changes made by this patch: none

Comment 22

[bhavana bajaj [:bajaj]]

Discussed offline with :kats on the common occurrence of this issue in different scenarios and determined it to be rare and something that may not be noticeable by an end user(without obviously looking at the assertion). I am worried about taking any code change at this point which does not have a critical user impact and dealing with any fallouts and this one is better off riding the trains.