Bug 207361 – There's an event loop cycle between an animation finishing, and it being removed from GraphicsLayerCA

• Component: Animations
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

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.

[Simon Fraser (smfr)]

The event keeping needsTick() true is the transitionend event; see bug 207364.

[Simon Fraser (smfr)]

Created attachment 390050 [details]
Simple hover transition

[Antoine Quint]

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().

[Antoine Quint]

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.

[Antoine Quint]

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.

[Simon Fraser (smfr)]

Created attachment 390089 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/59280370>

[Maciej Stachowiak]

Comment on attachment 390089 [details]
Patch

This wasn't flagged for review, but seems fine. (windows failure seems unrelated)  Is it possible to make a test for this?

[Antoine Quint]

Created attachment 390242 [details]
Patch

[Antoine Quint]

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.

[Antoine Quint]

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.

[Antoine Quint]

Created attachment 390247 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 390247 [details]
Patch

Clearing flags on attachment: 390247

Committed r256181: <https://trac.webkit.org/changeset/256181>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.