[Web Animations] Using a Web Animation leaks the Document
authorgraouts@webkit.org <graouts@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 29 Jun 2018 05:45:34 +0000 (05:45 +0000)
committergraouts@webkit.org <graouts@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 29 Jun 2018 05:45:34 +0000 (05:45 +0000)
https://bugs.webkit.org/show_bug.cgi?id=187088
<rdar://problem/41392046>

Reviewed by Dean Jackson.

Source/WebCore:

Test: webanimations/leak-document-with-web-animation.html

We need to ensure that any remaining animation is cleared when the DocumentTimeline is detached from its Document.
We rename WebAnimation::prepareAnimationForRemoval() to WebAnimation::remove() since it really actively disassociates
the animation from its timeline.

* animation/AnimationTimeline.cpp:
(WebCore::AnimationTimeline::removeAnimationsForElement): We no longer need the call to removeAnimation()
since the new WebAnimation::remove() method will also set the timeline to null which will eventually call
removeAnimation() on the disassociated timeline.
* animation/DeclarativeAnimation.cpp:
(WebCore::DeclarativeAnimation::remove):
(WebCore::DeclarativeAnimation::prepareAnimationForRemoval): Deleted.
* animation/DeclarativeAnimation.h:
* animation/DocumentTimeline.cpp:
(WebCore::DocumentTimeline::detachFromDocument): Call remove() on all known animations.
* animation/WebAnimation.cpp:
(WebCore::WebAnimation::remove): Set the timeline to null to fully disassociate this animation from its timeline.
(WebCore::WebAnimation::setTimeline): Factor the internal timeline-association code out of this JS API method so
that we can call this code without any JS-facing implications.
(WebCore::WebAnimation::setTimelineInternal):
(WebCore::WebAnimation::prepareAnimationForRemoval): Deleted.
* animation/WebAnimation.h:

LayoutTests:

Add a new test that creates an Animation object in JS within an iframe and checks that removing
the iframe clears its Document.

* webanimations/leak-document-with-web-animation-expected.txt: Added.
* webanimations/leak-document-with-web-animation.html: Added.
* webanimations/resources/web-animation-leak-iframe.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/webanimations/leak-document-with-web-animation-expected.txt [new file with mode: 0644]
LayoutTests/webanimations/leak-document-with-web-animation.html [new file with mode: 0644]
LayoutTests/webanimations/resources/web-animation-leak-iframe.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/animation/AnimationTimeline.cpp
Source/WebCore/animation/DeclarativeAnimation.cpp
Source/WebCore/animation/DeclarativeAnimation.h
Source/WebCore/animation/DocumentTimeline.cpp
Source/WebCore/animation/WebAnimation.cpp
Source/WebCore/animation/WebAnimation.h

index 73fc6c7..a952e40 100644 (file)
@@ -1,3 +1,18 @@
+2018-06-27  Antoine Quint  <graouts@apple.com>
+
+        [Web Animations] Using a Web Animation leaks the Document
+        https://bugs.webkit.org/show_bug.cgi?id=187088
+        <rdar://problem/41392046>
+
+        Reviewed by Dean Jackson.
+
+        Add a new test that creates an Animation object in JS within an iframe and checks that removing
+        the iframe clears its Document. 
+
+        * webanimations/leak-document-with-web-animation-expected.txt: Added.
+        * webanimations/leak-document-with-web-animation.html: Added.
+        * webanimations/resources/web-animation-leak-iframe.html: Added.
+
 2018-06-28  Olivia Barnett  <obarnett@apple.com>
         
         Find in page for typographic quotes does not find low (German) quotes
