IntersectionObserver leaks documents
authorajuma@chromium.org <ajuma@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 6 Sep 2018 13:51:24 +0000 (13:51 +0000)
committerajuma@chromium.org <ajuma@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 6 Sep 2018 13:51:24 +0000 (13:51 +0000)
https://bugs.webkit.org/show_bug.cgi?id=189128

Reviewed by Simon Fraser.

Source/WebCore:

Currently, Documents own IntersectionObservers while IntersectionObservers own callbacks
that have strong references to Documents. To break this cycle, make Documents only have
weak pointers to IntersectionObservers. Instead, manage the lifetime of an
IntersectionObserver as an ActiveDOMObject, overriding hasPendingActivity to keep
the observer alive while there are ongoing observations.

However, there is a still a potential reference cycle. The callback keeps global
references alive, so if there's a global reference to the observer in JavaScript,
we have an observer->callback->observer cycle, keeping the callback (and hence the Document)
alive. To break this cycle, make IntersectionObserver release the callback when its
Document is stopped.

With these changes, there are no longer any leaks reported with run-webkit-tests --world-leaks
on LayoutTests/intersection-observer and LayoutTests/imported/w3c/web-platform-tests/intersection-observer.

Tests: intersection-observer/no-document-leak.html
       intersection-observer/observer-and-callback-without-js-references.html

* dom/Document.cpp:
(WebCore::Document::addIntersectionObserver):
(WebCore::Document::removeIntersectionObserver):
* dom/Document.h:
* dom/Element.cpp:
(WebCore::Element::didMoveToNewDocument):
* page/IntersectionObserver.cpp:
(WebCore::IntersectionObserver::IntersectionObserver):
(WebCore::IntersectionObserver::~IntersectionObserver):
(WebCore::IntersectionObserver::observe):
(WebCore::IntersectionObserver::rootDestroyed):
(WebCore::IntersectionObserver::createTimestamp const):
(WebCore::IntersectionObserver::notify):
(WebCore::IntersectionObserver::hasPendingActivity const):
(WebCore::IntersectionObserver::activeDOMObjectName const):
(WebCore::IntersectionObserver::canSuspendForDocumentSuspension const):
(WebCore::IntersectionObserver::stop):
* page/IntersectionObserver.h:
(WebCore::IntersectionObserver::trackingDocument const):
(WebCore::IntersectionObserver::trackingDocument): Deleted.
* page/IntersectionObserver.idl:

LayoutTests:

* intersection-observer/no-document-leak-expected.txt: Added.
* intersection-observer/no-document-leak.html: Added.
* intersection-observer/observer-and-callback-without-js-references-expected.txt: Added.
* intersection-observer/observer-and-callback-without-js-references.html: Added.
* intersection-observer/resources/no-document-leak-frame.html: Added.

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

13 files changed:
LayoutTests/ChangeLog
LayoutTests/intersection-observer/no-document-leak-expected.txt [new file with mode: 0644]
LayoutTests/intersection-observer/no-document-leak.html [new file with mode: 0644]
LayoutTests/intersection-observer/observer-and-callback-without-js-references-expected.txt [new file with mode: 0644]
LayoutTests/intersection-observer/observer-and-callback-without-js-references.html [new file with mode: 0644]
LayoutTests/intersection-observer/resources/no-document-leak-frame.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/dom/Document.cpp
Source/WebCore/dom/Document.h
Source/WebCore/dom/Element.cpp
Source/WebCore/page/IntersectionObserver.cpp
Source/WebCore/page/IntersectionObserver.h
Source/WebCore/page/IntersectionObserver.idl

index 5d50e7c..d639b68 100644 (file)
@@ -1,3 +1,16 @@
+2018-09-06  Ali Juma  <ajuma@chromium.org>
+
+        IntersectionObserver leaks documents
+        https://bugs.webkit.org/show_bug.cgi?id=189128
+
+        Reviewed by Simon Fraser.
+
+        * intersection-observer/no-document-leak-expected.txt: Added.
+        * intersection-observer/no-document-leak.html: Added.
+        * intersection-observer/observer-and-callback-without-js-references-expected.txt: Added.
+        * intersection-observer/observer-and-callback-without-js-references.html: Added.
+        * intersection-observer/resources/no-document-leak-frame.html: Added.
+
 2018-09-05  Brent Fulgham  <bfulgham@apple.com>
 
         The width of a nullptr TextRun should be zero
