DocumentTimeline / CSSTransition objects are leaking on CNN.com
authorgraouts@webkit.org <graouts@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 23 Mar 2020 08:17:26 +0000 (08:17 +0000)
committergraouts@webkit.org <graouts@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 23 Mar 2020 08:17:26 +0000 (08:17 +0000)
https://bugs.webkit.org/show_bug.cgi?id=208069
<rdar://problem/59680143>

Reviewed by Simon Fraser, Geoffrey Garen and Darin Adler.

Source/WebCore:

Test: webanimations/leak-css-animation.html

We add a test feature that lets use query the availability of a given WebAnimation by its "id" property in the WebAnimation::instances list.
We also fix some build issues that appeared with a change in UnifiedSources order.

* animation/ElementAnimationRareData.cpp:
(WebCore::ElementAnimationRareData::setAnimationsCreatedByMarkup):
* animation/ElementAnimationRareData.h:
(WebCore::ElementAnimationRareData::setAnimationsCreatedByMarkup): Deleted.
* animation/WebAnimation.h:
* testing/Internals.cpp:
(WebCore::Internals::animationWithIdExists const):
* testing/Internals.h:
* testing/Internals.idl:

Source/WTF:

If a CSSAnimation is set on an element using the `animation-name` CSS property, and later removed, it will leak due to the ListHashSet<RefPtr<CSSAnimation>>
(aka CSSAnimationCollection) member on ElementAnimationRareData being replaced to the new list, but the old list not being cleared from its members.

We fix the ListHashSet assignment operator to use swap ensuring previously held items are cleared.

* wtf/ListHashSet.h:
(WTF::=):

Tools:

Add a test that checks that a ListHashSet containing RefPtr<> types correctly calls the destructor for those items when the assignment operator is used.

* TestWebKitAPI/Tests/WTF/ListHashSet.cpp:
(TestWebKitAPI::ListHashSetReferencedItem::create):
(TestWebKitAPI::ListHashSetReferencedItem::ListHashSetReferencedItem):
(TestWebKitAPI::ListHashSetReferencedItem::~ListHashSetReferencedItem):
(TestWebKitAPI::FakeElementAnimationRareData::FakeElementAnimationRareData):
(TestWebKitAPI::FakeElementAnimationRareData::~FakeElementAnimationRareData):
(TestWebKitAPI::FakeElementAnimationRareData::collection):
(TestWebKitAPI::FakeElementAnimationRareData::setCollection):
(TestWebKitAPI::TEST):

LayoutTests:

Add a test that checks that setting a CSSAnimation on an element, waiting a frame, and removing it will not leak that CSSAnimation.

* webanimations/leak-css-animation-expected.txt: Added.
* webanimations/leak-css-animation.html: Added.
* webanimations/resources/css-animation-leak-iframe.html: Added.

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

15 files changed:
LayoutTests/ChangeLog
LayoutTests/webanimations/leak-css-animation-expected.txt [new file with mode: 0644]
LayoutTests/webanimations/leak-css-animation.html [new file with mode: 0644]
LayoutTests/webanimations/resources/css-animation-leak-iframe.html [new file with mode: 0644]
Source/WTF/ChangeLog
Source/WTF/wtf/ListHashSet.h
Source/WebCore/ChangeLog
Source/WebCore/animation/ElementAnimationRareData.cpp
Source/WebCore/animation/ElementAnimationRareData.h
Source/WebCore/animation/WebAnimation.h
Source/WebCore/testing/Internals.cpp
Source/WebCore/testing/Internals.h
Source/WebCore/testing/Internals.idl
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WTF/ListHashSet.cpp

index 63e289a..f14a4a0 100644 (file)
@@ -1,5 +1,19 @@
 2020-03-22  Antoine Quint  <graouts@apple.com>
 