diff --git a/LayoutTests/webanimations/leak-document-with-web-animation-expected.txt b/LayoutTests/webanimations/leak-document-with-web-animation-expected.txt
new file mode 100644 (file)
index 0000000..3c7c90c
--- /dev/null
@@ -0,0 +1,13 @@
+This test asserts that Document doesn't leak when a Web Animation is created.
+
+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 successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/webanimations/leak-document-with-web-animation.html b/LayoutTests/webanimations/leak-document-with-web-animation.html
new file mode 100644 (file)
index 0000000..3ec9a5a
--- /dev/null
@@ -0,0 +1,48 @@
+<!DOCTYPE html>
+<html>
+<body onload="runTest()">
+<script src="../resources/js-test-pre.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;
+
+    var frame = document.body.appendChild(document.createElement("iframe"));
+
+    frame.onload = function() {
+        if (frame.src === 'about:blank')
+            return true;
+
+        numberOfLiveDocumentsAfterIframeLoaded = internals.numberOfLiveDocuments();
+        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("");
+                finishJSTest();
+            });
+        });
+    }
+
+    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
diff --git a/LayoutTests/webanimations/resources/web-animation-leak-iframe.html b/LayoutTests/webanimations/resources/web-animation-leak-iframe.html
new file mode 100644 (file)
index 0000000..63f269f
--- /dev/null
@@ -0,0 +1,4 @@
+<div></div>
+<script type="text/javascript">
+document.querySelector("div").animate({ marginLeft: ["0", "100px"] }, 1000);
+</script>
index dcca264..aeee5a8 100644 (file)
@@ -1,3 +1,35 @@
+2018-06-27  Antoine Quint  <graouts@apple.com>
+
+        [Web Animations] Using a Web Animation leaks the Document
+        https://bugs.webkit.org/show_bug.cgi?id=187088
+        <rdar://problem/41392046>
+
+        Reviewed by Dean Jackson.
+
+        Test: webanimations/leak-document-with-web-animation.html
+
+        We need to ensure that any remaining animation is cleared when the DocumentTimeline is detached from its Document.
+        We rename WebAnimation::prepareAnimationForRemoval() to WebAnimation::remove() since it really actively disassociates
+        the animation from its timeline.
+
+        * animation/AnimationTimeline.cpp:
+        (WebCore::AnimationTimeline::removeAnimationsForElement): We no longer need the call to removeAnimation()
+        since the new WebAnimation::remove() method will also set the timeline to null which will eventually call
+        removeAnimation() on the disassociated timeline.
+        * animation/DeclarativeAnimation.cpp:
+        (WebCore::DeclarativeAnimation::remove):
+        (WebCore::DeclarativeAnimation::prepareAnimationForRemoval): Deleted.
+        * animation/DeclarativeAnimation.h:
+        * animation/DocumentTimeline.cpp:
+        (WebCore::DocumentTimeline::detachFromDocument): Call remove() on all known animations.
+        * animation/WebAnimation.cpp:
+        (WebCore::WebAnimation::remove): Set the timeline to null to fully disassociate this animation from its timeline.
+        (WebCore::WebAnimation::setTimeline): Factor the internal timeline-association code out of this JS API method so
+        that we can call this code without any JS-facing implications.
+        (WebCore::WebAnimation::setTimelineInternal):
+        (WebCore::WebAnimation::prepareAnimationForRemoval): Deleted.
+        * animation/WebAnimation.h:
+
 2018-06-28  Zalan Bujtas  <zalan@apple.com>
 
         [LFC] Out-of-flow positioned height does not necessarily equal to "bottom - top".
index ad1e436..bdf44ad 100644 (file)
@@ -176,10 +176,8 @@ Vector<RefPtr<WebAnimation>> AnimationTimeline::animationsForElement(Element& el
 
 void AnimationTimeline::removeAnimationsForElement(Element& element)
 {
-    for (auto& animation : animationsForElement(element)) {
-        animation->prepareAnimationForRemoval();
-        removeAnimation(animation.releaseNonNull());
-    }
+    for (auto& animation : animationsForElement(element))
+        animation->remove();
 }
 
 void AnimationTimeline::cancelDeclarativeAnimationsForElement(Element& element)
index 340c04e..b32b478 100644 (file)
@@ -49,10 +49,10 @@ DeclarativeAnimation::~DeclarativeAnimation()
 {
 }
 
-void DeclarativeAnimation::prepareAnimationForRemoval()
+void DeclarativeAnimation::remove()
 {
-    WebAnimation::prepareAnimationForRemoval();
     m_eventQueue.close();
+    WebAnimation::remove();
 }
 
 void DeclarativeAnimation::setBackingAnimation(const Animation& backingAnimation)
index a9bec97..b12f35d 100644 (file)
@@ -45,10 +45,10 @@ public:
     const Animation& backingAnimation() const { return m_backingAnimation; }
     void setBackingAnimation(const Animation&);
     void invalidateDOMEvents(Seconds elapsedTime = 0_s);
-    void prepareAnimationForRemoval() final;
 
     void setTimeline(RefPtr<AnimationTimeline>&&) final;
     void cancel() final;
+    void remove() final;
 
 protected:
     DeclarativeAnimation(Element&, const Animation&);
index 3f08837..2dd5676 100644 (file)
@@ -63,6 +63,10 @@ void DocumentTimeline::detachFromDocument()
     m_invalidationTaskQueue.close();
     m_eventDispatchTaskQueue.close();
     m_animationScheduleTimer.stop();
+
+    for (const auto& animation : animations())
+        animation->remove();
+
     m_document = nullptr;
 }
 
