Bug 202109

Summary: [Web Animations] Coordinate "update animations and send events" procedure across multiple timelines
Product: WebKit Reporter: Antoine Quint <graouts>
Component: AnimationsAssignee: Antoine Quint <graouts>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, dino, esprehn+autocc, ews-watchlist, graouts, kangil.han, Lawrence.j, webkit-bot-watchers-bugzilla, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=211309
Attachments:
Description Flags
Patch
none
Patch dino: review+

Antoine Quint
Reported 2019-09-23 10:06:55 PDT
The test at imported/w3c/web-platform-tests/web-animations/timing-model/timelines/update-and-send-events.html is flaky.
Attachments
Patch (26.08 KB, patch)
2020-04-22 02:53 PDT, Antoine Quint 		no flags
DetailsFormatted DiffDiff
Patch (26.07 KB, patch)
2020-04-22 03:46 PDT, Antoine Quint 		dino: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Antoine Quint
Comment 1 2020-02-14 10:05:38 PST
This test is no longer flaky after fixing bug 207364 (r256619).
Ryan Haddad
Comment 3 2020-02-14 14:21:30 PST
<rdar://problem/59470821>
Ryan Haddad
Comment 4 2020-02-14 14:21:56 PST
*** Bug 207788 has been marked as a duplicate of this bug. ***
Ryan Haddad
Comment 5 2020-02-14 14:27:39 PST
Marked the test as flaky again in http://trac.webkit.org/r256645.
Alexey Proskuryakov
Comment 6 2020-02-14 22:46:18 PST
*** Bug 207790 has been marked as a duplicate of this bug. ***
Antoine Quint
Comment 7 2020-03-18 08:15:28 PDT
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
Comment 8 2020-03-18 08:21:09 PDT
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
Comment 9 2020-04-20 09:31:35 PDT
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
Comment 10 2020-04-22 02:38:56 PDT
Retitling to better capture the scope of the work to unflake this test.
Antoine Quint
Comment 11 2020-04-22 02:53:54 PDT
Created attachment 397182 [details] Patch
Antoine Quint
Comment 12 2020-04-22 03:46:59 PDT
Created attachment 397183 [details] Patch
Dean Jackson
Comment 13 2020-04-22 11:28:39 PDT
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
Comment 14 2020-04-22 11:44:42 PDT
(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
Comment 15 2020-04-22 11:47:44 PDT
Committed r260525: <https://trac.webkit.org/changeset/260525>
Daniel Bates
Comment 16 2020-05-01 12:15:45 PDT
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
Comment 17 2020-05-06 01:09:19 PDT
Committed r261218: <https://trac.webkit.org/changeset/261218>
Antoine Quint
Comment 18 2020-05-06 01:09:53 PDT
(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.
Note You need to log in before you can comment on or make changes to this bug.