IntersectionObserverEntry doesn't keep JS wrappers of rects alive

https://bugs.webkit.org/show_bug.cgi?id=191330 Reviewed by Chris Dumez. Source/WebCore: Retain wrappers of each rect in an IntersectionObserverEntry as long as the entry's wrapper is alive, by adding these wrappers as opaque roots. Test: intersection-observer/intersection-observer-entry-keeps-js-wrappers-of-rects-alive.html * bindings/js/JSIntersectionObserverEntryCustom.cpp: (WebCore::JSIntersectionObserverEntry::visitAdditionalChildren): * dom/DOMRectReadOnly.idl: * page/IntersectionObserverEntry.h: (WebCore::IntersectionObserverEntry::rootBounds const): Make this return a raw pointer instead of a RefPtr so that it can be called in JSIntersectionObserverEntry::visitAdditionalChildren, which can be called from non-main threads. (WebCore::IntersectionObserverEntry::boundingClientRect const): Ditto. (WebCore::IntersectionObserverEntry::intersectionRect const): Ditto. LayoutTests: Add test coverage. * intersection-observer/intersection-observer-entry-keeps-js-wrappers-of-rects-alive-expected.txt: Added. * intersection-observer/intersection-observer-entry-keeps-js-wrappers-of-rects-alive.html: Added. Canonical link: https://commits.webkit.org/206168@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@237929 268f45cc-cd09-0410-ab3c-d52691b4dbfc
WebKit · Nov 7, 2018 · 3f54266 · 3f54266
1 parent abbe5dd
commit 3f54266
Show file tree

Hide file tree

Showing 7 changed files with 115 additions and 3 deletions.
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -1,3 +1,15 @@
+2018-11-07  Ali Juma  <ajuma@chromium.org>
+
+        IntersectionObserverEntry doesn't keep JS wrappers of rects alive
+        https://bugs.webkit.org/show_bug.cgi?id=191330
+
+        Reviewed by Chris Dumez.
+
+        Add test coverage.
+
+        * intersection-observer/intersection-observer-entry-keeps-js-wrappers-of-rects-alive-expected.txt: Added.
+        * intersection-observer/intersection-observer-entry-keeps-js-wrappers-of-rects-alive.html: Added.
+
 2018-11-07  Ryan Haddad  <ryanhaddad@apple.com>
 
         Unreviewed, update test expectations for fast/events/pointer.

