[Web Animations] DocumentTimeline::updateAnimations() is called endlessly
authorgraouts@webkit.org <graouts@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 21 Sep 2018 11:28:40 +0000 (11:28 +0000)
committergraouts@webkit.org <graouts@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 21 Sep 2018 11:28:40 +0000 (11:28 +0000)
https://bugs.webkit.org/show_bug.cgi?id=189784
<rdar://problem/41705679>

Reviewed by Dean Jackson.

Source/WebCore:

Test: webanimations/accelerated-animation-interruption-display-none.html

We have code that keeps queueing pending accelerated actions for an animation that does not have a renderer until it has one
so that we can deal with situations where animations are ready to commited before its composited renderer is available. This
code ended up running continuously when an element with an accelerated animation had its renderer removed without the animation
being removed itself, such as setting "display: none" on an element with an acceelerated CSS Animation targeting it.

We fix this by queueing up a "Stop" accelerated action when updating the accelerated state if there is no renderer for the current
animation target. Then, we no longer re-queue pending accelerated actions if the last queued operation is "Stop". This ensures that
we no longer queue actions endlessly when there is no longer a visible animation.

To test this, we add a new internals.numberOfAnimationTimelineInvalidations() method that indicates the number of times the current
document's animation timeline was invalidated.

* animation/DocumentTimeline.cpp:
(WebCore::DocumentTimeline::updateAnimations):
(WebCore::DocumentTimeline::numberOfAnimationTimelineInvalidationsForTesting const):
* animation/DocumentTimeline.h:
* animation/KeyframeEffectReadOnly.cpp:
(WebCore::KeyframeEffectReadOnly::updateAcceleratedAnimationState): If the animation target does not have a renderer and it's still
marked as running, enqueue a "Stop" accelerated action.
(WebCore::KeyframeEffectReadOnly::addPendingAcceleratedAction): If we enqueue a "Stop" accelerated action, remove any other queued
action so that we only process the "Stop" action, which would have superseded all previously queued actions anyway.
(WebCore::KeyframeEffectReadOnly::applyPendingAcceleratedActions): Only re-queue pending accelerated actions when a composited renderer
is not yet available if we don't have a "Stop" action queued.
* testing/Internals.cpp:
(WebCore::Internals::numberOfAnimationTimelineInvalidations const):
* testing/Internals.h:
* testing/Internals.idl:

LayoutTests:

Add a new test that checks that setting "display: none" on an element with an accelerated CSS animation on it
will no longer update the animation timeline.

* webanimations/accelerated-animation-interruption-display-none-expected.txt: Added.
* webanimations/accelerated-animation-interruption-display-none.html: Added.
* platform/win/TestExpectations:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@236308 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/platform/win/TestExpectations
LayoutTests/webanimations/accelerated-animation-interruption-display-none-expected.txt [new file with mode: 0644]
LayoutTests/webanimations/accelerated-animation-interruption-display-none.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/animation/DocumentTimeline.cpp
Source/WebCore/animation/DocumentTimeline.h
Source/WebCore/animation/KeyframeEffectReadOnly.cpp
Source/WebCore/testing/Internals.cpp
Source/WebCore/testing/Internals.h
Source/WebCore/testing/Internals.idl

index 09493bf..6b6c9c6 100644 (file)
@@ -1,3 +1,18 @@
+2018-09-21  Antoine Quint  <graouts@apple.com>
+
+        [Web Animations] DocumentTimeline::updateAnimations() is called endlessly
+        https://bugs.webkit.org/show_bug.cgi?id=189784
+        <rdar://problem/41705679>
+
+        Reviewed by Dean Jackson.
+
+        Add a new test that checks that setting "display: none" on an element with an accelerated CSS animation on it
+        will no longer update the animation timeline.
+
+        * webanimations/accelerated-animation-interruption-display-none-expected.txt: Added.
+        * webanimations/accelerated-animation-interruption-display-none.html: Added.
+        * platform/win/TestExpectations:
+
 2018-09-20  Dean Jackson  <dino@apple.com>
 
         Restrict the total combined size of backdrop filters
 2018-09-20  Dean Jackson  <dino@apple.com>
 
         Restrict the total combined size of backdrop filters
index ce61950..309d3dc 100644 (file)
@@ -4144,6 +4144,7 @@ webkit.org/b/188166 webanimations/setting-css-animation-none-after-clearing-effe
 webkit.org/b/188166 webanimations/setting-css-animation-timing-property-via-style-after-clearing-effect.html [ Failure ]
 
 webkit.org/b/189468 webanimations/accelerated-transition-interrupted-on-composited-element.html [ Skip ]
 webkit.org/b/188166 webanimations/setting-css-animation-timing-property-via-style-after-clearing-effect.html [ Failure ]
 
 webkit.org/b/189468 webanimations/accelerated-transition-interrupted-on-composited-element.html [ Skip ]
