Bug 209239 - REGRESSION(r257417): imported/w3c/web-platform-tests/web-animations/timing-model/timelines/update-and-send-events-replacement.html is flaky failing.
Summary: REGRESSION(r257417): imported/w3c/web-platform-tests/web-animations/timing-mo...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: WebKit Nightly Build
Hardware: Mac macOS 10.14
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on: 208069
Blocks:
  Show dependency treegraph
 
Reported: 2020-03-18 10:57 PDT by Jason Lawrence
Modified: 2020-03-22 15:57 PDT (History)
8 users (show)

See Also:


Attachments
Patch (7.82 KB, patch)
2020-03-21 14:11 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch for landing (7.75 KB, patch)
2020-03-22 15:28 PDT, Antoine Quint
no flags 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-03-18 10:57:40 PDT
imported/w3c/web-platform-tests/web-animations/timing-model/timelines/update-and-send-events-replacement.html 

Description:
This test is flaky failing on Mac. When looking at just the recent set of flaky failures, they started on 03/02/2020.

History:
https://results.webkit.org/?suite=layout-tests&test=imported%2Fw3c%2Fweb-platform-tests%2Fweb-animations%2Ftiming-model%2Ftimelines%2Fupdate-and-send-events-replacement.html&platform=mac&recent=false&flavor=wk2&flavor=wk1&limit=50000

Diff:
--- /Volumes/Data/slave/mojave-release-tests-wk1/build/layout-test-results/imported/w3c/web-platform-tests/web-animations/timing-model/timelines/update-and-send-events-replacement-expected.txt
+++ /Volumes/Data/slave/mojave-release-tests-wk1/build/layout-test-results/imported/w3c/web-platform-tests/web-animations/timing-model/timelines/update-and-send-events-replacement-actual.txt
@@ -11,7 +11,7 @@
 PASS Removes an animation after updating its fill mode 
 PASS Removes an animation after updating another animation's effect to one with different timing 
 PASS Removes an animation after updating its effect to one with different timing 
-PASS Removes an animation after updating another animation's timeline 
+FAIL Removes an animation after updating another animation's timeline assert_equals: expected "removed" but got "active"
 PASS Removes an animation after updating its timeline 
 PASS Removes an animation after updating another animation's effect's properties 
 PASS Removes an animation after updating its effect's properties
Comment 1 Radar WebKit Bug Importer 2020-03-18 10:58:08 PDT
<rdar://problem/60591358>
Comment 2 Jason Lawrence 2020-03-18 11:10:34 PDT
I have marked this test as failing while this issue is investigated.
https://trac.webkit.org/changeset/258650/webkit
Comment 3 Antoine Quint 2020-03-19 09:54:37 PDT
The failure can be reproduced like so:

rwt imported/w3c/web-platform-tests/web-animations/timing-model/timelines/update-and-send-events-replacement.html --iterations=5000 --exit-after-n-failures=1 --force -f

The test can be reduced to the single flaky failing test.
Comment 4 Antoine Quint 2020-03-19 10:54:09 PDT
When the test fails, the Document only ever sees a single timeline in m_timelines and thus fails to update the animation correctly.
Comment 5 Antoine Quint 2020-03-19 11:00:24 PDT
OK, this is due to r257417, the fix for bug 208069. In this test, we have the following:

  animB.timeline = new DocumentTimeline({
    originTime:
      document.timeline.currentTime - 100 * MS_PER_SEC - animB.startTime,
  });

But the DocumentTimeline is not referenced by anything other than the animation and so it gets deref'd, no longer being considered. We need to revisit bug 208069.
Comment 6 Antoine Quint 2020-03-21 14:11:10 PDT
Created attachment 394177 [details]
Patch
Comment 7 David Kilzer (:ddkilzer) 2020-03-21 22:52:54 PDT
Comment on attachment 394177 [details]
Patch

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

> Source/WebCore/ChangeLog:20
> +        WebAnimation a weak reference, in some cases, if CG kicks in, the timeline would be dereferenced and the test would fail. We restore that relationship

CG => GC
Comment 8 Antoine Quint 2020-03-22 15:28:34 PDT
Created attachment 394233 [details]
Patch for landing
Comment 9 EWS 2020-03-22 15:57:27 PDT
Committed r258823: <https://trac.webkit.org/changeset/258823>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 394233 [details].