diff --git a/...ection-observer/intersection-observer-entry-keeps-js-wrappers-of-rects-alive-expected.txt b/...ection-observer/intersection-observer-entry-keeps-js-wrappers-of-rects-alive-expected.txt
@@ -0,0 +1,8 @@
+This tests that JS wrappers of rects in an IntersectionObserverEntry do not get collected.
+
+PASS
+PASS
+PASS
+PASS
+PASS
+
diff --git a/...s/intersection-observer/intersection-observer-entry-keeps-js-wrappers-of-rects-alive.html b/...s/intersection-observer/intersection-observer-entry-keeps-js-wrappers-of-rects-alive.html
@@ -0,0 +1,67 @@
+<!DOCTYPE html>
+<html>
+<body>
+<p>This tests that JS wrappers of rects in an IntersectionObserverEntry do not get collected.</p>
+<pre id="log"></pre>
+<script src="../resources/gc.js"></script>
+<script>
+
+if (window.testRunner)
+    testRunner.dumpAsText();
+
+const targetCount = 5;
+const iterationCount = 5;
+
+for (let i = 0; i < targetCount; ++i) {
+    let target = document.createElement('div');
+    document.body.appendChild(target);
+}
+
+async function observe() {
+    return new Promise((resolve) => {
+        const observer = new IntersectionObserver(entries => {
+            for (const entry of entries) {
+                entry.rootBounds.myState = 'live';
+                entry.boundingClientRect.myState = 'live';
+                entry.intersectionRect.myState = 'live';
+            }
+            resolve(entries);
+            observer.disconnect();
+        });
+        document.querySelectorAll('div').forEach(target => { observer.observe(target); });
+    });
+}
+
+function check(entries) {
+    let deadCount = 0;
+    for (const entry of entries) {
+        if (entry.rootBounds.myState != 'live')
+            deadCount++;
+        if (entry.boundingClientRect.myState != 'live')
+            deadCount++;
+        if (entry.intersectionRect.myState != 'live')
+            deadCount++;
+    }
+    document.getElementById('log').textContent += (deadCount ? `FAIL - ${deadCount} rects lost JS wrappers` : 'PASS') + '\n';
+}
+
+async function runAll() {
+    if (window.testRunner)
+        testRunner.waitUntilDone();
+
+    for (let j = 0; j < iterationCount; ++j) {
+        const entries = await observe();
+        await Promise.resolve();
+        gc();
+        check(entries);
+    }
+
+    if (window.testRunner)
+        testRunner.notifyDone();
+}
+
+runAll();
+
+</script>
+</body>
+</html>
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,24 @@
+2018-11-07  Ali Juma  <ajuma@chromium.org>
+
+        IntersectionObserverEntry doesn't keep JS wrappers of rects alive
+        https://bugs.webkit.org/show_bug.cgi?id=191330
+
+        Reviewed by Chris Dumez.
+
+        Retain wrappers of each rect in an IntersectionObserverEntry as long as the entry's wrapper
+        is alive, by adding these wrappers as opaque roots.
+
+        Test: intersection-observer/intersection-observer-entry-keeps-js-wrappers-of-rects-alive.html
+
+        * bindings/js/JSIntersectionObserverEntryCustom.cpp:
+        (WebCore::JSIntersectionObserverEntry::visitAdditionalChildren):
+        * dom/DOMRectReadOnly.idl:
+        * page/IntersectionObserverEntry.h:
+        (WebCore::IntersectionObserverEntry::rootBounds const): Make this return a raw pointer instead of a RefPtr so that it
+        can be called in JSIntersectionObserverEntry::visitAdditionalChildren, which can be called from non-main threads.
+        (WebCore::IntersectionObserverEntry::boundingClientRect const): Ditto.
+        (WebCore::IntersectionObserverEntry::intersectionRect const): Ditto.
+
 2018-11-07  Simon Fraser  <simon.fraser@apple.com>
 
         TileController::tileSize() should not have side effects

diff --git a/Source/WebCore/bindings/js/JSIntersectionObserverEntryCustom.cpp b/Source/WebCore/bindings/js/JSIntersectionObserverEntryCustom.cpp
@@ -33,6 +33,9 @@ namespace WebCore {
 void JSIntersectionObserverEntry::visitAdditionalChildren(JSC::SlotVisitor& visitor)
 {
     visitor.addOpaqueRoot(root(wrapped().target()));
+    visitor.addOpaqueRoot(wrapped().boundingClientRect());
+    visitor.addOpaqueRoot(wrapped().intersectionRect());
+    visitor.addOpaqueRoot(wrapped().rootBounds());
 }
 
 }
diff --git a/Source/WebCore/dom/DOMRectReadOnly.idl b/Source/WebCore/dom/DOMRectReadOnly.idl
@@ -29,6 +29,7 @@
     Constructor(optional unrestricted double x = 0, optional unrestricted double y = 0,
         optional unrestricted double width = 0, optional unrestricted double height = 0),
     Exposed=(Window,Worker),
+    GenerateIsReachable=Impl,
     ImplementationLacksVTable
 ]
 interface DOMRectReadOnly {

diff --git a/Source/WebCore/page/IntersectionObserverEntry.h b/Source/WebCore/page/IntersectionObserverEntry.h
@@ -57,9 +57,9 @@ class IntersectionObserverEntry : public RefCounted<IntersectionObserverEntry> {
     }
 
     double time() const { return m_time; }
-    RefPtr<DOMRectReadOnly> rootBounds() const { return m_rootBounds; }
-    RefPtr<DOMRectReadOnly> boundingClientRect() const { return m_boundingClientRect; }
-    RefPtr<DOMRectReadOnly> intersectionRect() const { return m_intersectionRect; }
+    DOMRectReadOnly* rootBounds() const { return m_rootBounds.get(); }
+    DOMRectReadOnly* boundingClientRect() const { return m_boundingClientRect.get(); }
+    DOMRectReadOnly* intersectionRect() const { return m_intersectionRect.get(); }
     Element* target() const { return m_target.get(); }
 
     bool isIntersecting() const { return m_isIntersecting; }