+webkit.org/b/189825 webanimations/accelerated-animation-interruption-display-none.html [ Skip ]
 
 webkit.org/b/188167 fast/repaint/canvas-object-fit.html [ Failure ]
 
 
 webkit.org/b/188167 fast/repaint/canvas-object-fit.html [ Failure ]
 
diff --git a/LayoutTests/webanimations/accelerated-animation-interruption-display-none-expected.txt b/LayoutTests/webanimations/accelerated-animation-interruption-display-none-expected.txt
new file mode 100644 (file)
index 0000000..270ab7c
--- /dev/null
@@ -0,0 +1,3 @@
+
+PASS Interrupting an animation by setting 'display: none' should stop invalidating the animation timeline. 
+
diff --git a/LayoutTests/webanimations/accelerated-animation-interruption-display-none.html b/LayoutTests/webanimations/accelerated-animation-interruption-display-none.html
new file mode 100644 (file)
index 0000000..9c7a13b
--- /dev/null
@@ -0,0 +1,57 @@
+<!DOCTYPE html><!-- webkit-test-runner [ experimental:WebAnimationsCSSIntegrationEnabled=true ] -->
+<html>
+<head>
+<style>
+
+#target {
+    width: 100px;
+    height: 100px;
+    background-color: black;
+    animation: foo linear 10s;
+}
+
+@keyframes foo {
+    from { transform: "translateX(100px)" };
+    to { transform: "none" };
+}
+
+</style>
+</head>
+<body>
+<script src="../resources/testharness.js"></script>
+<script src="../resources/testharnessreport.js"></script>
+<div id="target"></div>
+
+<script>
+
+function waitNFrames(numberOfFrames, continuation)
+{
+    let elapsedFrames = 0;
+    (function rAFCallback() {
+        if (elapsedFrames++ >= numberOfFrames)
+            continuation();
+        else
+            requestAnimationFrame(rAFCallback);
+    })();
+}
+
+async_test(t => {
+    const initialNumberOfTimelineInvalidations = internals.numberOfAnimationTimelineInvalidations();
+    waitNFrames(3, () => {
+        assert_greater_than(internals.numberOfAnimationTimelineInvalidations(), initialNumberOfTimelineInvalidations, "There should be updates made to the timeline as an animation is set up.");
+
+        document.getElementById("target").style.display = "none";
+
+        waitNFrames(2, () => {
+            const numberOfTimelineInvalidationsAfterInterruption = internals.numberOfAnimationTimelineInvalidations();
+            requestAnimationFrame(() => {
+                assert_equals(internals.numberOfAnimationTimelineInvalidations(), numberOfTimelineInvalidationsAfterInterruption, "There should not be any updates made to the timeline after interrupting the single running animation.");
+                t.done();
+            });
+        });
+    });
+}, "Interrupting an animation by setting 'display: none' should stop invalidating the animation timeline.");
+
+</script>
+</body>
+</html>
index cedea6a..453b218 100644 (file)
@@ -1,3 +1,41 @@
+2018-09-20  Antoine Quint  <graouts@apple.com>
+
+        [Web Animations] DocumentTimeline::updateAnimations() is called endlessly
+        https://bugs.webkit.org/show_bug.cgi?id=189784
+        <rdar://problem/41705679>
+
+        Reviewed by Dean Jackson.
+
+        Test: webanimations/accelerated-animation-interruption-display-none.html
+
+        We have code that keeps queueing pending accelerated actions for an animation that does not have a renderer until it has one
+        so that we can deal with situations where animations are ready to commited before its composited renderer is available. This
+        code ended up running continuously when an element with an accelerated animation had its renderer removed without the animation
+        being removed itself, such as setting "display: none" on an element with an acceelerated CSS Animation targeting it.
+
+        We fix this by queueing up a "Stop" accelerated action when updating the accelerated state if there is no renderer for the current
+        animation target. Then, we no longer re-queue pending accelerated actions if the last queued operation is "Stop". This ensures that
+        we no longer queue actions endlessly when there is no longer a visible animation.
+
+        To test this, we add a new internals.numberOfAnimationTimelineInvalidations() method that indicates the number of times the current
+        document's animation timeline was invalidated.
+
+        * animation/DocumentTimeline.cpp:
+        (WebCore::DocumentTimeline::updateAnimations):
+        (WebCore::DocumentTimeline::numberOfAnimationTimelineInvalidationsForTesting const):
+        * animation/DocumentTimeline.h:
+        * animation/KeyframeEffectReadOnly.cpp:
+        (WebCore::KeyframeEffectReadOnly::updateAcceleratedAnimationState): If the animation target does not have a renderer and it's still
+        marked as running, enqueue a "Stop" accelerated action.
+        (WebCore::KeyframeEffectReadOnly::addPendingAcceleratedAction): If we enqueue a "Stop" accelerated action, remove any other queued
+        action so that we only process the "Stop" action, which would have superseded all previously queued actions anyway.
+        (WebCore::KeyframeEffectReadOnly::applyPendingAcceleratedActions): Only re-queue pending accelerated actions when a composited renderer
+        is not yet available if we don't have a "Stop" action queued.
+        * testing/Internals.cpp:
+        (WebCore::Internals::numberOfAnimationTimelineInvalidations const):
+        * testing/Internals.h:
+        * testing/Internals.idl:
+
 2018-09-21  Yacine Bandou  <yacine.bandou@softathome.com>
 
         [EME] Fix typo in WebM sanitization variable
 2018-09-21  Yacine Bandou  <yacine.bandou@softathome.com>
 
         [EME] Fix typo in WebM sanitization variable
