- Revision
- 235736
- Author
- [email protected]
- Date
- 2018-09-06 06:51:24 -0700 (Thu, 06 Sep 2018)
Log Message
IntersectionObserver leaks documents
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.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (235735 => 235736)
--- trunk/LayoutTests/ChangeLog 2018-09-06 13:40:56 UTC (rev 235735)
+++ trunk/LayoutTests/ChangeLog 2018-09-06 13:51:24 UTC (rev 235736)
@@ -1,3 +1,16 @@
+2018-09-06 Ali Juma <[email protected]>
+
+ 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 <[email protected]>
The width of a nullptr TextRun should be zero
Added: trunk/LayoutTests/intersection-observer/no-document-leak-expected.txt (0 => 235736)
--- trunk/LayoutTests/intersection-observer/no-document-leak-expected.txt (rev 0)
+++ trunk/LayoutTests/intersection-observer/no-document-leak-expected.txt 2018-09-06 13:51:24 UTC (rev 235736)
@@ -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
+
Added: trunk/LayoutTests/intersection-observer/no-document-leak.html (0 => 235736)
--- trunk/LayoutTests/intersection-observer/no-document-leak.html (rev 0)
+++ trunk/LayoutTests/intersection-observer/no-document-leak.html 2018-09-06 13:51:24 UTC (rev 235736)
@@ -0,0 +1,39 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src=""
+</head>
+<body>
+<iframe id="testFrame" src=""
+<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=""
+</body>
+</html>
Added: trunk/LayoutTests/intersection-observer/observer-and-callback-without-js-references-expected.txt (0 => 235736)
--- trunk/LayoutTests/intersection-observer/observer-and-callback-without-js-references-expected.txt (rev 0)
+++ trunk/LayoutTests/intersection-observer/observer-and-callback-without-js-references-expected.txt 2018-09-06 13:51:24 UTC (rev 235736)
@@ -0,0 +1,3 @@
+
+PASS IntersectionObserver callback fires even when the observer and callback have no JS references
+
Added: trunk/LayoutTests/intersection-observer/observer-and-callback-without-js-references.html (0 => 235736)
--- trunk/LayoutTests/intersection-observer/observer-and-callback-without-js-references.html (rev 0)
+++ trunk/LayoutTests/intersection-observer/observer-and-callback-without-js-references.html 2018-09-06 13:51:24 UTC (rev 235736)
@@ -0,0 +1,25 @@
+<!DOCTYPE html>
+<script src=""
+<script src=""
+<script src=""
+
+<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>
Added: trunk/LayoutTests/intersection-observer/resources/no-document-leak-frame.html (0 => 235736)
--- trunk/LayoutTests/intersection-observer/resources/no-document-leak-frame.html (rev 0)
+++ trunk/LayoutTests/intersection-observer/resources/no-document-leak-frame.html 2018-09-06 13:51:24 UTC (rev 235736)
@@ -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>
Modified: trunk/Source/WebCore/ChangeLog (235735 => 235736)
--- trunk/Source/WebCore/ChangeLog 2018-09-06 13:40:56 UTC (rev 235735)
+++ trunk/Source/WebCore/ChangeLog 2018-09-06 13:51:24 UTC (rev 235736)
@@ -1,3 +1,50 @@
+2018-09-06 Ali Juma <[email protected]>
+
+ 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 <[email protected]>
[LFC] Adapt to the new const WeakPtr<>
Modified: trunk/Source/WebCore/dom/Document.cpp (235735 => 235736)
--- trunk/Source/WebCore/dom/Document.cpp 2018-09-06 13:40:56 UTC (rev 235735)
+++ trunk/Source/WebCore/dom/Document.cpp 2018-09-06 13:51:24 UTC (rev 235736)
@@ -7463,21 +7463,15 @@
}
#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)
Modified: trunk/Source/WebCore/dom/Document.h (235735 => 235736)
--- trunk/Source/WebCore/dom/Document.h 2018-09-06 13:40:56 UTC (rev 235735)
+++ trunk/Source/WebCore/dom/Document.h 2018-09-06 13:51:24 UTC (rev 235736)
@@ -1369,8 +1369,8 @@
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 @@
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
Modified: trunk/Source/WebCore/dom/Element.cpp (235735 => 235736)
--- trunk/Source/WebCore/dom/Element.cpp 2018-09-06 13:40:56 UTC (rev 235735)
+++ trunk/Source/WebCore/dom/Element.cpp 2018-09-06 13:51:24 UTC (rev 235736)
@@ -1699,8 +1699,10 @@
#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
Modified: trunk/Source/WebCore/page/IntersectionObserver.cpp (235735 => 235736)
--- trunk/Source/WebCore/page/IntersectionObserver.cpp 2018-09-06 13:40:56 UTC (rev 235735)
+++ trunk/Source/WebCore/page/IntersectionObserver.cpp 2018-09-06 13:51:24 UTC (rev 235736)
@@ -105,7 +105,8 @@
}
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,6 +118,7 @@
m_implicitRootDocument = makeWeakPtr(frame->mainFrame().document());
std::sort(m_thresholds.begin(), m_thresholds.end());
+ suspendIfNeeded();
}
IntersectionObserver::~IntersectionObserver()
@@ -123,7 +125,7 @@
{
if (m_root)
m_root->intersectionObserverData()->observers.removeFirst(this);
- removeAllTargets();
+ disconnect();
}
String IntersectionObserver::rootMargin() const
@@ -145,7 +147,7 @@
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 @@
m_observationTargets.append(&target);
auto* document = trackingDocument();
if (!hadObservationTargets)
- document->addIntersectionObserver(this);
+ document->addIntersectionObserver(*this);
document->scheduleIntersectionObservationUpdate();
}
@@ -219,16 +221,15 @@
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::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)
Modified: trunk/Source/WebCore/page/IntersectionObserver.h (235735 => 235736)
--- trunk/Source/WebCore/page/IntersectionObserver.h 2018-09-06 13:40:56 UTC (rev 235735)
+++ trunk/Source/WebCore/page/IntersectionObserver.h 2018-09-06 13:51:24 UTC (rev 235736)
@@ -27,6 +27,7 @@
#if ENABLE(INTERSECTION_OBSERVER)
+#include "ActiveDOMObject.h"
#include "IntersectionObserverCallback.h"
#include "IntersectionObserverEntry.h"
#include "LengthBox.h"
@@ -47,8 +48,8 @@
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.
@@ -55,7 +56,7 @@
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 @@
~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 @@
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 @@
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;
};
Modified: trunk/Source/WebCore/page/IntersectionObserver.idl (235735 => 235736)
--- trunk/Source/WebCore/page/IntersectionObserver.idl 2018-09-06 13:40:56 UTC (rev 235735)
+++ trunk/Source/WebCore/page/IntersectionObserver.idl 2018-09-06 13:51:24 UTC (rev 235736)
@@ -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;