+        DocumentTimeline / CSSTransition objects are leaking on CNN.com
+        https://bugs.webkit.org/show_bug.cgi?id=208069
+        <rdar://problem/59680143>
+
+        Reviewed by Simon Fraser, Geoffrey Garen and Darin Adler.
+
+        Add a test that checks that setting a CSSAnimation on an element, waiting a frame, and removing it will not leak that CSSAnimation.
+
+        * webanimations/leak-css-animation-expected.txt: Added.
+        * webanimations/leak-css-animation.html: Added.
+        * webanimations/resources/css-animation-leak-iframe.html: Added.
+
+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>
diff --git a/LayoutTests/webanimations/leak-css-animation-expected.txt b/LayoutTests/webanimations/leak-css-animation-expected.txt
new file mode 100644 (file)
index 0000000..abfd910
--- /dev/null
@@ -0,0 +1,10 @@
+This test asserts that a CSSAnimation doesn't leak after it was removed declaratively and the document was replaced.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS The CSS animation was destroyed.
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/webanimations/leak-css-animation.html b/LayoutTests/webanimations/leak-css-animation.html
new file mode 100644 (file)
index 0000000..02f0cc7
--- /dev/null
@@ -0,0 +1,74 @@
+<!DOCTYPE html>
+<html>
+<body onload="runTest()">
+<script src="../resources/js-test.js"></script>
+<script>
+description("This test asserts that a CSSAnimation doesn't leak after it was removed declaratively and the document was replaced.");
+
+if (window.internals)
+    jsTestIsAsync = true;
+
+function runTest() {
+    if (!window.internals)
+        return;
+
+    var frame = document.body.appendChild(document.createElement("iframe"));
+
+    frame.addEventListener("load", async () => {
+        if (frame.src === 'about:blank')
+            return;
+
+        const animationId = "leak-css-animation";
+
+        await (element => {
+            return new Promise(resolve => {
+                const animation = element.getAnimations()[0];
+                if (!animation) {
+                    testFailed("The expected CSS animation was not created.");
+                    finishJSTest();
+                }
+
+                animation.id = animationId;
+                if (!internals.animationWithIdExists(animationId)) {
+                    testFailed("The expected CSS animation with the provided ID was not initially found.");
+                    finishJSTest();
+                }
+
+                requestAnimationFrame(() => {
+                    element.style.animation = "none";
+                    resolve();
+                });
+            });
+        })(frame.contentDocument.querySelector("div"));
+
+        requestAnimationFrame(() => {
+            frame.remove();
+            frame = null;
+
+            gc();
+            let timeout = 0;
+            const handle = setInterval(() => {
+                if (!internals.animationWithIdExists(animationId)) {
+                    clearInterval(handle);
+                    testPassed("The CSS animation was destroyed.");
+                    finishJSTest();
+                    return;
+                }
+                timeout++;
+                if (timeout == 500) {
+                    clearInterval(handle);
+                    testFailed("The CSS animation was leaked.");
+                    finishJSTest();
+                    return;
+                }
+                gc();
+            }, 10);
+        });
+    });
+
+    frame.src = 'resources/css-animation-leak-iframe.html';
+}
+
+</script>
+</body>
+</html>
diff --git a/LayoutTests/webanimations/resources/css-animation-leak-iframe.html b/LayoutTests/webanimations/resources/css-animation-leak-iframe.html
new file mode 100644 (file)
index 0000000..47c30a9
--- /dev/null
@@ -0,0 +1,10 @@
+<style>
+div {
+    animation: animation 1000s;
+}
+
+@keyframes animation {
+    to { margin-left: 100px }
+}
+</style>
+<div></div>
index 005b466..6617bb8 100644 (file)
@@ -1,3 +1,19 @@
+2020-03-22  Antoine Quint  <graouts@apple.com>
+
+        DocumentTimeline / CSSTransition objects are leaking on CNN.com
+        https://bugs.webkit.org/show_bug.cgi?id=208069
+        <rdar://problem/59680143>
+
+        Reviewed by Simon Fraser, Geoffrey Garen and Darin Adler.
+
+        If a CSSAnimation is set on an element using the `animation-name` CSS property, and later removed, it will leak due to the ListHashSet<RefPtr<CSSAnimation>>
+        (aka CSSAnimationCollection) member on ElementAnimationRareData being replaced to the new list, but the old list not being cleared from its members.
+
+        We fix the ListHashSet assignment operator to use swap ensuring previously held items are cleared.
+
+        * wtf/ListHashSet.h:
+        (WTF::=):
+
 2020-03-20  Per Arne Vollan  <pvollan@apple.com>
 
         [Cocoa] Deny access to database mapping service
index 76e4b6a..e7f4d62 100644 (file)
@@ -351,9 +351,8 @@ inline ListHashSet<T, U>::ListHashSet(ListHashSet&& other)
 template<typename T, typename U>
 inline ListHashSet<T, U>& ListHashSet<T, U>::operator=(ListHashSet&& other)
 {
-    m_impl = WTFMove(other.m_impl);
-    m_head = std::exchange(other.m_head, nullptr);
-    m_tail = std::exchange(other.m_tail, nullptr);
+    ListHashSet tmp(WTFMove(other));
+    swap(tmp);
     return *this;
 }
 