index 772b491..3c13948 100644 (file)
@@ -291,6 +291,8 @@ void DocumentTimeline::animationResolutionTimerFired()
 
 void DocumentTimeline::updateAnimations()
 {
 
 void DocumentTimeline::updateAnimations()
 {
+    m_numberOfAnimationTimelineInvalidationsForTesting++;
+
     for (const auto& animation : animations())
         animation->runPendingTasks();
 
     for (const auto& animation : animations())
         animation->runPendingTasks();
 
@@ -499,4 +501,9 @@ Vector<std::pair<String, double>> DocumentTimeline::acceleratedAnimationsForElem
     return { };
 }
 
     return { };
 }
 
+unsigned DocumentTimeline::numberOfAnimationTimelineInvalidationsForTesting() const
+{
+    return m_numberOfAnimationTimelineInvalidationsForTesting;
+}
+
 } // namespace WebCore
 } // namespace WebCore
index 9e336f2..e555b15 100644 (file)
@@ -76,6 +76,7 @@ public:
     WEBCORE_EXPORT bool animationsAreSuspended();
     WEBCORE_EXPORT unsigned numberOfActiveAnimationsForTesting() const;
     WEBCORE_EXPORT Vector<std::pair<String, double>> acceleratedAnimationsForElement(Element&) const;    
     WEBCORE_EXPORT bool animationsAreSuspended();
     WEBCORE_EXPORT unsigned numberOfActiveAnimationsForTesting() const;
     WEBCORE_EXPORT Vector<std::pair<String, double>> acceleratedAnimationsForElement(Element&) const;    
+    WEBCORE_EXPORT unsigned numberOfAnimationTimelineInvalidationsForTesting() const;
 
 private:
     DocumentTimeline(Document&, Seconds);
 
 private:
     DocumentTimeline(Document&, Seconds);
@@ -101,6 +102,7 @@ private:
     Timer m_animationScheduleTimer;
     HashSet<RefPtr<WebAnimation>> m_acceleratedAnimationsPendingRunningStateChange;
     Vector<Ref<AnimationPlaybackEvent>> m_pendingAnimationEvents;
     Timer m_animationScheduleTimer;
     HashSet<RefPtr<WebAnimation>> m_acceleratedAnimationsPendingRunningStateChange;
     Vector<Ref<AnimationPlaybackEvent>> m_pendingAnimationEvents;
+    unsigned m_numberOfAnimationTimelineInvalidationsForTesting { 0 };
 
 #if !USE(REQUEST_ANIMATION_FRAME_DISPLAY_MONITOR)
     void animationResolutionTimerFired();
 
 #if !USE(REQUEST_ANIMATION_FRAME_DISPLAY_MONITOR)
     void animationResolutionTimerFired();
index 0fe00d8..fa387ef 100644 (file)
@@ -1199,8 +1199,11 @@ void KeyframeEffectReadOnly::updateAcceleratedAnimationState()
     if (!m_shouldRunAccelerated)
         return;
 
     if (!m_shouldRunAccelerated)
         return;
 
