Attachment 348969 Details for Bug 189128 – Patch

[patch] Patch

bug-189128-20180905180505.patch (text/plain), 18.51 KB, created by Ali Juma on 2018-09-05 15:05:06 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Ali Juma

Created: 2018-09-05 15:05:06 PDT

Size: 18.51 KB

patch

obsolete

>Subversion Revision: 235514
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 1c4dde3f2b5225f5a73fb3d71221c596ff863222..38d44bc434555afc46d681289e88d98e3a706c50 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,50 @@
>+2018-09-05  Ali Juma  <ajuma@chromium.org>
>+
>+        IntersectionObserver leaks documents
>+        https://bugs.webkit.org/show_bug.cgi?id=189128
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        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-08-30  Eric Carlson  <eric.carlson@apple.com>
> 
>         Mock video devices should only support discrete sizes
>diff --git a/Source/WebCore/dom/Document.cpp b/Source/WebCore/dom/Document.cpp
>index e723591a9133ca518ba0a4e512ed17fc4d74ce8e..6632f1cf1ef9ac9db541957a83b37e1fb78c8e94 100644
>--- a/Source/WebCore/dom/Document.cpp
>+++ b/Source/WebCore/dom/Document.cpp
>@@ -7427,21 +7427,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)
>diff --git a/Source/WebCore/dom/Document.h b/Source/WebCore/dom/Document.h
>index 9e3c31853f123ec9bea3f690b85e17bad07675d5..80490d274f29b0a92633130276fce2377cb73650 100644
>--- a/Source/WebCore/dom/Document.h
>+++ b/Source/WebCore/dom/Document.h
>@@ -1364,8 +1364,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();
>@@ -1774,7 +1774,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
>diff --git a/Source/WebCore/dom/Element.cpp b/Source/WebCore/dom/Element.cpp
>index b78da9cd332a7418abc5a99326fdf1b42a066cab..c27641294639ef5c95b8be343cf934c2ee0efefd 100644
>--- a/Source/WebCore/dom/Element.cpp
>+++ b/Source/WebCore/dom/Element.cpp
>@@ -1655,8 +1655,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
>diff --git a/Source/WebCore/page/IntersectionObserver.cpp b/Source/WebCore/page/IntersectionObserver.cpp
>index 5e64f7592bc02802f963c1b36b206a79241e6ccf..797ea47be145c2c3e9771167afa7cecae2458f0b 100644
>--- a/Source/WebCore/page/IntersectionObserver.cpp
>+++ b/Source/WebCore/page/IntersectionObserver.cpp
>@@ -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)
>diff --git a/Source/WebCore/page/IntersectionObserver.h b/Source/WebCore/page/IntersectionObserver.h
>index 70abc4930546e76e51075b5dbc59d6c1a45a5495..f5089e04fe92fda42a72a1fcbc41adbc823b1ea4 100644
>--- a/Source/WebCore/page/IntersectionObserver.h
>+++ b/Source/WebCore/page/IntersectionObserver.h
>@@ -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;
> };
>diff --git a/Source/WebCore/page/IntersectionObserver.idl b/Source/WebCore/page/IntersectionObserver.idl
>index f78772af74ec53d9109582365aac1c2f5c2fc852..ae88a6e232fba9ba7fbb85ee7aa69ce0f0f441aa 100644
>--- a/Source/WebCore/page/IntersectionObserver.idl
>+++ b/Source/WebCore/page/IntersectionObserver.idl
>@@ -26,11 +26,11 @@
> // 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;
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 5aedfbd4888cc0506e0333f9968ffd932884e6b2..9df5a54c3ae59ccfcf17cd3e1dbaf656f7155430 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,16 @@
>+2018-09-05  Ali Juma  <ajuma@chromium.org>
>+
>+        IntersectionObserver leaks documents
>+        https://bugs.webkit.org/show_bug.cgi?id=189128
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * 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-08-30  Eric Carlson  <eric.carlson@apple.com>
> 
>         Mock video devices should only support discrete sizes
>diff --git a/LayoutTests/intersection-observer/no-document-leak-expected.txt b/LayoutTests/intersection-observer/no-document-leak-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..749cb18128e7324090bb78ab15abe42f2ddeb26e
>--- /dev/null
>+++ b/LayoutTests/intersection-observer/no-document-leak-expected.txt
>@@ -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
>index 0000000000000000000000000000000000000000..28d5d44bccc17a764c274c3f362e08b9838fd283
>--- /dev/null
>+++ b/LayoutTests/intersection-observer/no-document-leak.html
>@@ -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
>index 0000000000000000000000000000000000000000..42e433a72c32a4482790c73d5e0acc1f725c0909
>--- /dev/null
>+++ b/LayoutTests/intersection-observer/observer-and-callback-without-js-references-expected.txt
>@@ -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
>index 0000000000000000000000000000000000000000..bc1749d061d97953e8cefe7aafa738643cd053ee
>--- /dev/null
>+++ b/LayoutTests/intersection-observer/observer-and-callback-without-js-references.html
>@@ -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
>index 0000000000000000000000000000000000000000..332baffaa13ae1c2fe659e3b2dfb4344dce9866f
>--- /dev/null
>+++ b/LayoutTests/intersection-observer/resources/no-document-leak-frame.html
>@@ -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>

Actions: View | Formatted Diff | Diff

Attachments on bug 189128: 348969 | 349020