Bug 206746 – [Mac wk2 Release] imported/w3c/web-platform-tests/web-animations/timing-model/animations/updating-the-finished-state.html flaky fail

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

[Jason Lawrence]

imported/w3c/web-platform-tests/web-animations/timing-model/animations/updating-the-finished-state.html

Description:This Mac wk2 test is flaky failing.


History:
https://results.webkit.org/?suite=layout-tests&test=imported%2Fw3c%2Fweb-platform-tests%2Fweb-animations%2Ftiming-model%2Fanimations%2Fupdating-the-finished-state.html

Diff:
--- /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

[Radar WebKit Bug Importer]

<rdar://problem/58867580>

[Alexey Proskuryakov]

Only timing out in release mode is a weird symptom! Why is Debug fine?

[Jason Lawrence]

Created attachment 388891 [details]
Patch

[Jason Lawrence]

Created attachment 388896 [details]
Update Test Expectations

[Truitt Savell]

Comment on attachment 388896 [details]
Update Test Expectations

Clearing flags on attachment: 388896

Committed r255168: <https://trac.webkit.org/changeset/255168>

[Antoine Quint]

Still happens on bots, not reproducing locally.

[Antoine Quint]

When this test times out, the failure is that the second .onfinish function is not called.

[Antoine Quint]

Increasing the test duration does not help.

[Antoine Quint]

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.

[Antoine Quint]

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`.

[Antoine Quint]

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.

[Antoine Quint]

Created attachment 393268 [details]
Patch

[Dean Jackson]

Comment on attachment 393268 [details]
Patch

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).

[Antoine Quint]

Committed r258274: <https://trac.webkit.org/changeset/258274>