1161049 - crash in mozilla::AnimationCollection::CanPerformOnCompositorThread(mozilla::AnimationCollection::CanAnimateFlags) const

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Aaron Train [:aaronmt]]

This bug was filed from the Socorro interface and is 
report bp-74aff38e-330c-4279-a16b-f28fc2150503.
=============================================================

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

mstange, any thoughts on what we should do when widget->GetLayerManager() returns null?  It looks like the android widget code is different and doesn't lazily create the layer manager when GetLayerManager is called, but instead does so in response to an AndroidGeckoEvent::COMPOSITOR_CREATE event.

Might it be easier to add yet another method to the widget API to ask whether an OMTC layer manager will be used?  (Seems unnecessary, though.)

Comment 2

[Markus Stange [:mstange]]

(In reply to David Baron [:dbaron] ⏰UTC+2 from comment #1)
> Might it be easier to add yet another method to the widget API to ask
> whether an OMTC layer manager will be used?  (Seems unnecessary, though.)

That sounds like the best approach to me. Android can just always return true from that method.
I think adding such a method is better than having an implicit assumption like "null layer manager => android => omtc".

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 1 - Back out changeset 962e15b81684 (part of bug 947753) in order to fix the bug a different way

This patch seems to be leading to too many complications; it requires us
to predict whether a layer that would be created for an element will use
off-main-thread compositing prior to the creation of that layer, which
has led to complications on both Mac (fixed by the other patches in bug
947753) and Android (bug 1161049).

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 2 - Add comments reflecting what CanPerformOnCompositorThread doesn't check

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 3 - Don't send animations to a layer if we're not using off-main-thread compositing

This refixes bug 947753 in a way that leads to fewer complications,
since we don't need to predict what kind of layer manager an element's
layer will have before the layer is actually created.  It has the
disadvantage that
AnimationPlayerCollection::CanPerformOnCompositorThread isn't really
telling the truth in cases where we won't have an layer that does
off-main-thread compositing.  This means that we will force the creation
of a layer to receive the animations (which might actually be good),
although it may have some disadvantages.

It also means that the additional (unlanded) patch in bug 947753 still
isn't needed, since we never set the animation generation on the layer,
so we will never try to throttle (suppress main thread execution) of
animations.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Do you think we should back out any other parts of bug 947753?

Comment 7

[Markus Stange [:mstange]]

Comment on attachment 8601783 [details] [diff] [review]
patch 1 - Back out changeset 962e15b81684 (part of bug 947753) in order to fix the bug a different way

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

Okay. I'm not convinced that this is the right approach and that we shouldn't continue with the previous one, but it doesn't matter much in the end.

Comment 8

[Markus Stange [:mstange]]

(In reply to David Baron [:dbaron] ⏰UTC+2 (busy, returning May 21) from comment #6)
> Do you think we should back out any other parts of bug 947753?

No, I think all changes in that bug were strict improvements that we should have made anyway.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Markus Stange [:mstange] from comment #7)
> Okay. I'm not convinced that this is the right approach and that we
> shouldn't continue with the previous one, but it doesn't matter much in the
> end.

My basic rationale for wanting to go this way is simplicity; the approach in comment 2 means more code -- and more code that has to be consistent with other code (providing more places to get something wrong).

Having CanPerformOnCompositorThread not check whether we can actually animate on the compositor thread feels a little weird, but we don't have anything concrete that's a problem with doing things that way.

So I'm inclined to prefer something simpler, at least as long as we don't have things that are broken by being simpler.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b1385ccee5f2
https://hg.mozilla.org/integration/mozilla-inbound/rev/e09370e4a895
https://hg.mozilla.org/integration/mozilla-inbound/rev/c33b62fc04ac

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Once this hits nightly it's worth verifying that it does actually fix the crash.  That said, as far as untested crash fixes go, I'm reasonably confident of this one.

Comment 12

[Markus Stange [:mstange]]

(In reply to David Baron [:dbaron] ⏰UTC+2 (busy, returning May 21) from comment #9)
> (In reply to Markus Stange [:mstange] from comment #7)
> > Okay. I'm not convinced that this is the right approach and that we
> > shouldn't continue with the previous one, but it doesn't matter much in the
> > end.
> 
> My basic rationale for wanting to go this way is simplicity; the approach in
> comment 2 means more code

This is a very good point. I'm convinced.

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e09370e4a895
https://hg.mozilla.org/mozilla-central/rev/c33b62fc04ac