index aa986ff..4bf0d84 100644 (file)
@@ -1,5 +1,28 @@
 2020-03-22  Antoine Quint  <graouts@apple.com>
 
+        DocumentTimeline / CSSTransition objects are leaking on CNN.com
+        https://bugs.webkit.org/show_bug.cgi?id=208069
+        <rdar://problem/59680143>
+
+        Reviewed by Simon Fraser, Geoffrey Garen and Darin Adler.
+
+        Test: webanimations/leak-css-animation.html
+
+        We add a test feature that lets use query the availability of a given WebAnimation by its "id" property in the WebAnimation::instances list.
+        We also fix some build issues that appeared with a change in UnifiedSources order.
+
+        * animation/ElementAnimationRareData.cpp:
+        (WebCore::ElementAnimationRareData::setAnimationsCreatedByMarkup):
+        * animation/ElementAnimationRareData.h:
+        (WebCore::ElementAnimationRareData::setAnimationsCreatedByMarkup): Deleted.
+        * animation/WebAnimation.h:
+        * testing/Internals.cpp:
+        (WebCore::Internals::animationWithIdExists const):
+        * testing/Internals.h:
+        * testing/Internals.idl:
+
+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>
index c83a13e..83b0a7a 100644 (file)
@@ -26,6 +26,7 @@
 #include "config.h"
 #include "ElementAnimationRareData.h"
 
+#include "CSSAnimation.h"
 #include "KeyframeEffectStack.h"
 
 namespace WebCore {
@@ -45,4 +46,9 @@ KeyframeEffectStack& ElementAnimationRareData::ensureKeyframeEffectStack()
     return *m_keyframeEffectStack.get();
 }
 
+void ElementAnimationRareData::setAnimationsCreatedByMarkup(CSSAnimationCollection&& animations)
+{
+    m_animationsCreatedByMarkup = WTFMove(animations);
+}
+
 } // namespace WebCore
index 56ea0a1..0a964d2 100644 (file)
@@ -48,7 +48,7 @@ public:
     AnimationCollection& cssAnimations() { return m_cssAnimations; }
     AnimationCollection& transitions() { return m_transitions; }
     CSSAnimationCollection& animationsCreatedByMarkup() { return m_animationsCreatedByMarkup; }
-    void setAnimationsCreatedByMarkup(CSSAnimationCollection&& animations) { m_animationsCreatedByMarkup = WTFMove(animations); }
+    void setAnimationsCreatedByMarkup(CSSAnimationCollection&&);
     PropertyToTransitionMap& completedTransitionByProperty() { return m_completedTransitionByProperty; }
     PropertyToTransitionMap& runningTransitionsByProperty() { return m_runningTransitionsByProperty; }
 
index 044b8c6..b14005a 100644 (file)
@@ -57,7 +57,7 @@ public:
     static Ref<WebAnimation> create(Document&, AnimationEffect*, AnimationTimeline*);
     ~WebAnimation();
 
-    static HashSet<WebAnimation*>& instances();
+    WEBCORE_EXPORT static HashSet<WebAnimation*>& instances();
 
     virtual bool isDeclarativeAnimation() const { return false; }
     virtual bool isCSSAnimation() const { return false; }
index d81981f..04a99f2 100644 (file)
 #include "UserMediaController.h"
 #include "ViewportArguments.h"
 #include "VoidCallback.h"
+#include "WebAnimation.h"
 #include "WebCoreJSClientData.h"
 #include "WindowProxy.h"
 #include "WorkerThread.h"
@@ -1035,6 +1036,15 @@ ExceptionOr<unsigned> Internals::lastSpatialNavigationCandidateCount() const
     return contextDocument()->page()->lastSpatialNavigationCandidateCount();
 }
 