diff --git a/LayoutTests/intersection-observer/no-document-leak-expected.txt b/LayoutTests/intersection-observer/no-document-leak-expected.txt
new file mode 100644 (file)
index 0000000..749cb18
--- /dev/null
@@ -0,0 +1,10 @@
+Tests that using IntersectionObserver does not cause the document to get leaked.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS Document did not leak
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/intersection-observer/no-document-leak.html b/LayoutTests/intersection-observer/no-document-leak.html
new file mode 100644 (file)
index 0000000..28d5d44
--- /dev/null
@@ -0,0 +1,39 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src="../resources/js-test-pre.js"></script>
+</head>
+<body>
+<iframe id="testFrame" src="resources/no-document-leak-frame.html"></iframe>
+<script>
+description("Tests that using IntersectionObserver does not cause the document to get leaked.");
+window.jsTestIsAsync = true;
+
+function documentShouldDie(documentIdentifier)
+{
+    return new Promise(function(resolve, reject) {
+        handle = setInterval(function() {
+            gc();
+            if (internals && !internals.isDocumentAlive(documentIdentifier) && internals.numberOfIntersectionObservers(document) == 0) {
+                clearInterval(handle);
+                resolve();
+            }
+        }, 10);
+    });
+}
+
+var testFrame = document.getElementById("testFrame");
+testFrame.onload = function() {
+    let frameDocumentIdentifier = internals.documentIdentifier(testFrame.contentDocument);
+    testFrame.remove();
+    setTimeout(function() {
+        documentShouldDie(frameDocumentIdentifier).then(function() {
+            testPassed("Document did not leak");
+            finishJSTest();
+        });
+    });
+};
+</script>
+<script src="../resources/js-test-post.js"></script>
+</body>
+</html>
diff --git a/LayoutTests/intersection-observer/observer-and-callback-without-js-references-expected.txt b/LayoutTests/intersection-observer/observer-and-callback-without-js-references-expected.txt
new file mode 100644 (file)
index 0000000..42e433a
--- /dev/null
@@ -0,0 +1,3 @@
+
+PASS IntersectionObserver callback fires even when the observer and callback have no JS references 
+
diff --git a/LayoutTests/intersection-observer/observer-and-callback-without-js-references.html b/LayoutTests/intersection-observer/observer-and-callback-without-js-references.html
new file mode 100644 (file)
index 0000000..bc1749d
--- /dev/null
@@ -0,0 +1,25 @@
+<!DOCTYPE html>
+<script src="../resources/gc.js"></script>
+<script src="../resources/testharness.js"></script>
+<script src="../resources/testharnessreport.js"></script>
+
+<style>
+#target {
+  width: 100px;
+  height: 100px;
+  background-color: green;
+}
+</style>
+<div id="target"></div>
+
+<script>
+var target = document.getElementById("target");
+
+async_test(function(t) {
+    new IntersectionObserver(function(changes) {
+        t.done();
+    }).observe(target);
+}, "IntersectionObserver callback fires even when the observer and callback have no JS references");
+
+gc();
+</script>
diff --git a/LayoutTests/intersection-observer/resources/no-document-leak-frame.html b/LayoutTests/intersection-observer/resources/no-document-leak-frame.html
new file mode 100644 (file)
index 0000000..332baff
--- /dev/null
@@ -0,0 +1,8 @@
+<!DOCTYPE html>
+<div id="target"></div>
+
+<script>
+var target = document.getElementById("target");
+var observer = new IntersectionObserver(function() { });
+observer.observe(target);
+</script>
index 6b1b3fb..8ebfd96 100644 (file)
@@ -1,3 +1,50 @@
+2018-09-06  Ali Juma  <ajuma@chromium.org>
+
+        IntersectionObserver leaks documents
+        https://bugs.webkit.org/show_bug.cgi?id=189128
+
+        Reviewed by Simon Fraser.
+
+        Currently, Documents own IntersectionObservers while IntersectionObservers own callbacks
+        that have strong references to Documents. To break this cycle, make Documents only have
+        weak pointers to IntersectionObservers. Instead, manage the lifetime of an
+        IntersectionObserver as an ActiveDOMObject, overriding hasPendingActivity to keep
+        the observer alive while there are ongoing observations.
+
+        However, there is a still a potential reference cycle. The callback keeps global
+        references alive, so if there's a global reference to the observer in JavaScript,
+        we have an observer->callback->observer cycle, keeping the callback (and hence the Document)
+        alive. To break this cycle, make IntersectionObserver release the callback when its
+        Document is stopped.
+
+        With these changes, there are no longer any leaks reported with run-webkit-tests --world-leaks
+        on LayoutTests/intersection-observer and LayoutTests/imported/w3c/web-platform-tests/intersection-observer.
+
+        Tests: intersection-observer/no-document-leak.html
+               intersection-observer/observer-and-callback-without-js-references.html
+
+        * dom/Document.cpp:
+        (WebCore::Document::addIntersectionObserver):
+        (WebCore::Document::removeIntersectionObserver):
+        * dom/Document.h:
+        * dom/Element.cpp:
+        (WebCore::Element::didMoveToNewDocument):
+        * page/IntersectionObserver.cpp:
+        (WebCore::IntersectionObserver::IntersectionObserver):
+        (WebCore::IntersectionObserver::~IntersectionObserver):
+        (WebCore::IntersectionObserver::observe):
+        (WebCore::IntersectionObserver::rootDestroyed):
+        (WebCore::IntersectionObserver::createTimestamp const):
+        (WebCore::IntersectionObserver::notify):
+        (WebCore::IntersectionObserver::hasPendingActivity const):
+        (WebCore::IntersectionObserver::activeDOMObjectName const):
+        (WebCore::IntersectionObserver::canSuspendForDocumentSuspension const):
+        (WebCore::IntersectionObserver::stop):
+        * page/IntersectionObserver.h:
+        (WebCore::IntersectionObserver::trackingDocument const):
+        (WebCore::IntersectionObserver::trackingDocument): Deleted.
+        * page/IntersectionObserver.idl:
+
 2018-09-05  Zalan Bujtas  <zalan@apple.com>
 
         [LFC] Adapt to the new const WeakPtr<>
