Bug 202109 – [Web Animations] Coordinate "update animations and send events" procedure across multiple timelines

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

[Antoine Quint]

The test at imported/w3c/web-platform-tests/web-animations/timing-model/timelines/update-and-send-events.html is flaky.

[Antoine Quint]

This test is no longer flaky after fixing bug 207364 (r256619).

[Ryan Haddad]

(In reply to Antoine Quint from comment #1)
> This test is no longer flaky after fixing bug 207364 (r256619).
The test is not fixed, as reported in https://bugs.webkit.org/show_bug.cgi?id=207788

https://results.webkit.org/?suite=layout-tests&test=imported%2Fw3c%2Fweb-platform-tests%2Fweb-animations%2Ftiming-model%2Ftimelines%2Fupdate-and-send-events.html&platform=mac

[Ryan Haddad]

<rdar://problem/59470821>

[Ryan Haddad]

*** Bug 207788 has been marked as a duplicate of this bug. ***

[Ryan Haddad]

Marked the test as flaky again in http://trac.webkit.org/r256645.

[Alexey Proskuryakov]

*** Bug 207790 has been marked as a duplicate of this bug. ***

[Antoine Quint]

This is the error I'm seeing:

-FAIL All timelines are updated before running microtasks promise_test: Unhandled rejection with value: object "AbortError: The operation was aborted."
+TIMEOUT All timelines are updated before running microtasks Test timed out

[Antoine Quint]

If that failing test runs in isolation over many iterations, it never times out. However, if it's run with its preceding test, it eventually times out.

[Antoine Quint]

We can simply fix the last test to be a PASS and this will resolve the issue. To do this, we must make sure we run the "update animations and send events" procedure with all timelines in a coordinated fashion such that there is only a single micro task point.

[Antoine Quint]

Retitling to better capture the scope of the work to unflake this test.

[Antoine Quint]

Created attachment 397182 [details]
Patch

[Antoine Quint]

Created attachment 397183 [details]
Patch

[Dean Jackson]

Comment on attachment 397183 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        So far, although we did manage multiple animation timelines per document, we mostly operated under the assumption that there really was a single timeline.
> +        In this patch we make the "update animations and send events" procedure, which is central to the lifecycle of animations, work with multiple timelines such
> +        that a single microtask checkpoint is performed even with multiple timelines, whereas we would perform one per timeline before. To do this, we move much of
> +        the logic DocumentTimeline::updateAnimationsAndSendEvents() to DocumentTimelinesController where each step is run across each timeline, rather than running
> +        all steps for each timeline one after the other, respecting the single microtask checkpoint in the middle of the process.

Why are you wrapping the lines at such a high column? I think you want at least half this width.

> Source/WebCore/animation/DocumentTimelinesController.cpp:71
> +    Vector<RefPtr<DocumentTimeline>> protectedTimelines;
> +    for (auto& timeline : m_timelines)
> +        protectedTimelines.append(&timeline);

Vector<RefPtr<DocumentTimeline>> protectedTimelines { m_timelines };

[Antoine Quint]

(In reply to Dean Jackson from comment #13)
> Comment on attachment 397183 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=397183&action=review
> 
> > Source/WebCore/ChangeLog:13
> Why are you wrapping the lines at such a high column? I think you want at
> least half this width.

I don't know! I don't really understand why we have to wrap them in the first place.

> > Source/WebCore/animation/DocumentTimelinesController.cpp:71
> > +    Vector<RefPtr<DocumentTimeline>> protectedTimelines;
> > +    for (auto& timeline : m_timelines)
> > +        protectedTimelines.append(&timeline);
> 
> Vector<RefPtr<DocumentTimeline>> protectedTimelines { m_timelines };

Note that m_timelines is a WeakHashSet<DocumentTimeline>, so I don't think that can work.

[Antoine Quint]

Committed r260525: <https://trac.webkit.org/changeset/260525>

[Daniel Bates]

Comment on attachment 397183 [details]
Patch

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

> Source/WebCore/animation/AnimationTimeline.h:53
> +    AnimationCollection relevantAnimations() const { return m_animations; }

OK as-is. Optimal solution is to return const lvalue ref. See bug #211309.

> Source/WebCore/animation/AnimationTimeline.h:54
> +    Vector<WeakPtr<WebAnimation>> allAnimations() const { return m_allAnimations; }

This is unused? Can it be removed? If not then optimal solution is to return const lvalue ref.

[Antoine Quint]

Committed r261218: <https://trac.webkit.org/changeset/261218>

[Antoine Quint]

(In reply to Daniel Bates from comment #16)
> > Source/WebCore/animation/AnimationTimeline.h:54
> > +    Vector<WeakPtr<WebAnimation>> allAnimations() const { return m_allAnimations; }
> 
> This is unused? Can it be removed? If not then optimal solution is to return
> const lvalue ref.

It was removed in r261218.