Changeset 258823 in webkit

Timestamp:

Mar 22, 2020 3:57:22 PM (4 years ago)

Author:

commit-queue@webkit.org

Message:

[ Mac ] imported/w3c/web-platform-tests/web-animations/timing-model/timelines/update-and-send-events-replacement.html is flaky failing.
​https://bugs.webkit.org/show_bug.cgi?id=209239
<rdar://problem/60591358>

Patch by Antoine Quint <Antoine Quint> on 2020-03-22
Reviewed by Simon Fraser.

Source/WebCore:

This test was made flaky by r257417, the initial fix for webkit.org/b/208069. A new, appropriate fix for that bug is in the works. In the
meantime we revert r257417 in this patch.

The reason this test became flaky is that it features the following code:

animB.timeline = new DocumentTimeline({

originTime:

document.timeline.currentTime - 100 * MS_PER_SEC - animB.startTime,

});

In this case the only reference to the created DocumentTimeline is through animB.timeline. But because r257417 made the timeline reference from
WebAnimation a weak reference, in some cases, if GC kicks in, the timeline would be dereferenced and the test would fail. We restore that relationship
to its previous state, which is a strong reference.

• animation/WebAnimation.cpp:

(WebCore::WebAnimation::setTimeline):
(WebCore::WebAnimation::setTimelineInternal):
(WebCore::WebAnimation::enqueueAnimationEvent):
(WebCore::WebAnimation::acceleratedStateDidChange):
(WebCore::WebAnimation::timeline const): Deleted.

• animation/WebAnimation.h:

(WebCore::WebAnimation::timeline const):

LayoutTests:

• platform/mac/TestExpectations:

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/platform/mac/TestExpectations (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/animation/WebAnimation.cpp (modified) (7 diffs)
• Source/WebCore/animation/WebAnimation.h (modified) (2 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2020-03-22  Antoine Quint  <graouts@apple.com>
+
+        [ Mac ] imported/w3c/web-platform-tests/web-animations/timing-model/timelines/update-and-send-events-replacement.html is flaky failing.
+        https://bugs.webkit.org/show_bug.cgi?id=209239
+        <rdar://problem/60591358>
+
+        Reviewed by Simon Fraser.
+
+        * platform/mac/TestExpectations:
+
 2020-03-22  Diego Pino Garcia  <dpino@igalia.com>
 

trunk/LayoutTests/platform/mac/TestExpectations

@@ -2033 +2033 @@
 webkit.org/b/207150 platform/mac/webrtc/captureCanvas-webrtc-software-encoder.html [ Pass Failure ]
 
-webkit.org/b/209239 imported/w3c/web-platform-tests/web-animations/timing-model/timelines/update-and-send-events-replacement.html [ Pass Failure ]
-
 # The line breaking rules changed in ICU 66. We've updated the tests to match, but old platforms won't get updated line breaking rules.
 webkit.org/b/209250 [ Sierra+ ] imported/w3c/web-platform-tests/css/css-text/line-break/line-break-normal-015.xht [ ImageOnlyFailure ]

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2020-03-22  Antoine Quint  <graouts@apple.com>
+
+        [ Mac ] imported/w3c/web-platform-tests/web-animations/timing-model/timelines/update-and-send-events-replacement.html is flaky failing.
+        https://bugs.webkit.org/show_bug.cgi?id=209239
+        <rdar://problem/60591358>
+
+        Reviewed by Simon Fraser.
+
+        This test was made flaky by r257417, the initial fix for webkit.org/b/208069. A new, appropriate fix for that bug is in the works. In the
+        meantime we revert r257417 in this patch.
+
+        The reason this test became flaky is that it features the following code:
+
+            animB.timeline = new DocumentTimeline({
+              originTime:
+                document.timeline.currentTime - 100 * MS_PER_SEC - animB.startTime,
+            });
+
+        In this case the only reference to the created DocumentTimeline is through `animB.timeline`. But because r257417 made the timeline reference from
+        WebAnimation a weak reference, in some cases, if GC kicks in, the timeline would be dereferenced and the test would fail. We restore that relationship
+        to its previous state, which is a strong reference.
+
+        * animation/WebAnimation.cpp:
+        (WebCore::WebAnimation::setTimeline):
+        (WebCore::WebAnimation::setTimelineInternal):
+        (WebCore::WebAnimation::enqueueAnimationEvent):
+        (WebCore::WebAnimation::acceleratedStateDidChange):
+        (WebCore::WebAnimation::timeline const): Deleted.
+        * animation/WebAnimation.h:
+        (WebCore::WebAnimation::timeline const):
+
 2020-03-22  Zalan Bujtas  <zalan@apple.com>
 

trunk/Source/WebCore/animation/WebAnimation.cpp

@@ -190 +190 @@
 }
 
-AnimationTimeline* WebAnimation::timeline() const
-{
-    return m_timeline.get();
-}
-
 void WebAnimation::setEffectInternal(RefPtr<AnimationEffect>&& newEffect, bool doNotRemoveFromTimeline)
 {
@@ -233 +228 @@
 
     // 2. If new timeline is the same object as old timeline, abort this procedure.
-    if (timeline == m_timeline.get())
+    if (timeline == m_timeline)
         return;
 
@@ -258 +253 @@
     setTimelineInternal(WTFMove(timeline));
 
-    setSuspended(is<DocumentTimeline>(m_timeline.get()) && downcast<DocumentTimeline>(*m_timeline).animationsAreSuspended());
+    setSuspended(is<DocumentTimeline>(m_timeline) && downcast<DocumentTimeline>(*m_timeline).animationsAreSuspended());
 
     // 5. Run the procedure to update an animation's finished state for animation with the did seek flag set to false,
@@ -269 +264 @@
 void WebAnimation::setTimelineInternal(RefPtr<AnimationTimeline>&& timeline)
 {
-    if (m_timeline.get() == timeline)
+    if (m_timeline == timeline)
         return;
 
@@ -275 +270 @@
         m_timeline->removeAnimation(*this);
 
-    m_timeline = makeWeakPtr(timeline.get());
+    m_timeline = WTFMove(timeline);
 
     if (m_effect)
@@ -684 +679 @@
 void WebAnimation::enqueueAnimationEvent(Ref<AnimationEventBase>&& event)
 {
-    if (is<DocumentTimeline>(m_timeline.get())) {
+    if (is<DocumentTimeline>(m_timeline)) {
         // If animation has a document for timing, then append event to its document for timing's pending animation event queue along
         // with its target, animation. If animation is associated with an active timeline that defines a procedure to convert timeline times
@@ -1240 +1235 @@
 void WebAnimation::acceleratedStateDidChange()
 {
-    if (is<DocumentTimeline>(m_timeline.get()))
+    if (is<DocumentTimeline>(m_timeline))
         downcast<DocumentTimeline>(*m_timeline).animationAcceleratedRunningStateDidChange(*this);
 }

trunk/Source/WebCore/animation/WebAnimation.h

@@ -69 +69 @@
     AnimationEffect* effect() const { return m_effect.get(); }
     void setEffect(RefPtr<AnimationEffect>&&);
-    AnimationTimeline* timeline() const;
+    AnimationTimeline* timeline() const { return m_timeline.get(); }
     virtual void setTimeline(RefPtr<AnimationTimeline>&&);
 
@@ -187 +187 @@
 
     RefPtr<AnimationEffect> m_effect;
-    WeakPtr<AnimationTimeline> m_timeline;
+    RefPtr<AnimationTimeline> m_timeline;
     UniqueRef<ReadyPromise> m_readyPromise;
     UniqueRef<FinishedPromise> m_finishedPromise;