Bug 25108 - Animation timer can keep firing after accelerated transitions finish
Summary: Animation timer can keep firing after accelerated transitions finish
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Chris Marrin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-04-08 21:53 PDT by Simon Fraser (smfr)
Modified: 2009-04-10 16:27 PDT (History)
1 user (show)

See Also:


Attachments
Testcase (942 bytes, text/html)
2009-04-08 21:54 PDT, Simon Fraser (smfr)
no flags Details
Patch (5.16 KB, patch)
2009-04-10 11:35 PDT, Chris Marrin
simon.fraser: review-
Details | Formatted Diff | Diff
Revised patch (10.16 KB, patch)
2009-04-10 14:33 PDT, Simon Fraser (smfr)
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2009-04-08 21:53:26 PDT
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.
Comment 1 Simon Fraser (smfr) 2009-04-08 21:54:10 PDT
Created attachment 29357 [details]
Testcase

You'll have to put a breakpoint in updateAnimationTimer() after the transition finishes in order to see the problem.
Comment 2 Simon Fraser (smfr) 2009-04-08 21:56:56 PDT
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.
Comment 3 Simon Fraser (smfr) 2009-04-08 22:06:16 PDT
Ah, this happens when the transition styles are removed before the transition ends.
Comment 4 Chris Marrin 2009-04-10 11:35:27 PDT
Created attachment 29394 [details]
Patch
Comment 5 Darin Adler 2009-04-10 13:44:01 PDT
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
Comment 6 Simon Fraser (smfr) 2009-04-10 14:16:56 PDT
Comment on attachment 29394 [details]
Patch

Ooops, we had a mid-air conflict. I'll clean up and re-submit the patch.
Comment 7 Simon Fraser (smfr) 2009-04-10 14:33:55 PDT
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.
Comment 8 Simon Fraser (smfr) 2009-04-10 16:27:12 PDT
http://trac.webkit.org/changeset/42410