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.
The event keeping needsTick() true is the transitionend event; see bug 207364.
Created attachment 390050 [details] Simple hover transition
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().
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.
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.
Created attachment 390089 [details] Patch
<rdar://problem/59280370>
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?
Created attachment 390242 [details] Patch
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.
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.
Created attachment 390247 [details] Patch
Comment on attachment 390247 [details] Patch Clearing flags on attachment: 390247 Committed r256181: <https://trac.webkit.org/changeset/256181>
All reviewed patches have been landed. Closing bug.