JSPerformanceObserverCallback creates a GC strongly-referenced Function that is never...
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 21 Jun 2018 19:04:02 +0000 (19:04 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 21 Jun 2018 19:04:02 +0000 (19:04 +0000)
https://bugs.webkit.org/show_bug.cgi?id=186873
<rdar://problem/41271574>

Reviewed by Simon Fraser.

Source/WebCore:

Add [IsWeakCallback] to PerformanceObserverCallback interface so that the generated
JSPerformanceObserverCallback uses a JSC::Weak instead of a JSC::Strong to store the
js function. To keep the function alive, add [JSCustomMarkFunction] to PerformanceObserver
interface and have its visitAdditionalChildren() visit the callback's js function.
Finally, because we want the callback to still be called even if the JS does not keep
the PerformanceObserver wrapper alive, add [CustomIsReachable] to PerformanceObserver
interface and have its isReachableFromOpaqueRoots() return true if the observer is
registered (i.e. it may need to call the callback in the future).

I have confirmed locally, that the Performance / PerformanceObserver / Document
objects properly get destroyed if I navigate away from a page that had a performance
observer and trigger a memory pressure warning. Also,
`notifyutil -p com.apple.WebKit.showAllDocuments` no longer shows the old document.

Tests: performance-api/performance-observer-callback-after-gc.html
       performance-api/performance-observer-no-document-leak.html

* Sources.txt:
* WebCore.xcodeproj/project.pbxproj:
* bindings/js/JSPerformanceObserverCustom.cpp: Added.
(WebCore::JSPerformanceObserver::visitAdditionalChildren):
(WebCore::JSPerformanceObserverOwner::isReachableFromOpaqueRoots):
* bindings/js/ScriptController.cpp:
* page/PerformanceObserver.cpp:
(WebCore::PerformanceObserver::disassociate):
* page/PerformanceObserver.h:
(WebCore::PerformanceObserver::isRegistered const):
(WebCore::PerformanceObserver::callback):
* page/PerformanceObserver.idl:
* page/PerformanceObserverCallback.h:
* page/PerformanceObserverCallback.idl:

LayoutTests:

* performance-api/performance-observer-callback-after-gc-expected.txt: Added.
* performance-api/performance-observer-callback-after-gc.html: Added.
Add layout test to make sure that a performance observer's callback still gets called, even if
the JS does not keep the performance observer alive.

* performance-api/performance-observer-no-document-leak-expected.txt: Added.
* performance-api/performance-observer-no-document-leak.html: Added.
* performance-api/resources/performance-observer-no-document-leak-frame.html: Added.
Add layout test coverage to make sure the document does not leak if PerformanceObserver was
used.

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

16 files changed:
LayoutTests/ChangeLog
LayoutTests/performance-api/performance-observer-callback-after-gc-expected.txt [new file with mode: 0644]
LayoutTests/performance-api/performance-observer-callback-after-gc.html [new file with mode: 0644]
LayoutTests/performance-api/performance-observer-no-document-leak-expected.txt [new file with mode: 0644]
LayoutTests/performance-api/performance-observer-no-document-leak.html [new file with mode: 0644]
LayoutTests/performance-api/resources/performance-observer-no-document-leak-frame.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/Sources.txt
Source/WebCore/WebCore.xcodeproj/project.pbxproj
Source/WebCore/bindings/js/JSPerformanceObserverCustom.cpp [new file with mode: 0644]
Source/WebCore/bindings/js/ScriptController.cpp
Source/WebCore/page/PerformanceObserver.cpp
Source/WebCore/page/PerformanceObserver.h
Source/WebCore/page/PerformanceObserver.idl
Source/WebCore/page/PerformanceObserverCallback.h
Source/WebCore/page/PerformanceObserverCallback.idl

index 6af8aeb..644b505 100644 (file)
@@ -1,3 +1,22 @@
+2018-06-21  Chris Dumez  <cdumez@apple.com>
+
+        JSPerformanceObserverCallback creates a GC strongly-referenced Function that is never cleaned up
+        https://bugs.webkit.org/show_bug.cgi?id=186873
+        <rdar://problem/41271574>
+
+        Reviewed by Simon Fraser.
+
+        * performance-api/performance-observer-callback-after-gc-expected.txt: Added.
+        * performance-api/performance-observer-callback-after-gc.html: Added.
+        Add layout test to make sure that a performance observer's callback still gets called, even if
+        the JS does not keep the performance observer alive.
+
+        * performance-api/performance-observer-no-document-leak-expected.txt: Added.
+        * performance-api/performance-observer-no-document-leak.html: Added.
+        * performance-api/resources/performance-observer-no-document-leak-frame.html: Added.
+        Add layout test coverage to make sure the document does not leak if PerformanceObserver was
+        used.
+
 2018-06-20  Antoine Quint  <graouts@apple.com>
 
         [Web Animations] Make imported/mozilla/css-animations/test_animation-ready.html pass reliably
diff --git a/LayoutTests/performance-api/performance-observer-callback-after-gc-expected.txt b/LayoutTests/performance-api/performance-observer-callback-after-gc-expected.txt
new file mode 100644 (file)
index 0000000..7db02f9
--- /dev/null
@@ -0,0 +1,14 @@
+Ensure PerformanceObserver callback fires even if the JS does not keep the PerformanceObserver object alive.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PerformanceObserver callback fired: 1 entries
+PASS mark1
+PerformanceObserver callback fired: 2 entries
+PASS mark2
+PASS mark3
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/performance-api/performance-observer-callback-after-gc.html b/LayoutTests/performance-api/performance-observer-callback-after-gc.html
new file mode 100644 (file)
index 0000000..00af012
--- /dev/null
@@ -0,0 +1,37 @@
+<!DOCTYPE HTML>
+<html>
+<head>
+<script src="../resources/js-test-pre.js"></script>
+</head>
+<body>
+<script>
+description("Ensure PerformanceObserver callback fires even if the JS does not keep the PerformanceObserver object alive.");
+window.jsTestIsAsync = true;
+
+let shouldEnd = false;
+
+let observer = new PerformanceObserver((list) => {
+    debug("PerformanceObserver callback fired: " + list.getEntries().length + " entries");
+    for (let mark of list.getEntries())
+        testPassed(mark.name);
+    if (shouldEnd)
+        finishJSTest();
+});
+observer.observe({entryTypes: ["mark"]});
+observer = null;
+gc();
+
+// ---
+
+performance.mark("mark1");
+
+setTimeout(() => {
+    gc();
+    performance.mark("mark2");
+    performance.mark("mark3");
+    shouldEnd = true;
+}, 50);
+</script>
+<script src="../resources/js-test-post.js"></script>
+</body>
+</html>
diff --git a/LayoutTests/performance-api/performance-observer-no-document-leak-expected.txt b/LayoutTests/performance-api/performance-observer-no-document-leak-expected.txt
new file mode 100644 (file)
index 0000000..4d32cbb
--- /dev/null
@@ -0,0 +1,12 @@
+Tests that using PerformanceObserver does not cause the document to get leaked.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS Number of document is 2
+PASS Number of document is 1
+PASS Document did not leak
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/performance-api/performance-observer-no-document-leak.html b/LayoutTests/performance-api/performance-observer-no-document-leak.html
new file mode 100644 (file)
index 0000000..ae341f8
--- /dev/null
@@ -0,0 +1,38 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src="../resources/js-test-pre.js"></script>
+</head>
+<body>
+<iframe id="testFrame" src="resources/performance-observer-no-document-leak-frame.html"></iframe>
+<script>
+description("Tests that using PerformanceObserver does not cause the document to get leaked.");
+window.jsTestIsAsync = true;
+
+function numberOfDocumentsShouldBecome(count)
+{
+    return new Promise(function(resolve, reject) {
+        gc();
+        handle = setInterval(function() {
+            if (internals.numberOfLiveDocuments() == count) {
+                testPassed("Number of document is " + count);
+                clearInterval(handle);
+                resolve();
+            }
+        }, 10);
+    });
+}
+
+onload = function() {
+    numberOfDocumentsShouldBecome(2).then(function() {
+        testFrame.remove();
+        numberOfDocumentsShouldBecome(1).then(function() {
+            testPassed("Document did not leak");
+            finishJSTest();
+        });
+    });
+}
+</script>
+<script src="../resources/js-test-post.js"></script>
+</body>
+</html>
diff --git a/LayoutTests/performance-api/resources/performance-observer-no-document-leak-frame.html b/LayoutTests/performance-api/resources/performance-observer-no-document-leak-frame.html
new file mode 100644 (file)
index 0000000..9ecdab2
--- /dev/null
@@ -0,0 +1,3 @@
+<script>
+p = new PerformanceObserver(function() { });
+</script>
index 0eea90d..9f84ca1 100644 (file)
@@ -1,3 +1,43 @@
+2018-06-21  Chris Dumez  <cdumez@apple.com>
+
+        JSPerformanceObserverCallback creates a GC strongly-referenced Function that is never cleaned up
+        https://bugs.webkit.org/show_bug.cgi?id=186873
+        <rdar://problem/41271574>
+
+        Reviewed by Simon Fraser.
+
+        Add [IsWeakCallback] to PerformanceObserverCallback interface so that the generated
+        JSPerformanceObserverCallback uses a JSC::Weak instead of a JSC::Strong to store the
+        js function. To keep the function alive, add [JSCustomMarkFunction] to PerformanceObserver
+        interface and have its visitAdditionalChildren() visit the callback's js function.
+        Finally, because we want the callback to still be called even if the JS does not keep
+        the PerformanceObserver wrapper alive, add [CustomIsReachable] to PerformanceObserver
+        interface and have its isReachableFromOpaqueRoots() return true if the observer is
+        registered (i.e. it may need to call the callback in the future).
+
+        I have confirmed locally, that the Performance / PerformanceObserver / Document
+        objects properly get destroyed if I navigate away from a page that had a performance
+        observer and trigger a memory pressure warning. Also,
+        `notifyutil -p com.apple.WebKit.showAllDocuments` no longer shows the old document.
+
+        Tests: performance-api/performance-observer-callback-after-gc.html
+               performance-api/performance-observer-no-document-leak.html
+
+        * Sources.txt:
+        * WebCore.xcodeproj/project.pbxproj:
+        * bindings/js/JSPerformanceObserverCustom.cpp: Added.
+        (WebCore::JSPerformanceObserver::visitAdditionalChildren):
+        (WebCore::JSPerformanceObserverOwner::isReachableFromOpaqueRoots):
+        * bindings/js/ScriptController.cpp:
+        * page/PerformanceObserver.cpp:
+        (WebCore::PerformanceObserver::disassociate):
+        * page/PerformanceObserver.h:
+        (WebCore::PerformanceObserver::isRegistered const):
+        (WebCore::PerformanceObserver::callback):
+        * page/PerformanceObserver.idl:
+        * page/PerformanceObserverCallback.h:
+        * page/PerformanceObserverCallback.idl:
+
 2018-06-20  Antoine Quint  <graouts@apple.com>
 
         [Web Animations] Make imported/mozilla/css-animations/test_animation-ready.html pass reliably
index 8b02601..958df37 100644 (file)
@@ -428,6 +428,7 @@ bindings/js/JSNodeIteratorCustom.cpp
 bindings/js/JSNodeListCustom.cpp
 bindings/js/JSOffscreenCanvasRenderingContext2DCustom.cpp
 bindings/js/JSPerformanceEntryCustom.cpp
+bindings/js/JSPerformanceObserverCustom.cpp
 bindings/js/JSPluginElementFunctions.cpp
 bindings/js/JSPopStateEventCustom.cpp
 bindings/js/JSReadableStreamPrivateConstructors.cpp
index 4566d7c..7472160 100644 (file)
                833B9E2D1F508D8000E0E428 /* JSFileSystemFileEntry.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = JSFileSystemFileEntry.cpp; sourceTree = "<group>"; };
                833B9E2E1F508D8000E0E428 /* JSFileSystemEntry.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = JSFileSystemEntry.cpp; sourceTree = "<group>"; };
                833B9E2F1F508D8000E0E428 /* JSFileSystemDirectoryEntry.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = JSFileSystemDirectoryEntry.h; sourceTree = "<group>"; };
+               833CF70F20DB3F5F00141BCC /* JSPerformanceObserverCustom.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = JSPerformanceObserverCustom.cpp; sourceTree = "<group>"; };
                83407FC01E8D9C1200E048D3 /* VisibilityChangeClient.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = VisibilityChangeClient.h; sourceTree = "<group>"; };
                8348BFA91B85729500912F36 /* ClassCollection.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = ClassCollection.cpp; sourceTree = "<group>"; };
                8348BFAA1B85729500912F36 /* ClassCollection.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = ClassCollection.h; sourceTree = "<group>"; };
                                AD20B18C18E9D216005A8083 /* JSNodeListCustom.h */,
                                3140C52B1FE06B4900D2A873 /* JSOffscreenCanvasRenderingContext2DCustom.cpp */,
                                CB38FD551CD21D5B00592A3F /* JSPerformanceEntryCustom.cpp */,
+                               833CF70F20DB3F5F00141BCC /* JSPerformanceObserverCustom.cpp */,
                                83F572941FA1066F003837BE /* JSServiceWorkerClientCustom.cpp */,
                                460D19441FCE21DD00C3DB85 /* JSServiceWorkerGlobalScopeCustom.cpp */,
                                BC98A27C0C0C9950004BEBF7 /* JSStyleSheetCustom.cpp */,
diff --git a/Source/WebCore/bindings/js/JSPerformanceObserverCustom.cpp b/Source/WebCore/bindings/js/JSPerformanceObserverCustom.cpp
new file mode 100644 (file)
index 0000000..37e9517
--- /dev/null
@@ -0,0 +1,43 @@
+/*
+ * Copyright (C) 2018 Apple Inc. All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY APPLE INC. ``AS IS'' AND ANY
+ * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
+ * PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL APPLE INC. OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
+ * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
+ * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
+ * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY
+ * OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include "config.h"
+#include "JSPerformanceObserver.h"
+
+#include "PerformanceObserverCallback.h"
+
+namespace WebCore {
+
+void JSPerformanceObserver::visitAdditionalChildren(JSC::SlotVisitor& visitor)
+{
+    wrapped().callback().visitJSFunction(visitor);
+}
+
+bool JSPerformanceObserverOwner::isReachableFromOpaqueRoots(JSC::Handle<JSC::Unknown> handle, void*, SlotVisitor&)
+{
+    return jsCast<JSPerformanceObserver*>(handle.slot()->asCell())->wrapped().isRegistered();
+}
+
+}
index 988b68a..ce4c4e0 100644 (file)
@@ -23,6 +23,7 @@
 
 #include "BridgeJSC.h"
 #include "CachedScriptFetcher.h"
+#include "CommonVM.h"
 #include "ContentSecurityPolicy.h"
 #include "DocumentLoader.h"
 #include "Event.h"
index 4b7686b..a7037ca 100644 (file)
@@ -51,6 +51,7 @@ PerformanceObserver::PerformanceObserver(ScriptExecutionContext& scriptExecution
 void PerformanceObserver::disassociate()
 {
     m_performance = nullptr;
+    m_registered = false;
 }
 
 ExceptionOr<void> PerformanceObserver::observe(Init&& init)
index afc5dfa..8ab1016 100644 (file)
@@ -59,6 +59,9 @@ public:
     void queueEntry(PerformanceEntry&);
     void deliver();
 
+    bool isRegistered() const { return m_registered; }
+    PerformanceObserverCallback& callback() { return m_callback.get(); }
+
 private:
     PerformanceObserver(ScriptExecutionContext&, Ref<PerformanceObserverCallback>&&);
 
index a9b0079..f4914be 100644 (file)
 [
     Constructor(PerformanceObserverCallback callback),
     ConstructorCallWith=ScriptExecutionContext,
+    CustomIsReachable,
     EnabledAtRuntime=PerformanceTimeline,
     Exposed=(Window,Worker),
     ImplementationLacksVTable,
+    JSCustomMarkFunction,
 ] interface PerformanceObserver {
     [MayThrowException] void observe(PerformanceObserverInit options);
     void disconnect();
index 5cad8b0..b59c3c6 100644 (file)
@@ -38,6 +38,8 @@ class PerformanceObserverCallback : public RefCounted<PerformanceObserverCallbac
 public:
     using ActiveDOMCallback::ActiveDOMCallback;
 
+    virtual bool hasCallback() const = 0;
+
     virtual CallbackResult<void> handleEvent(PerformanceObserverEntryList&, PerformanceObserver&) = 0;
 };
 
index 9662989..1d1fbc2 100644 (file)
@@ -25,4 +25,6 @@
 
 // https://w3c.github.io/performance-timeline/
 
-callback PerformanceObserverCallback = void (PerformanceObserverEntryList entries, PerformanceObserver observer);
+[
+    IsWeakCallback,
+] callback PerformanceObserverCallback = void (PerformanceObserverEntryList entries, PerformanceObserver observer);