WebAnimation should never prevent entering the back/forward cache
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 29 Oct 2019 23:07:16 +0000 (23:07 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 29 Oct 2019 23:07:16 +0000 (23:07 +0000)
https://bugs.webkit.org/show_bug.cgi?id=203088
<rdar://problem/56374249>

Patch by Antoine Quint <graouts@apple.com> on 2019-10-29
Reviewed by Antti Koivisto.

Source/WebCore:

Test: webanimations/animation-page-cache.html

We remove the Web Animation override of the deprecated method ActiveDOMObject::shouldPreventEnteringBackForwardCache_DEPRECATED()
and instead ensure event dispatch is suspended along with the WebAnimation object through the adoption of a SuspendableTaskQueue.

We also ensure an animation correctly suspends itself when ActiveDOMObject::suspend() and ActiveDOMObject::resume() are called.
Implementing these methods showed that we have some placeholders in DeclarativeAnimation that were not necessary, so we remove those.

Finally, we no longer need to track the stopped state since the SuspendableTaskQueue will close itself when ActiveDOMObject::stop()
is called.

* animation/DeclarativeAnimation.cpp:
(WebCore::DeclarativeAnimation::stop): Deleted.
(WebCore::DeclarativeAnimation::suspend): Deleted.
(WebCore::DeclarativeAnimation::resume): Deleted.
* animation/DeclarativeAnimation.h:
* animation/WebAnimation.cpp:
(WebCore::WebAnimation::WebAnimation):
(WebCore::WebAnimation::enqueueAnimationPlaybackEvent):
(WebCore::WebAnimation::suspend):
(WebCore::WebAnimation::resume):
(WebCore::WebAnimation::stop):
(WebCore::WebAnimation::hasPendingActivity):
(WebCore::WebAnimation::shouldPreventEnteringBackForwardCache_DEPRECATED const): Deleted.
* animation/WebAnimation.h:

LayoutTests:

Add a new test that checks that an Animation that would run past a page's navigation is correctly suspended
and resumed as it enters and leaves the back/forward cache.

* webanimations/animation-page-cache-expected.txt: Added.
* webanimations/animation-page-cache.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/webanimations/animation-page-cache-expected.txt [new file with mode: 0644]
LayoutTests/webanimations/animation-page-cache.html [new file with mode: 0644]
LayoutTests/webanimations/leak-document-with-web-animation-expected.txt
LayoutTests/webanimations/leak-document-with-web-animation.html
Source/WebCore/ChangeLog
Source/WebCore/animation/DeclarativeAnimation.cpp
Source/WebCore/animation/DeclarativeAnimation.h
Source/WebCore/animation/WebAnimation.cpp
Source/WebCore/animation/WebAnimation.h
Source/WebCore/platform/SuspendableTaskQueue.h

index fe0228b..a176947 100644 (file)
@@ -1,3 +1,17 @@
+2019-10-29  Antoine Quint  <graouts@apple.com>
+
+        WebAnimation should never prevent entering the back/forward cache
+        https://bugs.webkit.org/show_bug.cgi?id=203088
+        <rdar://problem/56374249>
+
+        Reviewed by Antti Koivisto.
+
+        Add a new test that checks that an Animation that would run past a page's navigation is correctly suspended
+        and resumed as it enters and leaves the back/forward cache.
+
+        * webanimations/animation-page-cache-expected.txt: Added.
+        * webanimations/animation-page-cache.html: Added.
+
 2019-10-29  Megan Gardner  <megan_gardner@apple.com>
 
         Update autocorrect test to have correctly focused contenteditable
diff --git a/LayoutTests/webanimations/animation-page-cache-expected.txt b/LayoutTests/webanimations/animation-page-cache-expected.txt
new file mode 100644 (file)
index 0000000..821037b
--- /dev/null
@@ -0,0 +1,2 @@
+This test verifies that the page cache suspends and resumes Web Animations from the page cache. The test starts an animation, then navigates away, waits a bit, navigates back, confirming that the timeline froze at a given time and resumed in the same spot, advancing the animation. If successful, it outputs 'PASS' below.
+PASS.
diff --git a/LayoutTests/webanimations/animation-page-cache.html b/LayoutTests/webanimations/animation-page-cache.html
new file mode 100644 (file)
index 0000000..b2657b4
--- /dev/null
@@ -0,0 +1,58 @@
+<!doctype html><!-- webkit-test-runner [ enableBackForwardCache=true ] -->
+<html>
+<script>
+
+let suspended = false;
+let restored = false;
+
+function finish(msg)
+{
+    document.getElementById("result").textContent = msg;
+    if (window.testRunner)
+        testRunner.notifyDone();
+}
+
+window.addEventListener("pagehide", event => {
+    suspended = event.persisted;
+    if (!suspended)
+        finish("FAIL: event.persisted was false for pagehide event.")
+});
+
+window.addEventListener("pageshow", event => {
+    // If we haven't been suspended, then this is the initial page load, not the back navigation.
+    if (!suspended)
+        return;
+
+    restored = event.persisted;
+    if (!restored)
+        finish("FAIL: event.persisted was false for pageshow event.")
+});
+
+window.addEventListener("DOMContentLoaded", event => {
+    if (window.testRunner) {
+        testRunner.dumpAsText();
+        testRunner.waitUntilDone();
+    }
+
+    // We start an animation that is just long enough that it would finish while the page is hidden.
+    document.body.animate({ backgroundColor: "red" }, 500).finished.then(() => {
+        if (!suspended)
+            finish("FAIL: Animation finished but prevented the page from being suspended.");
+        else if (!restored)
+            finish("FAIL: Animation finished but prevented the page from being restored.");
+        else
+            finish("PASS.");
+    });
+
+    requestAnimationFrame(() => {
+        // Load a new page, and let it go back after 250ms.
+        window.location.href = "data:text/html,<body onload='setTimeout(() => history.back(), 250)'></body>";
+    });
+});
+
+</script>
+<body>
+This test verifies that the page cache suspends and resumes Web Animations from the page cache. The test starts an animation, then navigates away, waits a bit, navigates back, confirming that the timeline froze at a given time and resumed in the same spot, advancing the animation. If successful, it outputs 'PASS' below.
+<div id="result"></div>
+</body>
+</html>
index 3c7c90c..823285b 100644 (file)
@@ -4,9 +4,7 @@ On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE
 
 
 The iframe has finished loading.
-The iframe has been destroyed.
-PASS internals.numberOfLiveDocuments() is numberOfLiveDocumentsAfterIframeLoaded - 1
-
+PASS The document was destroyed
 PASS successfullyParsed is true
 
 TEST COMPLETE
index 3ec9a5a..2055d71 100644 (file)
@@ -1,17 +1,13 @@
 <!DOCTYPE html>
 <html>
 <body onload="runTest()">
-<script src="../resources/js-test-pre.js"></script>
+<script src="../resources/js-test.js"></script>
 <script>
 description("This test asserts that Document doesn't leak when a Web Animation is created.");
 
 if (window.internals)
     jsTestIsAsync = true;
 
-gc();
-
-var numberOfLiveDocumentsAfterIframeLoaded = 0;
-
 function runTest() {
     if (!window.internals)
         return;
@@ -22,27 +18,35 @@ function runTest() {
         if (frame.src === 'about:blank')
             return true;
 
-        numberOfLiveDocumentsAfterIframeLoaded = internals.numberOfLiveDocuments();
+        documentIdentifier = internals.documentIdentifier(frame.contentDocument);
         debug("The iframe has finished loading.");
 
         frame.remove();
         frame = null;
 
-        setTimeout(() => {
-            gc();
-            setTimeout(function () {
-                debug("The iframe has been destroyed.");
-                shouldBe("internals.numberOfLiveDocuments()", "numberOfLiveDocumentsAfterIframeLoaded - 1");
-                debug("");
+        gc();
+        timeout = 0;
+        handle = setInterval(() => {
+            if (!internals.isDocumentAlive(documentIdentifier)) {
+                clearInterval(handle);
+                testPassed("The document was destroyed");
                 finishJSTest();
-            });
-        });
+                return;
+            }
+            timeout++;
+            if (timeout == 500) {
+                clearInterval(handle);
+                testFailed("The document was leaked");
+                finishJSTest();
+                return;
+            }
+            gc();
+        }, 10);
     }
 
     frame.src = 'resources/web-animation-leak-iframe.html';
 }
 
 </script>
-<script src="../resources/js-test-post.js"></script>
 </body>
-</html>
\ No newline at end of file
+</html>
index 2eff484..70b786f 100644 (file)
@@ -1,3 +1,37 @@
+2019-10-29  Antoine Quint  <graouts@apple.com>
+
+        WebAnimation should never prevent entering the back/forward cache
+        https://bugs.webkit.org/show_bug.cgi?id=203088
+        <rdar://problem/56374249>
+
+        Reviewed by Antti Koivisto.
+
+        Test: webanimations/animation-page-cache.html
+
+        We remove the Web Animation override of the deprecated method ActiveDOMObject::shouldPreventEnteringBackForwardCache_DEPRECATED()
+        and instead ensure event dispatch is suspended along with the WebAnimation object through the adoption of a SuspendableTaskQueue.
+
+        We also ensure an animation correctly suspends itself when ActiveDOMObject::suspend() and ActiveDOMObject::resume() are called.
+        Implementing these methods showed that we have some placeholders in DeclarativeAnimation that were not necessary, so we remove those.
+
+        Finally, we no longer need to track the stopped state since the SuspendableTaskQueue will close itself when ActiveDOMObject::stop()
+        is called.
+
+        * animation/DeclarativeAnimation.cpp:
+        (WebCore::DeclarativeAnimation::stop): Deleted.
+        (WebCore::DeclarativeAnimation::suspend): Deleted.
+        (WebCore::DeclarativeAnimation::resume): Deleted.
+        * animation/DeclarativeAnimation.h:
+        * animation/WebAnimation.cpp:
+        (WebCore::WebAnimation::WebAnimation):
+        (WebCore::WebAnimation::enqueueAnimationPlaybackEvent):
+        (WebCore::WebAnimation::suspend):
+        (WebCore::WebAnimation::resume):
+        (WebCore::WebAnimation::stop):
+        (WebCore::WebAnimation::hasPendingActivity):
+        (WebCore::WebAnimation::shouldPreventEnteringBackForwardCache_DEPRECATED const): Deleted.
+        * animation/WebAnimation.h:
+
 2019-10-07  Jer Noble  <jer.noble@apple.com>
 
         Implement the Remote Playback API.
index bb8ca84..6de6c34 100644 (file)
@@ -346,19 +346,4 @@ void DeclarativeAnimation::enqueueDOMEvent(const AtomString& eventType, Seconds
         m_eventQueue->enqueueEvent(TransitionEvent::create(eventType, downcast<CSSTransition>(this)->transitionProperty(), time, PseudoElement::pseudoElementNameForEvents(m_owningElement->pseudoId())));
 }
 
-void DeclarativeAnimation::stop()
-{
-    WebAnimation::stop();
-}
-
-void DeclarativeAnimation::suspend(ReasonForSuspension reason)
-{
-    WebAnimation::suspend(reason);
-}
-
-void DeclarativeAnimation::resume()
-{
-    WebAnimation::resume();
-}
-
 } // namespace WebCore
