Bug 207361 - There's an event loop cycle between an animation finishing, and it being removed from GraphicsLayerCA
Summary: There's an event loop cycle between an animation finishing, and it being remo...
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
Keywords: InRadar
Depends on:
Reported: 2020-02-06 15:02 PST by Simon Fraser (smfr)
Modified: 2020-02-10 09:24 PST (History)
7 users (show)

See Also:

Simple hover transition (457 bytes, text/html)
2020-02-06 19:54 PST, Simon Fraser (smfr)
no flags Details
Patch (1.77 KB, patch)
2020-02-07 09:34 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (6.14 KB, patch)
2020-02-10 03:48 PST, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (8.23 KB, patch)
2020-02-10 05:50 PST, Antoine Quint
no flags 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) 2020-02-06 15:02:21 PST
For the rendering update where an animation finishes, Document::updateAnimationsAndSendEvents() runs and animation->tick() changes the state to Finished. isRelevant() is set to false.

However. at:
  if (!animation->isRelevant() && !animation->needsTick())

needsTick() is still true, so we don't add the animation to animationsToRemove.

That results in one frame where timeline->isRunningAcceleratedAnimationOnRenderer(renderer(), CSSPropertyTransform) returns false, but we haven't yet removed the animation from the GraphicsLayer, so GraphicsLayer::isRunningTransformAnimation() returns true. In this same frame, RenderLayerBacking::updateGeometry() has cleared the animation extent, so we're left with a GraphicsLayer that always keeps its backing store attached because it thinks it's running a transform animation with unknown extent.
Comment 1 Simon Fraser (smfr) 2020-02-06 15:42:47 PST
The event keeping needsTick() true is the transitionend event; see bug 207364.
Comment 2 Simon Fraser (smfr) 2020-02-06 19:54:38 PST
Created attachment 390050 [details]
Simple hover transition
Comment 3 Antoine Quint 2020-02-07 07:59:00 PST
What the code you refer to does is keeping the animation in m_animations which is correct, this is where we keep animations that are still relevant for the next update so that events and promises may be resolved.

The removal of any accelerated animation, as well as the blending of non-accelerated animations, is enqueued under KeyframeEffect::apply() which is called by WebAnimation::resolve(), which typically happens during style resolution under TreeResolver::createAnimatedElementUpdate().
Comment 4 Antoine Quint 2020-02-07 08:02:37 PST
That said maybe we ought to enqueue accelerated actions under Animation::tick() somewhere since this is the soonest we know that an animation's time has changed.
Comment 5 Antoine Quint 2020-02-07 08:05:54 PST
WebAnimation::tick() is also where we call WebAnimation::invalidateEffect(). Maybe we should just move the calls to KeyframeEffect::updateAcceleratedActions() under KeyframeEffect::invalidate(). We could then move m_phaseAtLastApplication there too and use this under KeyframeEffect::apply() so that we don't re-compute the timing properties. However, they could have been changed in the meantime, I'm not sure whether this is a problem just by looking at the code. What's absolutely required though is that the progress used to blend animations via style and the progress used to enqueue accelerated actions are the same.
Comment 6 Simon Fraser (smfr) 2020-02-07 09:34:53 PST
Created attachment 390089 [details]
Comment 7 Radar WebKit Bug Importer 2020-02-07 17:46:27 PST
Comment 8 Maciej Stachowiak 2020-02-09 13:31:47 PST
Comment on attachment 390089 [details]

This wasn't flagged for review, but seems fine. (windows failure seems unrelated)  Is it possible to make a test for this?
Comment 9 Antoine Quint 2020-02-10 03:48:17 PST
Created attachment 390242 [details]
Comment 10 Antoine Quint 2020-02-10 03:50:44 PST
Filed an improved patch which only updated accelerated actions under WebAnimation::tick() and WebAnimation::play(), Simon's previous patch would do it on each effect invalidation which happens much more frequently.

I'll let the bots chew first to see if there are any regressions, an initial local test pass of known animation tests didn't show any issue. If that's successful, I think I can make a test by adding some new testing functionality to query an effect's accelerated actions.
Comment 11 Antoine Quint 2020-02-10 05:50:07 PST
Looks good, filing a new patch with a modification to an existing test to lower the number of frames to wait before checking an accelerated animation was removed from the graphics layer, thus demonstrating that accelerated animations are enqueued and applied during the "update animations and send events" procedure. This modification would have failed prior to this patch.
Comment 12 Antoine Quint 2020-02-10 05:50:26 PST
Created attachment 390247 [details]
Comment 13 WebKit Commit Bot 2020-02-10 09:24:55 PST
Comment on attachment 390247 [details]

Clearing flags on attachment: 390247

Committed r256181: <https://trac.webkit.org/changeset/256181>
Comment 14 WebKit Commit Bot 2020-02-10 09:24:56 PST
All reviewed patches have been landed.  Closing bug.