1237467 - Delete empty effect sets

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Brian Birtles (:birtles)]

In bug 1232577, I tried to add an optimization to EffectCompositor::GetAnimationRule that would do an early return that returns nullptr when the effect set is empty. This caused tests such as css-animations/test_animation-cancel.html to fail, specifically, 'After cancelling an animation, it can still be re-used' test would fail after restarting the animation.

After some debugging I discovered it's because when we do the early return we fail to go through and clean up the mElementsToRestyle map.

Specifically, the chain of events is something like the following:

1. When we cancel, we remove the elements from the effect set and we post a
   standard restyle which adds the element to mElementsToRestyle.
2. Then we call EffectCompositor::GetAnimationRule but we return early (since
   the effect set is not empty) so we *neither*
   - update EffectCompositor::mElementsToRestyle, nor
   - update the animation rule on the effect set
3. Then when we play again we correctly detect that we need to request a restyle
   (We're getting lucky here since we failed to update mProgressOnLastCompose
    when we cancelled so it's really just chance that we actually get a
    different progress value. However, even if we were unlucky here, the
    PostUpdate call inside Play() will make sure we get a restyle.)
4. However, in EffectCompositor::RequestRestyle we notice we already have the
   element in mElementsToRestyle so we don't post a further restyle for this
   element and the old animation rule remains in use.

We already know we need to delete EffectSets when they are empty so we should fix all these problems at once.

Specifically, in this bug we should:
  
  1. Find some safe way of ensuring an EffectSet is not being enumerated so we
     can safely delete it when it is empty.
  2. Actually delete the effect set when it is empty.
  3. When we remove ourselves from the effect set, reset our
     mProgressOnLastCompose to null but only after making sure we post a
     standard restyle first.
  4. In GetAnimationRule, if the effect set is null, also clear the entry from
     mElementsToRestyle before doing an early return.

Comment 1

[Brian Birtles (:birtles)]

Attachment: part 1 - No longer mark element as needing an animation restyle if we go to restyle it and it no longer has an effect set

Comment 2

[Brian Birtles (:birtles)]

Attachment: part 2 - Clear mProgressOnLastCompose when the effect is no longer relevant

Comment 3

[Brian Birtles (:birtles)]

Attachment: part 3 - Add debug methods to determine if an EffectSet is currently being enumerated

Comment 4

[Brian Birtles (:birtles)]

Attachment: part 4 - Delete the EffectSet when it becomes empty

Comment 5

[Brian Birtles (:birtles)]

Attachment: part 1 - No longer mark element as needing an animation restyle if we go to restyle it and it no longer has an effect set

Comment 6

[Brian Birtles (:birtles)]

Attachment: part 2 - Clear mProgressOnLastCompose when the effect is no longer relevant

Comment 7

[Brian Birtles (:birtles)]

Attachment: part 3 - Add debug methods to determine if an EffectSet is currently being enumerated

I'm really not sure how best to set this up. Originally I tried making this
debug-only but all the #ifdefs became very distracting (see attachment 8706804 [details] [diff] [review])
so I just dropped them.

On further thought, I should probably try using DebugOnly. If I add a DebugOnly
member, it will still take up one byte of space, plus padding, but at least the
increment/decrement code would be optimized out.

Comment 8

[Brian Birtles (:birtles)]

Attachment: part 3 - Add debug methods to determine if an EffectSet is currently being enumerated

Switch to using DebugOnly

Comment 9

[Brian Birtles (:birtles)]

Attachment: part 4 - Delete the EffectSet when it becomes empty

Comment 10

[Cameron McCormack (:heycam)]

Comment on attachment 8707209 [details] [diff] [review]
part 2 - Clear mProgressOnLastCompose when the effect is no longer relevant

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

::: dom/animation/KeyframeEffect.cpp
@@ +187,5 @@
>                         mAnimation->CascadeLevel());
>      }
> +
> +    // If we're not relevant, we will have been removed from the EffectSet
> +    // so when the restyle we just requested actually gets run, our

This sentence doesn't look right.

Comment 11

[Cameron McCormack (:heycam)]

Comment on attachment 8707220 [details] [diff] [review]
part 3 - Add debug methods to determine if an EffectSet is currently being enumerated

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

Passing the DebugOnly<> around looks a little odd to me, but OK...

Comment 12

[Brian Birtles (:birtles)]

