Bug 202190 – [Web Animations] Implement replaced animations

• Status: RESOLVED
• Resolution: FIXED
• Component: Animations

[Antoine Quint]

Before we can ship Web Animations, we need to implement https://drafts.csswg.org/web-animations/#replacing-animations.

[Radar WebKit Bug Importer]

<rdar://problem/55697719>

[Antoine Quint]

Created attachment 380002 [details]
Patch

[Dean Jackson]

Comment on attachment 380002 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=380002&action=review

> Source/WebCore/animation/AnimationTimeline.cpp:195
> +            Vector<RefPtr<WebAnimation>> sortedWebAnimations;

Initialize the vector with the size of webAnimations.size()

> Source/WebCore/animation/DocumentTimeline.cpp:450
> +    auto animations = animationsForElement(*target, AnimationTimeline::Ordering::Sorted);
> +    for (auto& animationWithHigherCompositeOrder : WTF::makeReversedRange(animations)) {

If you always access them in this reversed manner, why not have Ordering::SortedDescending or something like that, to avoid sorting then reversing?

> Source/WebCore/animation/DocumentTimeline.cpp:476
> +    for (const auto& animation : m_allAnimations) {

Do you need the const here?

> Source/WebCore/animation/DocumentTimeline.cpp:480
> +            animation->setReplaceState(WebAnimation::ReplaceState::Removed);

Also, this looks like a non-const method?

> Source/WebCore/animation/DocumentTimeline.cpp:488
> +            // 6. If animation has a document for timing, then append removeEvent to its document for timing's pending animation
> +            //    event queue along with its target, animation. For the scheduled event time, use the result of applying the procedure
> +            //    to convert timeline time to origin-relative time to the current time of the timeline with which animation is associated.
> +            //    Otherwise, queue a task to dispatch removeEvent at animation. The task source for this task is the DOM manipulation task source.

It's not clear you're doing this. What does queue a task to dispatch removeEvent at animation mean?

[Antoine Quint]

(In reply to Dean Jackson from comment #3)
> Comment on attachment 380002 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=380002&action=review
> 
> > Source/WebCore/animation/AnimationTimeline.cpp:195
> > +            Vector<RefPtr<WebAnimation>> sortedWebAnimations;
> 
> Initialize the vector with the size of webAnimations.size()

Will do.

> > Source/WebCore/animation/DocumentTimeline.cpp:450
> > +    auto animations = animationsForElement(*target, AnimationTimeline::Ordering::Sorted);
> > +    for (auto& animationWithHigherCompositeOrder : WTF::makeReversedRange(animations)) {
> 
> If you always access them in this reversed manner, why not have
> Ordering::SortedDescending or something like that, to avoid sorting then
> reversing?

It would make animationsForElement() more complex while I don't believe there's any cost in using this reversed iterator. But I could be mistaken!

> > Source/WebCore/animation/DocumentTimeline.cpp:476
> > +    for (const auto& animation : m_allAnimations) {
> 
> Do you need the const here?
> 
> > Source/WebCore/animation/DocumentTimeline.cpp:480
> > +            animation->setReplaceState(WebAnimation::ReplaceState::Removed);
> 
> Also, this looks like a non-const method?

Correct, I'll remove the const here.

> > Source/WebCore/animation/DocumentTimeline.cpp:488
> > +            // 6. If animation has a document for timing, then append removeEvent to its document for timing's pending animation
> > +            //    event queue along with its target, animation. For the scheduled event time, use the result of applying the procedure
> > +            //    to convert timeline time to origin-relative time to the current time of the timeline with which animation is associated.
> > +            //    Otherwise, queue a task to dispatch removeEvent at animation. The task source for this task is the DOM manipulation task source.
> 
> It's not clear you're doing this. What does queue a task to dispatch
> removeEvent at animation mean?

It's done by WebAnimation::enqueueAnimationPlaybackEvent().

[Antoine Quint]

Committed r250603: <https://trac.webkit.org/changeset/250603>

[Truitt Savell]

It looks like the changes in https://trac.webkit.org/changeset/250603/webkit

is causing testing to exit early with 50 crashes.

Build:
https://build.webkit.org/builders/Apple%20Mojave%20Release%20WK2%20%28Tests%29/builds/7061

results:
https://build.webkit.org/results/Apple%20Mojave%20Release%20WK2%20(Tests)/r250603%20(7061)/results.html

I was able to reproduce this locally by running the effected tests on 250603 which failed, and 250602 which passed. Can this be addressed quickly? Otherwise we will need to roll this back.

[Truitt Savell]

Reverted r250603 for reason:

Caused testing to exit early with crashes on Mac

Committed r250607: <https://trac.webkit.org/changeset/250607>

[Antoine Quint]

Committed r250617: <https://trac.webkit.org/changeset/250617>

[Truitt Savell]

It looks like the changes in https://trac.webkit.org/changeset/250617/webkit

caused this test for crash on debug: imported/w3c/web-platform-tests/web-animations/timing-model/timelines/update-and-send-events-replacement.html

History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=imported%2Fw3c%2Fweb-platform-tests%2Fweb-animations%2Ftiming-model%2Ftimelines%2Fupdate-and-send-events-replacement.html

Crash Log:
No crash log found for DumpRenderTree:41843.

stdout:

stderr:
ASSERTION FAILED: !animation.timeline() || animation.timeline() == this
./animation/AnimationTimeline.cpp(75) : virtual void WebCore::AnimationTimeline::removeAnimation(WebCore::WebAnimation &)
1   0x10ce41cb9 WTFCrash
2   0x121eb0a0b WTFCrashWithInfo(int, char const*, char const*, int)
3   0x1240b53d7 WebCore::AnimationTimeline::removeAnimation(WebCore::WebAnimation&)
4   0x1240be557 WebCore::DocumentTimeline::removeAnimation(WebCore::WebAnimation&)
5   0x1240bf088 WebCore::DocumentTimeline::removeReplacedAnimations()
6   0x1240be89a WebCore::DocumentTimeline::internalUpdateAnimationsAndSendEvents()
7   0x1240be60c WebCore::DocumentTimeline::updateAnimationsAndSendEvents(double)
8   0x1247214c6 WebCore::Document::updateAnimationsAndSendEvents(double)
9   0x12545f8d1 WebCore::Page::updateRendering()
10  0x1163b6966 -[WebView(WebPrivate) _viewWillDrawInternal]
11  0x1163de04c LayerFlushController::flushLayers()
12  0x1164d0d39 WebViewLayerFlushScheduler::layerFlushCallback()
13  0x1164d22c8 WebViewLayerFlushScheduler::WebViewLayerFlushScheduler(LayerFlushController*)::$_0::operator()() const
14  0x1164d2289 WTF::Detail::CallableWrapper<WebViewLayerFlushScheduler::WebViewLayerFlushScheduler(LayerFlushController*)::$_0, void>::call()
15  0x121ebe15a WTF::Function<void ()>::operator()() const
16  0x1256bbffc WebCore::RunLoopObserver::runLoopObserverFired()
17  0x1256bbf60 WebCore::RunLoopObserver::runLoopObserverFired(__CFRunLoopObserver*, unsigned long, void*)
18  0x7fff34ed2f28 __CFRUNLOOP_IS_CALLING_OUT_TO_AN_OBSERVER_CALLBACK_FUNCTION__
19  0x7fff34ed2e5d __CFRunLoopDoObservers
20  0x7fff34e756a7 __CFRunLoopRun
21  0x7fff34e74ebe CFRunLoopRunSpecific
22  0x10c18036b runTest(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&)
23  0x10c17f56a runTestingServerLoop()
24  0x10c17ec93 dumpRenderTree(int, char const**)
25  0x10c180d7d DumpRenderTreeMain(int, char const**)
26  0x10c209202 main
27  0x7fff60dcb3d5 start

[Antoine Quint]

The crash is specific to Debug builds and affects both WK1 and WK2.

[Antoine Quint]

Two of the subtests cause this crash: "Removes an animation after updating another animation's timeline" and "Removes an animation after updating its timeline".

[Aakash Jain]

This (In reply to Truitt Savell from comment #9)
> It looks like the changes in https://trac.webkit.org/changeset/250617/webkit caused this test for crash on debug:
> imported/w3c/web-platform-tests/web-animations/timing-model/timelines/update-and-send-events-replacement.html
This is slowing down macOS Debug WK1 EWS, can we either fix this soon, revert the commit or mark the test as failing.

[Antoine Quint]

The crash fix is covered by bug 202583 which has a patch up for review.