Bug 206746 - [Mac wk2 Release] imported/w3c/web-platform-tests/web-animations/timing-model/animations/updating-the-finished-state.html flaky fail
Summary: [Mac wk2 Release] imported/w3c/web-platform-tests/web-animations/timing-model...
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
Keywords: InRadar
Depends on:
Reported: 2020-01-24 07:45 PST by Jason Lawrence
Modified: 2020-03-11 13:04 PDT (History)
6 users (show)

See Also:

Patch (1.55 KB, patch)
2020-01-27 13:16 PST, Jason Lawrence
no flags Details | Formatted Diff | Diff
Update Test Expectations (1.59 KB, patch)
2020-01-27 13:34 PST, Jason Lawrence
no flags Details | Formatted Diff | Diff
Patch (3.87 KB, patch)
2020-03-11 11:08 PDT, Antoine Quint
dino: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jason Lawrence 2020-01-24 07:45:41 PST

Description:This Mac wk2 test is flaky failing.


--- /Volumes/Data/slave/mojave-release-tests-wk2/build/layout-test-results/imported/w3c/web-platform-tests/web-animations/timing-model/animations/updating-the-finished-state-expected.txt
+++ /Volumes/Data/slave/mojave-release-tests-wk2/build/layout-test-results/imported/w3c/web-platform-tests/web-animations/timing-model/animations/updating-the-finished-state-actual.txt
@@ -1,3 +1,5 @@
+Harness Error (TIMEOUT), message = null
 PASS Updating the finished state when playing past end 
 PASS Updating the finished state when seeking past end 
@@ -23,7 +25,7 @@
 PASS Animation finished promise is replaced after seeking back to start 
 PASS Animation finished promise is replaced after replaying from start 
 PASS Animation finish event is fired again after seeking back to start 
-PASS Animation finish event is fired again after replaying from start 
+TIMEOUT Animation finish event is fired again after replaying from start Test timed out
 PASS finish event is not fired at the end of the active interval when the endDelay has not expired 
 PASS finish event is fired after the endDelay has expired
Comment 1 Radar WebKit Bug Importer 2020-01-24 07:46:04 PST
Comment 2 Alexey Proskuryakov 2020-01-26 13:30:33 PST
Only timing out in release mode is a weird symptom! Why is Debug fine?
Comment 3 Jason Lawrence 2020-01-27 13:16:56 PST
Created attachment 388891 [details]
Comment 4 Jason Lawrence 2020-01-27 13:34:45 PST
Created attachment 388896 [details]
Update Test Expectations
Comment 5 Truitt Savell 2020-01-27 13:36:59 PST
Comment on attachment 388896 [details]
Update Test Expectations

Clearing flags on attachment: 388896

Committed r255168: <https://trac.webkit.org/changeset/255168>
Comment 6 Antoine Quint 2020-02-10 00:55:02 PST
Still happens on bots, not reproducing locally.
Comment 7 Antoine Quint 2020-03-11 09:59:45 PDT
When this test times out, the failure is that the second .onfinish function is not called.
Comment 8 Antoine Quint 2020-03-11 10:05:29 PDT
Increasing the test duration does not help.
Comment 9 Antoine Quint 2020-03-11 10:42:29 PDT
We get in a situation where the hold time is still resolved within the play() call and we return early, not scheduling any further update, and thus failing to ever advance that animation's time and finish it.
Comment 10 Antoine Quint 2020-03-11 10:47:49 PDT
This is the branch that we should get into but aren't inside WebAnimation::play():

`if (effectivePlaybackRate() > 0 && autoRewind == AutoRewind::Yes && (!localTime || localTime.value() < 0_s || localTime.value() >= endTime))`

I expect it'll be a rounding issue when comparing times, we should probably use `timeEpsilon`.
Comment 11 Antoine Quint 2020-03-11 10:56:40 PDT
That's it! In the bad case, these are the values we have:

    localTime = 0.000999999999999994s, endTime = 0.001000s

In the good case:

    localTime = 0.001s, endTime = 0.001000s

We just need to use timeEpsilon when comparing localTime and endTime.
Comment 12 Antoine Quint 2020-03-11 11:08:44 PDT
Created attachment 393268 [details]
Comment 13 Dean Jackson 2020-03-11 11:13:49 PDT
Comment on attachment 393268 [details]

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

> Source/WebCore/ChangeLog:12
> +        Because we could end up in situation where localTime was very marginally smaller than endTime inside of WebAnimation::play(), we would end up
> +        with an unresolved hold time and we would return before calling WebAnimation::timingDidChange() and thus scheduling an animation update from
> +        the timeline because we'd assume it was paused. As a result, the animation would never end and the test would wait for a "finish" event which
> +        would never come.

It's kind of weird to pick a line wrap value that is so large. Maybe you should rewrap to 80-100 chars?

> Source/WebCore/animation/WebAnimation.cpp:931
> +    if (effectivePlaybackRate() > 0 && autoRewind == AutoRewind::Yes && (!localTime || localTime.value() < 0_s || (localTime.value() + timeEpsilon) >= endTime)) {

Not now, but would it make sense to have helpers for nearly_equals or nearly_greater than? I think we have some helpers for that already (maybe with a better name).
Comment 14 Antoine Quint 2020-03-11 13:04:27 PDT
Committed r258274: <https://trac.webkit.org/changeset/258274>