index 680ef54..12cbbe7 100644 (file)
@@ -7463,21 +7463,15 @@ void Document::removeViewportDependentPicture(HTMLPictureElement& picture)
 }
 
 #if ENABLE(INTERSECTION_OBSERVER)
-void Document::addIntersectionObserver(RefPtr<IntersectionObserver>&& observer)
+void Document::addIntersectionObserver(IntersectionObserver& observer)
 {
-    ASSERT(m_intersectionObservers.find(observer) == notFound);
-    m_intersectionObservers.append(WTFMove(observer));
+    ASSERT(m_intersectionObservers.find(&observer) == notFound);
+    m_intersectionObservers.append(makeWeakPtr(&observer));
 }
 
-RefPtr<IntersectionObserver> Document::removeIntersectionObserver(IntersectionObserver& observer)
+void Document::removeIntersectionObserver(IntersectionObserver& observer)
 {
-    RefPtr<IntersectionObserver> observerRef;
-    auto index = m_intersectionObservers.find(&observer);
-    if (index != notFound) {
-        observerRef = WTFMove(m_intersectionObservers[index]);
-        m_intersectionObservers.remove(index);
-    }
-    return observerRef;
+    m_intersectionObservers.removeFirst(&observer);
 }
 
 static void computeIntersectionRects(FrameView& frameView, IntersectionObserver& observer, Element& target, FloatRect& absTargetRect, FloatRect& absIntersectionRect, FloatRect& absRootBounds)
index d972cb7..d6e24f5 100644 (file)
@@ -1369,8 +1369,8 @@ public:
     void removeViewportDependentPicture(HTMLPictureElement&);
 
 #if ENABLE(INTERSECTION_OBSERVER)
-    void addIntersectionObserver(RefPtr<IntersectionObserver>&&);
-    RefPtr<IntersectionObserver> removeIntersectionObserver(IntersectionObserver&);
+    void addIntersectionObserver(IntersectionObserver&);
+    void removeIntersectionObserver(IntersectionObserver&);
     unsigned numberOfIntersectionObservers() const { return m_intersectionObservers.size(); }
     void updateIntersectionObservations();
     void scheduleIntersectionObservationUpdate();
