Attachment 343205 Details for Bug 186873 – Patch

[patch] Patch

bug-186873-20180620200404.patch (text/plain), 13.91 KB, created by Chris Dumez on 2018-06-20 20:04:05 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2018-06-20 20:04:05 PDT

Size: 13.91 KB

patch

obsolete

>Subversion Revision: 233021
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 339fb6709686c38d32f589a585893d86158e191f..129d828015c6343b8a207aa968f30924276f4ce7 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,42 @@
>+2018-06-20  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 NOBODY (OOPS!).
>+
>+        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.
>+
>+        Test: performance-api/performance-observer-callback-after-gc.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..c77e64d06cf6a7c3332494f28b00854d792e1415 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,17 @@
>+2018-06-20  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 NOBODY (OOPS!).
>+
>+        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-callback-after-gc-expected.txt: Added.
>+        * performance-api/performance-observer-callback-after-gc.html: Added.
>+
> 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>

Actions: View | Formatted Diff | Diff

Attachments on bug 186873: 343205 | 343247