Bug 25108 – Animation timer can keep firing after accelerated transitions finish

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Simon Fraser (smfr)]

There's a problem in the animation code that seems to only show up with accelerated compositing turned on. In some cases, the updateAnimationTimer() can keep firing, even when a transition has finished. I'll attach a testcase.

[Simon Fraser (smfr)]

Created attachment 29357 [details]
Testcase

You'll have to put a breakpoint in updateAnimationTimer() after the transition finishes in order to see the problem.

[Simon Fraser (smfr)]

The problem seems to occur because animation controller assumes that calling setChanged() on a node will result in a subsequent call to CompositeAnimation::animate() for the appropriate renderer; that method can then call cleanupFinishedAnimations().

However, in this testcase, AnimationController::updateAnimations() is returning early, because neither oldStyle nor newStyle contain animations or transitions. I'm not sure how this can be, yet.

[Simon Fraser (smfr)]

Ah, this happens when the transition styles are removed before the transition ends.

[Chris Marrin]

Created attachment 29394 [details]
Patch

[Darin Adler]

Comment on attachment 29394 [details]
Patch

> +    // We can now safely remove any animations or transitions that are finished.
> +    // We can't remove them any earlier because we might get a false restart of
> +    // a transition. This can happen because we have not yet set the final property
> +    // value until we call the rendering dispatcher. So this can make the current
> +    // style slightly different from the desired final style (because our last 
> +    // animation step was, say 0.9999 or something). And we need to remove them
> +    // here because if there are no more animations running we'll never get back
> +    // into the animation code to clean them up.
> +    RenderObjectAnimationMap::const_iterator animationsEnd = m_compositeAnimations.end();
> +    for (RenderObjectAnimationMap::const_iterator it = m_compositeAnimations.begin(); it != animationsEnd; ++it) {
> +        RefPtr<CompositeAnimation> compAnim = it->second;
> +        compAnim->cleanupFinishedAnimations();
> +    }

Is there a guarantee that the cleanupFinishedAnimations function and the functions it calls won't result in a change to m_compositeAnimations? If not, then we'll need to copy the animations out of the map before iterating them, because you can't safely iterate a hash table if it might change.

This unnecessarily thrashes the reference count of the CompositeAnimation object. The local variable should be a raw pointer, or be eliminated entirely.

review- because of these two issues

[Simon Fraser (smfr)]

Comment on attachment 29394 [details]
Patch

Ooops, we had a mid-air conflict. I'll clean up and re-submit the patch.

[Simon Fraser (smfr)]

Created attachment 29398 [details]
Revised patch

I fixed some other lines to reduce ref churn while iterating, and added a comment that cleanupFinishedAnimations() will never affect m_compositeAnimations.

[Simon Fraser (smfr)]

http://trac.webkit.org/changeset/42410