@@ -1784,7 +1784,7 @@ private:
     HashSet<HTMLPictureElement*> m_viewportDependentPictures;
 
 #if ENABLE(INTERSECTION_OBSERVER)
-    Vector<RefPtr<IntersectionObserver>> m_intersectionObservers;
+    Vector<WeakPtr<IntersectionObserver>> m_intersectionObservers;
     Vector<WeakPtr<IntersectionObserver>> m_intersectionObserversWithPendingNotifications;
 
     // FIXME: Schedule intersection observation updates in a way that fits into the HTML
index 6fa86b1..b57b475 100644 (file)
@@ -1699,8 +1699,10 @@ void Element::didMoveToNewDocument(Document& oldDocument, Document& newDocument)
 #if ENABLE(INTERSECTION_OBSERVER)
     if (auto* observerData = intersectionObserverData()) {
         for (auto observer : observerData->observers) {
-            if (observer->hasObservationTargets())
-                newDocument.addIntersectionObserver(oldDocument.removeIntersectionObserver(*observer));
+            if (observer->hasObservationTargets()) {
+                oldDocument.removeIntersectionObserver(*observer);
+                newDocument.addIntersectionObserver(*observer);
+            }
         }
     }
 #endif
index 5e64f75..797ea47 100644 (file)
@@ -105,7 +105,8 @@ ExceptionOr<Ref<IntersectionObserver>> IntersectionObserver::create(Document& do
 }
 
 IntersectionObserver::IntersectionObserver(Document& document, Ref<IntersectionObserverCallback>&& callback, Element* root, LengthBox&& parsedRootMargin, Vector<double>&& thresholds)
-    : m_root(root)
+    : ActiveDOMObject(downcast<Document>(callback->scriptExecutionContext()))
+    , m_root(root)
     , m_rootMargin(WTFMove(parsedRootMargin))
     , m_thresholds(WTFMove(thresholds))
     , m_callback(WTFMove(callback))
@@ -117,13 +118,14 @@ IntersectionObserver::IntersectionObserver(Document& document, Ref<IntersectionO
         m_implicitRootDocument = makeWeakPtr(frame->mainFrame().document());
 
     std::sort(m_thresholds.begin(), m_thresholds.end());
+    suspendIfNeeded();
 }
 
 IntersectionObserver::~IntersectionObserver()
 {
     if (m_root)
         m_root->intersectionObserverData()->observers.removeFirst(this);
-    removeAllTargets();
+    disconnect();
 }
 
 String IntersectionObserver::rootMargin() const
@@ -145,7 +147,7 @@ String IntersectionObserver::rootMargin() const
 
 void IntersectionObserver::observe(Element& target)
 {
-    if (!trackingDocument() || m_observationTargets.contains(&target))
+    if (!trackingDocument() || !m_callback || m_observationTargets.contains(&target))
         return;
 
     target.ensureIntersectionObserverData().registrations.append({ makeWeakPtr(this), std::nullopt });
@@ -153,7 +155,7 @@ void IntersectionObserver::observe(Element& target)
     m_observationTargets.append(&target);
     auto* document = trackingDocument();
     if (!hadObservationTargets)
-        document->addIntersectionObserver(this);
+        document->addIntersectionObserver(*this);
     document->scheduleIntersectionObservationUpdate();
 }
 
