1238660 - crash in OOM | unknown | mozalloc_abort | mozalloc_handle_oom | moz_xmalloc | nsAnimationManager::BuildAnimations

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

This bug was filed from the Socorro interface and is 
report bp-3fce5789-2926-4d7c-8d7f-c73052160111.
=============================================================

When loading some pages (e.g. [1]), we run into some kind of infinite loop that eventually causes an OOM crash.

Regression range points to one of the bugs landed in 3a75759624a0 [2].

[1] http://www.meetup.com/hike-cincinnati/
[2] https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=3a75759624a0

Comment 1

[Brian Birtles (:birtles)]

Looking into it now.

Comment 2

[Brian Birtles (:birtles)]

So far I can tell the culprit is https://hg.mozilla.org/integration/mozilla-inbound/rev/5efd7a041b2d (bug 1228229) which, unfortunately, is the patch the switches from the old implementation of animation cascade resolution to the new implementation.

The only animation here seems to the be 'fontfix' animation. After some initial debugging, it seems like we create an opacity animation with a positive delay. Initially we set its 'wins-in-cascade' flag to true. Then when we call EffectCompositor::UpdateCascadeResults, we discover that it's not yet in-effect, hence it can't win in the cascade, so we set 'wins-in-cascade' to false. Since we've updated the cascade, we need to update animations on layers, so we schedule a restyle.

That's different to the previous implementation which didn't consider whether or not the animation was in effect or not before setting the "wins in cascade" flag the first time around.

The previous implementation was a bit odd since we would ignore the in-effect status when initially setting the "wins in cascade" flag but then consider it when deciding if we should update or not. And that in turn lead to a very similar infinite iteration bug which we worked around by forcibly update the cached in-effect status.

Right now I'm wondering if initializing 'wins-in-cascade' to false would actually work. We never used to do that since we'd only update that flag for compositor animatable-properties on CSS animations. We no longer have that restriction (we do, however, only consider compositor-animatable properties when detecting whether an animation is being overridden, but I don't think that should matter here).

Comment 3

[Brian Birtles (:birtles)]

(In reply to Brian Birtles (:birtles) from comment #2)
> The previous implementation was a bit odd since we would ignore the
> in-effect status when initially setting the "wins in cascade" flag but then
> consider it when deciding if we should update or not. And that in turn lead
> to a very similar infinite iteration bug which we worked around by forcibly
> update the cached in-effect status.

That was bug 1164813 for my reference.

Comment 4

[Brian Birtles (:birtles)]

Somewhat ironically, the problem content is this:

  <style type="text/css">
  a,p,em,span,strong{
    -webkit-animation-duration:0.1s;
    -webkit-animation-name:fontfix;
    -webkit-animation-iteration-count:1;
    -webkit-animation-timing-function:linear;
    -webkit-animation-delay:0.1s;
  }
  @-webkit-keyframes fontfix{from{opacity:1;}to{opacity:1;}}
  </style>

i.e. some webkit-specific font rendering workaround that we now render since we support -webkit prefixes :/

Comment 5

[Brian Birtles (:birtles)]

Attachment: Minimized test case (crashes browser!)

Comment 6

[Brian Birtles (:birtles)]

At first I thought, "Maybe we shouldn't be posting a restyle from UpdateCascadeResults at all." After all, it's a bit odd to be triggering more restyles from within a restyle.

However, I notice that we are deliberately doing that as of bug 847287, specifically this changeset:

https://hg.mozilla.org/mozilla-central/rev/de066f41c377
(which references the function called here: https://hg.mozilla.org/mozilla-central/rev/5b1671857589)

So I think we simply want to ensure that when we create an animation we don't unnecessarily trigger a subsequent restyle.

I think it makes sense that an animation is initially created its "wins in cascade" flag should be false and we update it once it actually becomes "in effect". So I think we should make that change at least.

However, I don't quite understand why this doesn't fail more often. In the reduced test case, we only reproduce the problem when the animation applies to both an element and its *grandchild* (not child).

Furthermore, when we update animations we don't take care to preserve the "wins in cascade" state like we do with the "is running on compositor" state. That explains why we end up with infinite iteration (as opposed to a single unnecessary restyle). We probably need to do something like what we did for "is running on compositor": https://hg.mozilla.org/mozilla-central/rev/ea91eabfc250

For now I'm going to make the following changes:

1. Make mWinsInCascade initially false.
2. Copy the mWinsInCascade state over when updating animations.

Comment 7

[Brian Birtles (:birtles)]

Attachment: part 1 - Make mWinsInCascade initially false

As of bug 1228229, the mWinsInCascade member of animation properties is set
consistently for both animations and transitions such that we only set this
to true if an animation is "in effect".

When an effect is initially created it is not "in effect" until it is attached
to a non-idle animation. We should, therefore, initialize this to false and,
when we become in effect, mark the cascade as needing an update.

Comment 8

[Brian Birtles (:birtles)]

Attachment: part 3 - Add crashtest

Comment 9

[Brian Birtles (:birtles)]

Attachment: part 2 - Preserve "wins in cascade" state when updating animations

When updating animations, we shouldn't unnecessarily clobber the "wins in
cascade" state of their properties since this can lead to unnecessary restyles
when we then decide we need to update the cascade.

Comment 10

[Brian Birtles (:birtles)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=31ddae45b90d

Comment 11

[Brian Birtles (:birtles)]

Comment on attachment 8706799 [details] [diff] [review]
part 3 - Add crashtest

This test fails without the patches from this bug applied.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8706799 [details] [diff] [review]
part 3 - Add crashtest

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

::: layout/style/crashtests/1238660-1.html
@@ +4,5 @@
> +    <title>Bug 1238660</title>
> +    <style>
> +      @keyframes opacity {
> +        to { opacity: 0 }
> +      }

I think this keyframes is not necessary.

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8706801 [details] [diff] [review]
part 2 - Preserve "wins in cascade" state when updating animations

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

I should have known this fast when I did write CopyIsRunningOnCompositor.

Comment 15

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #7)
> Created attachment 8706797 [details] [diff] [review]
> part 1 - Make mWinsInCascade initially false
> 
> As of bug 1228229, the mWinsInCascade member of animation properties is set
> consistently for both animations and transitions such that we only set this
> to true if an animation is "in effect".
> 
> When an effect is initially created it is not "in effect" until it is
> attached
> to a non-idle animation. We should, therefore, initialize this to false and,
> when we become in effect, mark the cascade as needing an update.

I wonder we do not need to set the flag false in case of KeyframeEffectReadOnly constructor?

https://dxr.mozilla.org/mozilla-central/rev/e790bba372f14241addda469a4bdb7ab00786ab3/dom/animation/KeyframeEffect.cpp#1378
https://dxr.mozilla.org/mozilla-central/rev/e790bba372f14241addda469a4bdb7ab00786ab3/dom/animation/KeyframeEffect.cpp#1580

If we need it, I am going to write crash tests when I write automation tests for element.animate().

Comment 16

[Brian Birtles (:birtles)]

Attachment: part 1 - Make mWinsInCascade initially false

Yes, I don't think we should set mWinsInCascade in the KeyframeEffectReadOnly constructor

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9955578aabd1
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb40d887724c
https://hg.mozilla.org/integration/mozilla-inbound/rev/f58e7ca07205

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9955578aabd1
https://hg.mozilla.org/mozilla-central/rev/fb40d887724c
https://hg.mozilla.org/mozilla-central/rev/f58e7ca07205