Attachment 343700 Details for Bug 187088 – Patch

[patch] Patch

bug-187088-20180627100829.patch (text/plain), 11.83 KB, created by Antoine Quint on 2018-06-27 01:08:31 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Antoine Quint

Created: 2018-06-27 01:08:31 PDT

Size: 11.83 KB

patch

obsolete

>Subversion Revision: 233164
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 041728d2f48537e7d98007b39e98d85e309af9b8..300d648764588bbfe1234195ff1f2421e5e0bba3 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -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 NOBODY (OOPS!).
>+
>+        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-25  Antoine Quint  <graouts@apple.com>
> 
>         REGRESSION: hardware-accelerated animation fails on inline element
>diff --git a/Source/WebCore/animation/AnimationTimeline.cpp b/Source/WebCore/animation/AnimationTimeline.cpp
>index 3f69cb169ab663e78e0cec061d137637a3681e37..8e124e779b5ceacd9a9277dfa22497f6d76f01e5 100644
>--- a/Source/WebCore/animation/AnimationTimeline.cpp
>+++ b/Source/WebCore/animation/AnimationTimeline.cpp
>@@ -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)
>diff --git a/Source/WebCore/animation/DeclarativeAnimation.cpp b/Source/WebCore/animation/DeclarativeAnimation.cpp
>index b944a2a0c69bf0e2444b094fd80d50b7ac7fcbc2..173d403aafcfe25e2d84a40c4ba2f8e81365801b 100644
>--- a/Source/WebCore/animation/DeclarativeAnimation.cpp
>+++ b/Source/WebCore/animation/DeclarativeAnimation.cpp
>@@ -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)
>diff --git a/Source/WebCore/animation/DeclarativeAnimation.h b/Source/WebCore/animation/DeclarativeAnimation.h
>index a9bec97a5c8c38722ac8342e54fb6cee0607e72e..b12f35d6ba8e62188d2c86251ab20ce4b3daa5e9 100644
>--- a/Source/WebCore/animation/DeclarativeAnimation.h
>+++ b/Source/WebCore/animation/DeclarativeAnimation.h
>@@ -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&);
>diff --git a/Source/WebCore/animation/DocumentTimeline.cpp b/Source/WebCore/animation/DocumentTimeline.cpp
>index 5ab209827fa661488f936043abad56b8b92522aa..0c38d8d12998c779207aac73e8f58496583bc78b 100644
>--- a/Source/WebCore/animation/DocumentTimeline.cpp
>+++ b/Source/WebCore/animation/DocumentTimeline.cpp
>@@ -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;
> }
> 
>diff --git a/Source/WebCore/animation/WebAnimation.cpp b/Source/WebCore/animation/WebAnimation.cpp
>index 9753c0183c094f3340524fb4222739838d1ccdcf..cd23039de94d1e698ed8e9d49e7e174dc413c678 100644
>--- a/Source/WebCore/animation/WebAnimation.cpp
>+++ b/Source/WebCore/animation/WebAnimation.cpp
>@@ -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)
>diff --git a/Source/WebCore/animation/WebAnimation.h b/Source/WebCore/animation/WebAnimation.h
>index 052b94af779259c68aa81069ff6d9c54e83e111d..a48c9ef200c3170baee3e01ade1ef4af5dfa04c3 100644
>--- a/Source/WebCore/animation/WebAnimation.h
>+++ b/Source/WebCore/animation/WebAnimation.h
>@@ -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>&&);
> 
>@@ -116,7 +115,7 @@ public:
>     void unsuspendEffectInvalidation();
>     void setSuspended(bool);
>     bool isSuspended() const { return m_isSuspended; }
>-    virtual void prepareAnimationForRemoval();
>+    virtual void remove();
> 
>     String description();
> 
>@@ -155,7 +154,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;
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 08b97b207e4ae3b6ce1cf28ed22c528a5a64806b..91f834a7a11062d0a9880cc17951f461d4b8fa85 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -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 NOBODY (OOPS!).
>+
>+        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-25  Antoine Quint  <graouts@apple.com>
> 
>         REGRESSION: hardware-accelerated animation fails on inline element
>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
>index 0000000000000000000000000000000000000000..3c7c90c6157f8c14e95ecc402579250709bde185
>--- /dev/null
>+++ b/LayoutTests/webanimations/leak-document-with-web-animation-expected.txt
>@@ -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
>index 0000000000000000000000000000000000000000..3ec9a5a2141cde9e63a2bec1836380db8bbc8adc
>--- /dev/null
>+++ b/LayoutTests/webanimations/leak-document-with-web-animation.html
>@@ -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
>index 0000000000000000000000000000000000000000..63f269f526492bddb193bc31bc502938c2a315ff
>--- /dev/null
>+++ b/LayoutTests/webanimations/resources/web-animation-leak-iframe.html
>@@ -0,0 +1,4 @@
>+<div></div>
>+<script type="text/javascript">
>+document.querySelector("div").animate({ marginLeft: ["0", "100px"] }, 1000);
>+</script>

Actions: View | Formatted Diff | Diff

Attachments on bug 187088: 343700 | 344097 | 344124 | 344167 | 344192 | 344197