@@ -219,16 +221,15 @@ void IntersectionObserver::removeAllTargets()
 void IntersectionObserver::rootDestroyed()
 {
     ASSERT(m_root);
-    auto& document = m_root->document();
+    disconnect();
     m_root = nullptr;
-    if (hasObservationTargets()) {
-        removeAllTargets();
-        document.removeIntersectionObserver(*this);
-    }
 }
 
 bool IntersectionObserver::createTimestamp(DOMHighResTimeStamp& timestamp) const
 {
+    if (!m_callback)
+        return false;
+
     auto* context = m_callback->scriptExecutionContext();
     if (!context)
         return false;
@@ -250,12 +251,33 @@ void IntersectionObserver::appendQueuedEntry(Ref<IntersectionObserverEntry>&& en
 
 void IntersectionObserver::notify()
 {
-    if (m_queuedEntries.isEmpty() || !m_callback->canInvokeCallback())
+    if (m_queuedEntries.isEmpty() || !m_callback || !m_callback->canInvokeCallback())
         return;
 
     m_callback->handleEvent(takeRecords(), *this);
 }
 
+bool IntersectionObserver::hasPendingActivity() const
+{
+    return hasObservationTargets() && trackingDocument();
+}
+
+const char* IntersectionObserver::activeDOMObjectName() const
+{
+    return "IntersectionObserver";
+}
+
+bool IntersectionObserver::canSuspendForDocumentSuspension() const
+{
+    return true;
+}
+
+void IntersectionObserver::stop()
+{
+    disconnect();
+    m_callback = nullptr;
+}
+
 } // namespace WebCore
 
 #endif // ENABLE(INTERSECTION_OBSERVER)
index 70abc49..f5089e0 100644 (file)
@@ -27,6 +27,7 @@
 
 #if ENABLE(INTERSECTION_OBSERVER)
 
+#include "ActiveDOMObject.h"
 #include "IntersectionObserverCallback.h"
 #include "IntersectionObserverEntry.h"
 #include "LengthBox.h"
@@ -47,15 +48,15 @@ struct IntersectionObserverRegistration {
 
 struct IntersectionObserverData {
     // IntersectionObservers for which the element that owns this IntersectionObserverData is the root.
-    // An IntersectionObserver without any targets is only owned by JavaScript wrappers. An
-    // IntersectionObserver with at least one target is also owned by its trackingDocument.
+    // An IntersectionObserver is only owned by a JavaScript wrapper. ActiveDOMObject::hasPendingActivity
+    // is overridden to keep this wrapper alive while the observer has ongoing observations.
     Vector<WeakPtr<IntersectionObserver>> observers;
 
     // IntersectionObserverRegistrations for which the element that owns this IntersectionObserverData is the target.
     Vector<IntersectionObserverRegistration> registrations;
 };
 
-class IntersectionObserver : public RefCounted<IntersectionObserver>, public CanMakeWeakPtr<IntersectionObserver> {
+class IntersectionObserver : public RefCounted<IntersectionObserver>, public ActiveDOMObject, public CanMakeWeakPtr<IntersectionObserver> {
 public:
     struct Init {
         Element* root { nullptr };
@@ -67,7 +68,7 @@ public:
 
     ~IntersectionObserver();
 
-    Document* trackingDocument() { return m_root ? &m_root->document() : m_implicitRootDocument.get(); }
+    Document* trackingDocument() const { return m_root ? &m_root->document() : m_implicitRootDocument.get(); }
 
     Element* root() const { return m_root; }
     String rootMargin() const;
@@ -89,6 +90,12 @@ public:
     void appendQueuedEntry(Ref<IntersectionObserverEntry>&&);
     void notify();
 
+    // ActiveDOMObject.
+    bool hasPendingActivity() const override;
+    const char* activeDOMObjectName() const override;
+    bool canSuspendForDocumentSuspension() const override;
+    void stop() override;
+
 private:
     IntersectionObserver(Document&, Ref<IntersectionObserverCallback>&&, Element* root, LengthBox&& parsedRootMargin, Vector<double>&& thresholds);
 
@@ -99,7 +106,7 @@ private:
     Element* m_root;
     LengthBox m_rootMargin;
     Vector<double> m_thresholds;
-    Ref<IntersectionObserverCallback> m_callback;
+    RefPtr<IntersectionObserverCallback> m_callback;
     Vector<Element*> m_observationTargets;
     Vector<Ref<IntersectionObserverEntry>> m_queuedEntries;
 };
index f78772a..ae88a6e 100644 (file)
 // https://wicg.github.io/IntersectionObserver/
 
 [
+    ActiveDOMObject,
     Conditional=INTERSECTION_OBSERVER,
     ConstructorCallWith=Document,
     ConstructorMayThrowException,
     Constructor(IntersectionObserverCallback callback, optional IntersectionObserverInit options),
-    ImplementationLacksVTable,
     EnabledAtRuntime=IntersectionObserver
 ] interface IntersectionObserver {
     readonly attribute Element? root;