index 31d0cfb..c966e58 100644 (file)
@@ -81,11 +81,6 @@ private:
     void enqueueDOMEvent(const AtomString&, Seconds);
     void remove() final;
 
-    // ActiveDOMObject.
-    void suspend(ReasonForSuspension) final;
-    void resume() final;
-    void stop() final;
-
     bool m_wasPending { false };
     AnimationEffectPhase m_previousPhase { AnimationEffectPhase::Idle };
 
index 39f6b34..884ff4c 100644 (file)
@@ -67,6 +67,7 @@ WebAnimation::WebAnimation(Document& document)
     : ActiveDOMObject(document)
     , m_readyPromise(makeUniqueRef<ReadyPromise>(*this, &WebAnimation::readyPromiseResolve))
     , m_finishedPromise(makeUniqueRef<FinishedPromise>(*this, &WebAnimation::finishedPromiseResolve))
+    , m_taskQueue(SuspendableTaskQueue::create(document))
 {
     m_readyPromise->resolve(*this);
     suspendIfNeeded();
@@ -613,9 +614,8 @@ void WebAnimation::enqueueAnimationPlaybackEvent(const AtomString& type, Optiona
         downcast<DocumentTimeline>(*m_timeline).enqueueAnimationPlaybackEvent(WTFMove(event));
     } else {
         // Otherwise, queue a task to dispatch event at animation. The task source for this task is the DOM manipulation task source.
-        callOnMainThread([this, pendingActivity = makePendingActivity(*this), event = WTFMove(event)]() {
-            if (!m_isStopped)
-                this->dispatchEvent(event);
+        m_taskQueue->enqueueTask([this, event = WTFMove(event)] {
+            dispatchEvent(event);
         });
     }
 }
@@ -1157,26 +1157,27 @@ const char* WebAnimation::activeDOMObjectName() const
     return "Animation";
 }
 
-// FIXME: This should never prevent entering the back/forward cache.
-bool WebAnimation::shouldPreventEnteringBackForwardCache_DEPRECATED() const
+void WebAnimation::suspend(ReasonForSuspension)
 {
-    // Use the base class's implementation of hasPendingActivity() since we wouldn't want the custom implementation
-    // in this class designed to keep JS wrappers alive to interfere with the ability for a page using animations
-    // to enter the back/forward cache.
-    return ActiveDOMObject::hasPendingActivity();
+    setSuspended(true);
+}
+
+void WebAnimation::resume()
+{
+    setSuspended(false);
 }
 
 void WebAnimation::stop()
 {
+    m_taskQueue->cancelAllTasks();
     ActiveDOMObject::stop();
-    m_isStopped = true;
     removeAllEventListeners();
 }
 
 bool WebAnimation::hasPendingActivity() const
 {
     // Keep the JS wrapper alive if the animation is considered relevant or could become relevant again by virtue of having a timeline.
-    return m_timeline || m_isRelevant || ActiveDOMObject::hasPendingActivity();
+    return m_timeline || m_isRelevant || m_taskQueue->hasPendingTasks() || ActiveDOMObject::hasPendingActivity();
 }
 
 void WebAnimation::updateRelevance()
index d4159df..1c11dcf 100644 (file)
@@ -29,6 +29,7 @@
 #include "EventTarget.h"
 #include "ExceptionOr.h"
 #include "IDLTypes.h"
+#include "SuspendableTaskQueue.h"
 #include "WebAnimationUtilities.h"
 #include <wtf/Markable.h>
 #include <wtf/RefCounted.h>
@@ -140,8 +141,6 @@ public:
 protected:
     explicit WebAnimation(Document&);
 
-    void stop() override;
-
 private:
     enum class DidSeek : uint8_t { Yes, No };
     enum class SynchronouslyNotify : uint8_t { Yes, No };
@@ -176,6 +175,7 @@ private:
     RefPtr<AnimationTimeline> m_timeline;
     UniqueRef<ReadyPromise> m_readyPromise;
     UniqueRef<FinishedPromise> m_finishedPromise;
+    UniqueRef<SuspendableTaskQueue> m_taskQueue;
     Markable<Seconds, Seconds::MarkableTraits> m_previousCurrentTime;
     Markable<Seconds, Seconds::MarkableTraits> m_startTime;
     Markable<Seconds, Seconds::MarkableTraits> m_holdTime;
@@ -185,7 +185,6 @@ private:
 
     int m_suspendCount { 0 };
 
-    bool m_isStopped { false };
     bool m_isSuspended { false };
     bool m_finishNotificationStepsMicrotaskPending;
     bool m_isRelevant;
@@ -197,7 +196,9 @@ private:
 
     // ActiveDOMObject.
     const char* activeDOMObjectName() const final;
-    bool shouldPreventEnteringBackForwardCache_DEPRECATED() const final;
+    void suspend(ReasonForSuspension) final;
+    void resume() final;
+    void stop() final;
 
     // EventTarget
     EventTargetInterface eventTargetInterface() const final { return WebAnimationEventTargetInterfaceType; }
index b3260f4..5ba2cb0 100644 (file)
@@ -58,7 +58,7 @@ public:
     void close();
     bool isClosed() const { return m_isClosed; }
 
-    bool hasPendingTasks() const { return m_pendingTasks.isEmpty(); }
+    bool hasPendingTasks() const { return !m_pendingTasks.isEmpty(); }
 
 private:
     friend UniqueRef<SuspendableTaskQueue> WTF::makeUniqueRefWithoutFastMallocCheck<SuspendableTaskQueue, WebCore::ScriptExecutionContext*&>(WebCore::ScriptExecutionContext*&);