-    if (!renderer())
+    if (!renderer()) {
+        if (isRunningAccelerated())
+            addPendingAcceleratedAction(AcceleratedAction::Stop);
         return;
         return;
+    }
 
     auto localTime = animation()->currentTime();
 
 
     auto localTime = animation()->currentTime();
 
@@ -1225,6 +1228,10 @@ void KeyframeEffectReadOnly::updateAcceleratedAnimationState()
     if (playState == WebAnimation::PlayState::Finished) {
         if (isRunningAccelerated())
             addPendingAcceleratedAction(AcceleratedAction::Stop);
     if (playState == WebAnimation::PlayState::Finished) {
         if (isRunningAccelerated())
             addPendingAcceleratedAction(AcceleratedAction::Stop);
+        else {
+            m_lastRecordedAcceleratedAction = AcceleratedAction::Stop;
+            m_pendingAcceleratedActions.clear();
+        }
         return;
     }
 
         return;
     }
 
@@ -1237,6 +1244,8 @@ void KeyframeEffectReadOnly::updateAcceleratedAnimationState()
 
 void KeyframeEffectReadOnly::addPendingAcceleratedAction(AcceleratedAction action)
 {
 
 void KeyframeEffectReadOnly::addPendingAcceleratedAction(AcceleratedAction action)
 {
+    if (action == AcceleratedAction::Stop)
+        m_pendingAcceleratedActions.clear();
     m_pendingAcceleratedActions.append(action);
     if (action != AcceleratedAction::Seek)
         m_lastRecordedAcceleratedAction = action;
     m_pendingAcceleratedActions.append(action);
     if (action != AcceleratedAction::Seek)
         m_lastRecordedAcceleratedAction = action;
@@ -1269,7 +1278,8 @@ void KeyframeEffectReadOnly::applyPendingAcceleratedActions()
 
     auto* renderer = this->renderer();
     if (!renderer || !renderer->isComposited()) {
 
     auto* renderer = this->renderer();
     if (!renderer || !renderer->isComposited()) {
-        animation()->acceleratedStateDidChange();
+        if (m_lastRecordedAcceleratedAction != AcceleratedAction::Stop || m_pendingAcceleratedActions.last() != AcceleratedAction::Stop)
+            animation()->acceleratedStateDidChange();
         return;
     }
 
         return;
     }
 
index 56b5e25..8685487 100644 (file)
@@ -1076,6 +1076,13 @@ Vector<Internals::AcceleratedAnimation> Internals::acceleratedAnimationsForEleme
     return animations;
 }
 
     return animations;
 }
 
+unsigned Internals::numberOfAnimationTimelineInvalidations() const
+{
+    if (RuntimeEnabledFeatures::sharedFeatures().webAnimationsCSSIntegrationEnabled())
+        return frame()->document()->timeline().numberOfAnimationTimelineInvalidationsForTesting();
+    return 0;
+}
+
 ExceptionOr<RefPtr<Element>> Internals::pseudoElement(Element& element, const String& pseudoId)
 {
     if (pseudoId != "before" && pseudoId != "after")
 ExceptionOr<RefPtr<Element>> Internals::pseudoElement(Element& element, const String& pseudoId)
 {
     if (pseudoId != "before" && pseudoId != "after")
index 4407d1e..8dd3167 100644 (file)
@@ -206,6 +206,7 @@ public:
         double speed;
     };
     Vector<AcceleratedAnimation> acceleratedAnimationsForElement(Element&);
         double speed;
     };
     Vector<AcceleratedAnimation> acceleratedAnimationsForElement(Element&);
+    unsigned numberOfAnimationTimelineInvalidations() const;
 
     // For animations testing, we need a way to get at pseudo elements.
     ExceptionOr<RefPtr<Element>> pseudoElement(Element&, const String&);
 
     // For animations testing, we need a way to get at pseudo elements.
     ExceptionOr<RefPtr<Element>> pseudoElement(Element&, const String&);
index 55fabf4..9445718 100644 (file)
@@ -211,6 +211,7 @@ enum CompositingPolicy {
 
     // Web Animations testing.
     sequence<AcceleratedAnimation> acceleratedAnimationsForElement(Element element);
 
     // Web Animations testing.
     sequence<AcceleratedAnimation> acceleratedAnimationsForElement(Element element);
+    unsigned long numberOfAnimationTimelineInvalidations();
 
     // For animations testing, we need a way to get at pseudo elements.
     [MayThrowException] Element? pseudoElement(Element element, DOMString pseudoId);
 
     // For animations testing, we need a way to get at pseudo elements.
     [MayThrowException] Element? pseudoElement(Element element, DOMString pseudoId);