Closed Bug 1161049 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Layout, defect, critical)

ARM
Android
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: aaronmt, Assigned: dbaron)

References

Details

(Keywords: crash)

Crash Data

Attachments

(3 files)

This bug was filed from the Socorro interface and is 
report bp-74aff38e-330c-4279-a16b-f28fc2150503.
=============================================================
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.)
Flags: needinfo?(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".
Flags: needinfo?(mstange)
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
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).
Attachment #8601783 - Flags: review?(mstange)
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.
Attachment #8601785 - Flags: review?(mstange)
Do you think we should back out any other parts of bug 947753?
Flags: needinfo?(mstange)
Attachment #8601784 - Flags: review?(bbirtles) → review+
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.
Attachment #8601783 - Flags: review?(mstange) → review+
Attachment #8601785 - Flags: review?(mstange) → review+
(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.
Flags: needinfo?(mstange)
(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.
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.
(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.
https://hg.mozilla.org/mozilla-central/rev/e09370e4a895
https://hg.mozilla.org/mozilla-central/rev/c33b62fc04ac
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.