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

RESOLVED FIXED in Firefox 36, Firefox OS v2.2

Status

()

Core
Panning and Zooming
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: khuey, Assigned: kats)

Tracking

({assertion})

32 Branch
mozilla36
ARM
Gonk (Firefox OS)
assertion
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox36 fixed, b2g-v2.1 wontfix, b2g-v2.2 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

STR:

Set up two email accounts on b2g
Click the drop down selector in the hamburger panel to switch between them
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"?
Flags: needinfo?(roc)
It looks like an APZC bug to me.
Flags: needinfo?(roc)
(I don't know what causes the bug, it's just that that assertion definitely indicates a bug.)
Component: Graphics: Layers → Panning and Zooming
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.
I'm on a Nexus 5, so my viewport is bigger ;)
Ok, I reproduced it by returning true at http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp?rev=0af9b31d714c#5017.

Will investigate.
Assignee: nobody → bugmail.mozilla
And actually I can't reproduce this on tip.  Not sure if that's because Gecko changed or Gaia changed.
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.
Created attachment 8518921 [details] [diff] [review]
Part 1 - Refactoring
Attachment #8518921 - Flags: review?(matt.woodrow)
Created attachment 8518922 [details] [diff] [review]
Part 2 - When compositing start each frame with a clear shadowtransform
Attachment #8518922 - Flags: review?(matt.woodrow)
Created attachment 8518923 [details] [diff] [review]
Part 3 - Combine APZ transform with OMTA transform instead of clobbering it
Attachment #8518923 - Flags: review?(matt.woodrow)
(Assignee)

Comment 12

4 years ago
try
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
Attachment #8518921 - Flags: review?(matt.woodrow) → review+
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.
Attachment #8518922 - Flags: review?(matt.woodrow) → review-
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.
Attachment #8518923 - Flags: review?(matt.woodrow) → review+
Created attachment 8519970 [details] [diff] [review]
Part 2 - Remove redundant code to reset shadow transform (v2)
Attachment #8518922 - Attachment is obsolete: true
Attachment #8519970 - Flags: review?(matt.woodrow)
Created attachment 8519971 [details] [diff] [review]
Part 3 - Combine APZ transform with OMTA transform instead of clobbering it (v2)

Updated comment; carrying r+
Attachment #8518923 - Attachment is obsolete: true
Attachment #8519971 - Flags: review+
Attachment #8519970 - Flags: review?(matt.woodrow) → review+
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
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.
status-b2g-v2.1: --- → affected
status-b2g-v2.2: --- → fixed
status-firefox36: --- → fixed
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
Attachment #8518921 - Flags: approval-mozilla-b2g34?
Attachment #8519970 - Flags: approval-mozilla-b2g34?
Attachment #8519971 - Flags: approval-mozilla-b2g34?
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.

Updated

4 years ago
Attachment #8518921 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34-

Updated

4 years ago
Attachment #8519970 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34-

Updated

4 years ago
Attachment #8519971 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34-
status-b2g-v2.1: affected → wontfix
You need to log in before you can comment on or make changes to this bug.