(In reply to Cameron McCormack (:heycam) from comment #11)
> Comment on attachment 8707220 [details] [diff] [review]
> part 3 - Add debug methods to determine if an EffectSet is currently being
> enumerated
> 
> Review of attachment 8707220 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Passing the DebugOnly<> around looks a little odd to me, but OK...

Yeah, I tried a few different patterns like passing a pointer and conditionally setting it depending on if DEBUG was defined (attachment 8706804 [details] [diff] [review]) or just doing all the refcounting work even in release builds (attachment 8707219 [details] [diff] [review]) but this version here seemed the cleanest. Do you have any other suggestions?

Comment 13

[Cameron McCormack (:heycam)]

No, apart from perhaps passing in the EffectSet itself and not the DebugOnly<> (and then making Iterator a friend of the EffectSet, and poking at the member variable).

Comment 14

[Brian Birtles (:birtles)]

(In reply to Cameron McCormack (:heycam) from comment #13)
> No, apart from perhaps passing in the EffectSet itself and not the
> DebugOnly<> (and then making Iterator a friend of the EffectSet, and poking
> at the member variable).

That's not a bad idea. I'll give it a try.

Comment 15

[Brian Birtles (:birtles)]

Attachment: part 3 - Add debug methods to determine if an EffectSet is currently being enumerated

How about this?

Comment 16

[Cameron McCormack (:heycam)]

Comment on attachment 8707331 [details] [diff] [review]
part 3 - Add debug methods to determine if an EffectSet is currently being enumerated

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

I think that looks a lot better, thanks.

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a3d7e16b1e0
https://hg.mozilla.org/integration/mozilla-inbound/rev/d63ca2677bd5
https://hg.mozilla.org/integration/mozilla-inbound/rev/7609ca218902
https://hg.mozilla.org/integration/mozilla-inbound/rev/c47a6e0a6d95

Comment 18

[Brian Birtles (:birtles)]

This got backed out for failures like the following:
http://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-inbound-macosx64-debug/1452739469/mozilla-inbound_snowleopard-debug_test-mochitest-devtools-chrome-4-bm107-tests1-macosx-build2904.txt.gz

After some discussion on IRC with bz and glandium, it seems like despite 0x0 being reported as the crash address, we might actually have a deref after free situation.

For example, one scenario that seems likely to me is that in EffectCompositor::GetAnimationRule, we initially check for a null effect set, then we call MaybeUpdateAnimationRule, and something along the way triggers a call to KeyframeEffectReadOnly::NotifyAnimationTimingUpdated where we end up deleting the effect set.

Then, at the end of EffectCompositor::GetAnimationRule, we fetch the animation rule of the now-dead effect set (that method is inlined so we wouldn't expect to get any complaints in doing that) and pass that back. That animation rule, however, is also dead.

If any of that turns out to be true, in addition to re-fetching the effect set after calling MaybeUpdateAnimationRule, we should make the dtor for EffectSet null out the animation rules on it so we don't have other potential situations of deref after free.

Comment 19

[Brian Birtles (:birtles)]

Attachment: part 1 - No longer mark element as needing an animation restyle if we go to restyle it and it no longer has an effect set

Hi Cameron, sorry, I know I said I wouldn't bother you with more review
requests this week but I want to try landing this again.

The trouble is, I don't fully understand what is going wrong. I wrote my theory
in comment 18 but when I added assertions to that check that, they didn't fail.
In fact, I added all sorts of assertions all over the place and none of them
failed so I suspect that the assertions are simply not showing up in the log
and that the theory is correct.

For example, I added an assertion that GetEffectSet doesn't return a null
pointer after MaybeUpdateAnimationRule and it fails to fire but the crash
occurs:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=21188a69c295

However, when I simply replace that assertion with null check and early return
the crash no longer occurs:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f1ae75c541f

Note: I tried using a ternary operator for the early return but that produced
a build error on Mac since we end up creating a temporary RefPtr only to
dereference it:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=621fc54677cc

I thought that perhaps that was the issue (accidentally destroying the
AnimationRule) but it doesn't appear to be.

I'd like to understand this further, but it only reproduces on 10.6 and it
currently takes about ~6 hours to get a result every time I push to try for
10.6 mochitests. I've been trying to get a local build going on an old 10.6
machine in the office but still haven't succeeded.

So, to recap, my theory is still that we are deleting the effect set while we
have a raw pointer to it. Then we are calling AnimationRule() on the freed
object and getting back a pointer to the freed animation rule and passing that
back.

I will follow up with a change to part 4 that also nulls out the animation rule
in the destructor for EffectSet so that even if this situation does happen
again, we will avoid crashing since any callers of GetAnimationRule should
already be checking for a nullptr result.

Comment 20

[Brian Birtles (:birtles)]

Attachment: part 4 - Delete the EffectSet when it becomes empty

Here is the updated part 4 with changes to the EffectSet dtor.

A try run with this change included looks good:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=69cac778c570&selectedJob=15485223

Comment 21

[Cameron McCormack (:heycam)]

Comment on attachment 8708202 [details] [diff] [review]
part 1 - No longer mark element as needing an animation restyle if we go to restyle it and it no longer has an effect set

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

r=me on this one after some IRC discussion explaining why we no longer need to update mElementsToRestyle.

Comment 22

[Cameron McCormack (:heycam)]

Comment on attachment 8708203 [details] [diff] [review]
part 4 - Delete the EffectSet when it becomes empty

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

If your investigation shows that part 1 is the correct fix, I don't think we need this.

Comment 23

[Brian Birtles (:birtles)]

Cameron, you were right, we're getting a new EffectSet. The assertions and extra logging in this try run show the EffectSet returned from GetEffectSet changes either side of the call to MaybeUpdateAnimationRule:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=1804d9f83cbd

Dropping the changes to part 4 and adding an assertion that mElementsToRestyle is cleared by MaybeUpdateAnimationRule (as discussed on IRC):

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=99f15387e8f8

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d51246e9c8f
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fdf99692385
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f5396096fd9
https://hg.mozilla.org/integration/mozilla-inbound/rev/d51451c41cab

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1d51246e9c8f
https://hg.mozilla.org/mozilla-central/rev/7fdf99692385
https://hg.mozilla.org/mozilla-central/rev/9f5396096fd9
https://hg.mozilla.org/mozilla-central/rev/d51451c41cab