index d0b6f3b..01682f2 100644 (file)
@@ -70,9 +70,10 @@ WebAnimation::~WebAnimation()
 {
 }
 
-void WebAnimation::prepareAnimationForRemoval()
+void WebAnimation::remove()
 {
     setEffectInternal(nullptr);
+    setTimelineInternal(nullptr);
 }
 
 void WebAnimation::suspendEffectInvalidation()
@@ -177,12 +178,6 @@ void WebAnimation::setTimeline(RefPtr<AnimationTimeline>&& timeline)
     if (m_startTime)
         setHoldTime(std::nullopt);
 
-    if (m_timeline)
-        m_timeline->removeAnimation(*this);
-
-    if (timeline)
-        timeline->addAnimation(*this);
-
     if (is<KeyframeEffectReadOnly>(m_effect)) {
         auto* keyframeEffect = downcast<KeyframeEffectReadOnly>(m_effect.get());
         auto* target = keyframeEffect->target();
@@ -197,7 +192,7 @@ void WebAnimation::setTimeline(RefPtr<AnimationTimeline>&& timeline)
         }
     }
 
-    m_timeline = WTFMove(timeline);
+    setTimelineInternal(WTFMove(timeline));
 
     setSuspended(is<DocumentTimeline>(m_timeline) && downcast<DocumentTimeline>(*m_timeline).animationsAreSuspended());
 
@@ -208,6 +203,17 @@ void WebAnimation::setTimeline(RefPtr<AnimationTimeline>&& timeline)
     updateFinishedState(DidSeek::No, SynchronouslyNotify::No);
 }
 
+void WebAnimation::setTimelineInternal(RefPtr<AnimationTimeline>&& timeline)
+{
+    if (m_timeline)
+        m_timeline->removeAnimation(*this);
+
+    if (timeline)
+        timeline->addAnimation(*this);
+
+    m_timeline = WTFMove(timeline);
+}
+
 void WebAnimation::effectTargetDidChange(Element* previousTarget, Element* newTarget)
 {
     if (!m_timeline)
index bb6393e..13b3e39 100644 (file)
@@ -63,7 +63,6 @@ public:
 
     AnimationEffectReadOnly* effect() const { return m_effect.get(); }
     void setEffect(RefPtr<AnimationEffectReadOnly>&&);
-    void setEffectInternal(RefPtr<AnimationEffectReadOnly>&&, bool = false);
     AnimationTimeline* timeline() const { return m_timeline.get(); }
     virtual void setTimeline(RefPtr<AnimationTimeline>&&);
 
@@ -117,7 +116,7 @@ public:
     void unsuspendEffectInvalidation();
     void setSuspended(bool);
     bool isSuspended() const { return m_isSuspended; }
-    virtual void prepareAnimationForRemoval();
+    virtual void remove();
 
     String description();
 
@@ -156,7 +155,9 @@ private:
     void runPendingPauseTask();
     void runPendingPlayTask();
     void resetPendingTasks();
-    
+    void setEffectInternal(RefPtr<AnimationEffectReadOnly>&&, bool = false);
+    void setTimelineInternal(RefPtr<AnimationTimeline>&&);
+
     String m_id;
     RefPtr<AnimationEffectReadOnly> m_effect;
     RefPtr<AnimationTimeline> m_timeline;