Closed Bug 1091296 Opened 8 years ago Closed 8 years ago

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


(Core :: Panning and Zooming, defect)

32 Branch
Gonk (Firefox OS)
Not set



Tracking Status
firefox36 --- fixed
b2g-v2.1 --- wontfix
b2g-v2.2 --- fixed


(Reporter: khuey, Assigned: kats)



(Keywords: assertion)


(3 files, 2 obsolete files)


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

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.
Attachment #8518921 - Flags: review?(matt.woodrow)
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:
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+
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.
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 on attachment 8518921 [details] [diff] [review]
Part 1 - Refactoring

NOTE: Please see 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.
Attachment #8518921 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34-
Attachment #8519970 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34-
Attachment #8519971 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34-
You need to log in before you can comment on or make changes to this bug.