Attachment 343247 Details for Bug 186873 – Patch

[patch] Patch

bug-186873-20180621105255.patch (text/plain), 16.95 KB, created by Chris Dumez on 2018-06-21 10:52:56 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2018-06-21 10:52:56 PDT

Size: 16.95 KB

patch

obsolete

>Subversion Revision: 233021
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 339fb6709686c38d32f589a585893d86158e191f..1d1b6dc1dbd898746bfc976a84f8c11eda38d65f 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -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  Simon Fraser  <simon.fraser@apple.com>
> 
>         AnimationList wastes 60KB of vector capacity
>diff --git a/Source/WebCore/Sources.txt b/Source/WebCore/Sources.txt
>index 8b02601cb890da9fc4e1d0b9b7d8c34a590e29f3..958df37b77ad6e6bf30fda265bdee9f034d396ab 100644
>--- a/Source/WebCore/Sources.txt
>+++ b/Source/WebCore/Sources.txt
>@@ -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
>diff --git a/Source/WebCore/WebCore.xcodeproj/project.pbxproj b/Source/WebCore/WebCore.xcodeproj/project.pbxproj
>index 4566d7cd34498c159d2454d92160e2d72eeb6d7b..747216060b1081e5088ca87b67161d2cff3ac49a 100644
>--- a/Source/WebCore/WebCore.xcodeproj/project.pbxproj
>+++ b/Source/WebCore/WebCore.xcodeproj/project.pbxproj
>@@ -9818,6 +9818,7 @@
> 		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>"; };
>@@ -19824,6 +19825,7 @@
> 				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
>index 0000000000000000000000000000000000000000..37e95171122ddda05dd376b61eea1f8e1d3c729f
>--- /dev/null
>+++ b/Source/WebCore/bindings/js/JSPerformanceObserverCustom.cpp
>@@ -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();
>+}
>+
>+}
>diff --git a/Source/WebCore/bindings/js/ScriptController.cpp b/Source/WebCore/bindings/js/ScriptController.cpp
>index 988b68a8b18acfb9237c662183ab674dc061957a..ce4c4e095f69cdf044112c190bca135d911cceb6 100644
>--- a/Source/WebCore/bindings/js/ScriptController.cpp
>+++ b/Source/WebCore/bindings/js/ScriptController.cpp
>@@ -23,6 +23,7 @@
> 
> #include "BridgeJSC.h"
> #include "CachedScriptFetcher.h"
>+#include "CommonVM.h"
> #include "ContentSecurityPolicy.h"
> #include "DocumentLoader.h"
> #include "Event.h"
>diff --git a/Source/WebCore/page/PerformanceObserver.cpp b/Source/WebCore/page/PerformanceObserver.cpp
>index 4b7686bcf9e71cfe49437a6ef5487d81b8058085..a7037ca5f653abcb57f670a8c12480742e345312 100644
>--- a/Source/WebCore/page/PerformanceObserver.cpp
>+++ b/Source/WebCore/page/PerformanceObserver.cpp
>@@ -51,6 +51,7 @@ PerformanceObserver::PerformanceObserver(ScriptExecutionContext& scriptExecution
> void PerformanceObserver::disassociate()
> {
>     m_performance = nullptr;
>+    m_registered = false;
> }
> 
> ExceptionOr<void> PerformanceObserver::observe(Init&& init)
>diff --git a/Source/WebCore/page/PerformanceObserver.h b/Source/WebCore/page/PerformanceObserver.h
>index afc5dfa8eea3a0ccc6dd86807c23ec55f2aadfb0..8ab101656d74f57d66a9b53d12963573a313e32d 100644
>--- a/Source/WebCore/page/PerformanceObserver.h
>+++ b/Source/WebCore/page/PerformanceObserver.h
>@@ -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>&&);
> 
>diff --git a/Source/WebCore/page/PerformanceObserver.idl b/Source/WebCore/page/PerformanceObserver.idl
>index a9b0079becd5457ba76a257149bd433646a4119c..f4914be755e21accb400c00ecbb1ca642628ce46 100644
>--- a/Source/WebCore/page/PerformanceObserver.idl
>+++ b/Source/WebCore/page/PerformanceObserver.idl
>@@ -28,9 +28,11 @@
> [
>     Constructor(PerformanceObserverCallback callback),
>     ConstructorCallWith=ScriptExecutionContext,
>+    CustomIsReachable,
>     EnabledAtRuntime=PerformanceTimeline,
>     Exposed=(Window,Worker),
>     ImplementationLacksVTable,
>+    JSCustomMarkFunction,
> ] interface PerformanceObserver {
>     [MayThrowException] void observe(PerformanceObserverInit options);
>     void disconnect();
>diff --git a/Source/WebCore/page/PerformanceObserverCallback.h b/Source/WebCore/page/PerformanceObserverCallback.h
>index 5cad8b0232e6928edbc7aab340ad072d36537ab4..b59c3c6ed9fa79a7efd2fc0ea801f3fadf5ae24b 100644
>--- a/Source/WebCore/page/PerformanceObserverCallback.h
>+++ b/Source/WebCore/page/PerformanceObserverCallback.h
>@@ -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;
> };
> 
>diff --git a/Source/WebCore/page/PerformanceObserverCallback.idl b/Source/WebCore/page/PerformanceObserverCallback.idl
>index 96629899c9620dc2d2d2769a2fb29d97829fe392..1d1fbc2d8b252612f45eecfdf4a5bbd3f3180c38 100644
>--- a/Source/WebCore/page/PerformanceObserverCallback.idl
>+++ b/Source/WebCore/page/PerformanceObserverCallback.idl
>@@ -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);
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index be753824bda2a0c0e5f35b70494ac98fc99159c1..ab9f9bf527102130d1d4f83aee6f4e05cce9567a 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -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  Alicia Boya GarcÃa  <aboya@igalia.com>
> 
>         Unreviewed GTK+ test gardening.
>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
>index 0000000000000000000000000000000000000000..7db02f9f7ae436f7c6a27e82887b63c22c925db3
>--- /dev/null
>+++ b/LayoutTests/performance-api/performance-observer-callback-after-gc-expected.txt
>@@ -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
>index 0000000000000000000000000000000000000000..00af012ee147092eeb450501f9c853526a3e550f
>--- /dev/null
>+++ b/LayoutTests/performance-api/performance-observer-callback-after-gc.html
>@@ -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
>index 0000000000000000000000000000000000000000..4d32cbbfc5085b9167242450a8d7cb50540ce5f3
>--- /dev/null
>+++ b/LayoutTests/performance-api/performance-observer-no-document-leak-expected.txt
>@@ -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
>index 0000000000000000000000000000000000000000..ae341f8cdf5febb5280134692362e99c7cc21be9
>--- /dev/null
>+++ b/LayoutTests/performance-api/performance-observer-no-document-leak.html
>@@ -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
>index 0000000000000000000000000000000000000000..9ecdab296220483637377531952888b55cd4229d
>--- /dev/null
>+++ b/LayoutTests/performance-api/resources/performance-observer-no-document-leak-frame.html
>@@ -0,0 +1,3 @@
>+<script>
>+p = new PerformanceObserver(function() { });
>+</script>

Actions: View | Formatted Diff | Diff

Attachments on bug 186873: 343205 | 343247