Bug 207361

Summary:

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

Product:

WebKit

Reporter:

Simon Fraser (smfr) <simon.fraser>

Component:

Animations

Assignee:

Antoine Quint <graouts>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, dino, graouts, graouts, mjs, simon.fraser, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

Safari Technology Preview

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugs.webkit.org/show_bug.cgi?id=207364

Attachments:

Description	Flags
Simple hover transition	none
Patch	none
Patch	none
Patch	none

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]
Patch

Comment 7 Radar WebKit Bug Importer 2020-02-07 17:46:27 PST

<rdar://problem/59280370>

Comment 8 Maciej Stachowiak 2020-02-09 13:31:47 PST

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?

Comment 9 Antoine Quint 2020-02-10 03:48:17 PST

Created attachment 390242 [details]
Patch

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]
Patch

Comment 13 WebKit Commit Bot 2020-02-10 09:24:55 PST

Comment on attachment 390247 [details]
Patch

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.