+bool Internals::animationWithIdExists(const String& id) const
+{
+    for (auto* animation : WebAnimation::instances()) {
+        if (animation && animation->id() == id)
+            return true;
+    }
+    return false;
+}
+
 unsigned Internals::numberOfActiveAnimations() const
 {
     if (RuntimeEnabledFeatures::sharedFeatures().webAnimationsCSSIntegrationEnabled())
index 76c4389..72f49e2 100644 (file)
@@ -210,6 +210,7 @@ public:
     ExceptionOr<unsigned> lastSpatialNavigationCandidateCount() const;
 
     // CSS Animation testing.
+    bool animationWithIdExists(const String&) const;
     unsigned numberOfActiveAnimations() const;
     ExceptionOr<bool> animationsAreSuspended() const;
     ExceptionOr<void> suspendAnimations() const;
index 514749e..162faf4 100644 (file)
@@ -242,6 +242,7 @@ enum CompositingPolicy {
     readonly attribute unsigned long inflightBeaconsCount;
 
     // CSS Animation testing.
+    boolean animationWithIdExists(DOMString id);
     unsigned long numberOfActiveAnimations();
     [MayThrowException] void suspendAnimations();
     [MayThrowException] void resumeAnimations();
index d39ed4f..f406364 100644 (file)
@@ -1,3 +1,23 @@
+2020-03-22  Antoine Quint  <graouts@apple.com>
+
+        DocumentTimeline / CSSTransition objects are leaking on CNN.com
+        https://bugs.webkit.org/show_bug.cgi?id=208069
+        <rdar://problem/59680143>
+
+        Reviewed by Simon Fraser, Geoffrey Garen and Darin Adler.
+
+        Add a test that checks that a ListHashSet containing RefPtr<> types correctly calls the destructor for those items when the assignment operator is used.
+
+        * TestWebKitAPI/Tests/WTF/ListHashSet.cpp:
+        (TestWebKitAPI::ListHashSetReferencedItem::create):
+        (TestWebKitAPI::ListHashSetReferencedItem::ListHashSetReferencedItem):
+        (TestWebKitAPI::ListHashSetReferencedItem::~ListHashSetReferencedItem):
+        (TestWebKitAPI::FakeElementAnimationRareData::FakeElementAnimationRareData):
+        (TestWebKitAPI::FakeElementAnimationRareData::~FakeElementAnimationRareData):
+        (TestWebKitAPI::FakeElementAnimationRareData::collection):
+        (TestWebKitAPI::FakeElementAnimationRareData::setCollection):
+        (TestWebKitAPI::TEST):
+
 2020-03-21  Michael Catanzaro  <mcatanzaro@gnome.org>
 
         [GTK] Use ${PYTHON_EXECUTABLE} to run generate-gtkdoc
index b770a5b..1ce1d0f 100644 (file)
@@ -28,6 +28,8 @@
 #include "Counters.h"
 #include "MoveOnly.h"
 #include <wtf/ListHashSet.h>
+#include <wtf/NeverDestroyed.h>
+#include <wtf/RefCounted.h>
 
 namespace TestWebKitAPI {
 
@@ -476,4 +478,63 @@ TEST(WTF_ListHashSet, UniquePtrKey_RemoveUsingRawPointer)
     EXPECT_EQ(1u, ConstructorDestructorCounter::destructionCount);
 }
 
+class ListHashSetReferencedItem : public RefCounted<ListHashSetReferencedItem> {
+public:
+    static Ref<ListHashSetReferencedItem> create()
+    {
+        auto result = adoptRef(*new ListHashSetReferencedItem());
+        return result;
+    }
+
+    explicit ListHashSetReferencedItem()
+    {
+        instances().add(this);
+    }
+
+    ~ListHashSetReferencedItem()
+    {
+        ASSERT(instances().contains(this));
+        instances().remove(this);
+    }
+
+    static HashSet<ListHashSetReferencedItem*>& instances()
+    {
+        static NeverDestroyed<HashSet<ListHashSetReferencedItem*>> instances;
+        return instances;
+    }
+};
+
+using Collection = ListHashSet<RefPtr<ListHashSetReferencedItem>>;
+
+class FakeElementAnimationRareData {
+    WTF_MAKE_NONCOPYABLE(FakeElementAnimationRareData);
+    WTF_MAKE_FAST_ALLOCATED;
+public:
+    explicit FakeElementAnimationRareData() { };
+    ~FakeElementAnimationRareData() { };
+
+    Collection& collection() { return m_collection; }
+    void setCollection(Collection&& collection) { m_collection = WTFMove(collection); }
+
+private:
+    Collection m_collection;
+};
+
+TEST(WTF_ListHashSet, ClearsItemUponAssignment)
+{
+    std::unique_ptr<FakeElementAnimationRareData> data = makeUnique<FakeElementAnimationRareData>();
+
+    EXPECT_EQ(0u, ListHashSetReferencedItem::instances().size());
+
+    Collection firstCollection({ ListHashSetReferencedItem::create() });
+    data->setCollection(WTFMove(firstCollection));
+
+    EXPECT_EQ(1u, ListHashSetReferencedItem::instances().size());
+
+    Collection secondCollection;
+    data->setCollection(WTFMove(secondCollection));
+
+    EXPECT_EQ(0u, ListHashSetReferencedItem::instances().size());
+}
+
 } // namespace TestWebKitAPI