Attachment 346581 Details for Bug 172575 – Fixes the bug

[patch] Fixes the bug

bug-172575-20180803211313.patch (text/plain), 8.38 KB, created by Ryosuke Niwa on 2018-08-03 21:13:14 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Ryosuke Niwa

Created: 2018-08-03 21:13:14 PDT

Size: 8.38 KB

patch

obsolete

>Index: Source/WebCore/ChangeLog
>===================================================================
>--- Source/WebCore/ChangeLog	(revision 234575)
>+++ Source/WebCore/ChangeLog	(working copy)
>@@ -1,3 +1,26 @@
>+2018-08-03  Ryosuke Niwa  <rniwa@webkit.org>
>+
>+        Properties set on window.customElements can disappear due to GC
>+        https://bugs.webkit.org/show_bug.cgi?id=172575
>+        <rdar://problem/32440668>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Fixed the bug that JS wrapper of CustomElementsRegistry can erroneously get collected during GC
>+        by keeping it alive as long as the global object is alive.
>+
>+        Test: fast/custom-elements/custom-element-registry-wrapper-should-stay-alive.html
>+
>+        * dom/CustomElementRegistry.cpp:
>+        (WebCore::CustomElementRegistry::create):
>+        (WebCore::CustomElementRegistry::CustomElementRegistry):
>+        * dom/CustomElementRegistry.h:
>+        (WebCore::CustomElementRegistry): Make this inherited from ContextDestructionObserver.
>+        * dom/CustomElementRegistry.idl: Set GenerateIsReachable=ImplScriptExecutionContext in IDL. This will
>+        make CustomElementRegistry reachable from the global object.
>+        * page/DOMWindow.cpp:
>+        (WebCore::DOMWindow::ensureCustomElementRegistry):
>+
> 2018-08-03  Ben Richards  <benton_richards@apple.com>
> 
>         We should cache the compiled sandbox profile in a data vault
>Index: Source/WebCore/dom/CustomElementRegistry.cpp
>===================================================================
>--- Source/WebCore/dom/CustomElementRegistry.cpp	(revision 234575)
>+++ Source/WebCore/dom/CustomElementRegistry.cpp	(working copy)
>@@ -41,13 +41,14 @@
> 
> namespace WebCore {
> 
>-Ref<CustomElementRegistry> CustomElementRegistry::create(DOMWindow& window)
>+Ref<CustomElementRegistry> CustomElementRegistry::create(DOMWindow& window, ScriptExecutionContext* scriptExecutionContext)
> {
>-    return adoptRef(*new CustomElementRegistry(window));
>+    return adoptRef(*new CustomElementRegistry(window, scriptExecutionContext));
> }
> 
>-CustomElementRegistry::CustomElementRegistry(DOMWindow& window)
>-    : m_window(window)
>+CustomElementRegistry::CustomElementRegistry(DOMWindow& window, ScriptExecutionContext* scriptExecutionContext)
>+    : ContextDestructionObserver(scriptExecutionContext)
>+    , m_window(window)
> {
> }
> 
>Index: Source/WebCore/dom/CustomElementRegistry.h
>===================================================================
>--- Source/WebCore/dom/CustomElementRegistry.h	(revision 234575)
>+++ Source/WebCore/dom/CustomElementRegistry.h	(working copy)
>@@ -25,6 +25,7 @@
> 
> #pragma once
> 
>+#include "ContextDestructionObserver.h"
> #include "QualifiedName.h"
> #include <wtf/HashMap.h>
> #include <wtf/text/AtomicString.h>
>@@ -47,9 +48,9 @@ class JSCustomElementInterface;
> class Node;
> class QualifiedName;
> 
>-class CustomElementRegistry : public RefCounted<CustomElementRegistry> {
>+class CustomElementRegistry : public RefCounted<CustomElementRegistry>, public ContextDestructionObserver {
> public:
>-    static Ref<CustomElementRegistry> create(DOMWindow&);
>+    static Ref<CustomElementRegistry> create(DOMWindow&, ScriptExecutionContext*);
>     ~CustomElementRegistry();
> 
>     void addElementDefinition(Ref<JSCustomElementInterface>&&);
>@@ -68,7 +69,7 @@ public:
>     HashMap<AtomicString, Ref<DeferredPromise>>& promiseMap() { return m_promiseMap; }
> 
> private:
>-    CustomElementRegistry(DOMWindow&);
>+    CustomElementRegistry(DOMWindow&, ScriptExecutionContext*);
> 
>     DOMWindow& m_window;
>     HashMap<AtomicString, Ref<JSCustomElementInterface>> m_nameMap;
>Index: Source/WebCore/dom/CustomElementRegistry.idl
>===================================================================
>--- Source/WebCore/dom/CustomElementRegistry.idl	(revision 234575)
>+++ Source/WebCore/dom/CustomElementRegistry.idl	(working copy)
>@@ -25,8 +25,8 @@
> 
> [
>     EnabledAtRuntime=CustomElements,
>-    ImplementationLacksVTable,
>     JSGenerateToNativeObject,
>+    GenerateIsReachable=ImplScriptExecutionContext
> ] interface CustomElementRegistry {
>     [CEReactions, Custom] void define(DOMString name, Function constructor);
>     any get(DOMString name);
>Index: Source/WebCore/page/DOMWindow.cpp
>===================================================================
>--- Source/WebCore/page/DOMWindow.cpp	(revision 234575)
>+++ Source/WebCore/page/DOMWindow.cpp	(working copy)
>@@ -610,7 +610,7 @@ bool DOMWindow::isCurrentlyDisplayedInFr
> CustomElementRegistry& DOMWindow::ensureCustomElementRegistry()
> {
>     if (!m_customElementRegistry)
>-        m_customElementRegistry = CustomElementRegistry::create(*this);
>+        m_customElementRegistry = CustomElementRegistry::create(*this, scriptExecutionContext());
>     return *m_customElementRegistry;
> }
> 
>Index: LayoutTests/ChangeLog
>===================================================================
>--- LayoutTests/ChangeLog	(revision 234575)
>+++ LayoutTests/ChangeLog	(working copy)
>@@ -1,3 +1,16 @@
>+2018-08-03  Ryosuke Niwa  <rniwa@webkit.org>
>+
>+        Properties set on window.customElements can disappear due to GC
>+        https://bugs.webkit.org/show_bug.cgi?id=172575
>+        <rdar://problem/32440668>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Added a regression test.
>+
>+        * fast/custom-elements/custom-element-registry-wrapper-should-stay-alive-expected.txt: Added.
>+        * fast/custom-elements/custom-element-registry-wrapper-should-stay-alive.html: Added.
>+
> 2018-08-03  Justin Fan  <justin_fan@apple.com>
> 
>         WebGL 2 conformance: vertex_arrays/vertex_array_object.html
>Index: LayoutTests/fast/custom-elements/custom-element-registry-wrapper-should-stay-alive-expected.txt
>===================================================================
>--- LayoutTests/fast/custom-elements/custom-element-registry-wrapper-should-stay-alive-expected.txt	(nonexistent)
>+++ LayoutTests/fast/custom-elements/custom-element-registry-wrapper-should-stay-alive-expected.txt	(working copy)
>@@ -0,0 +1,19 @@
>+This tests that the property added on window.customElements persist after a lot of memory allocation
>+
>+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
>+
>+
>+PASS The property was present - the JS wrapper of customElements was not collected
>+PASS The property was present - the JS wrapper of customElements was not collected
>+PASS The property was present - the JS wrapper of customElements was not collected
>+PASS The property was present - the JS wrapper of customElements was not collected
>+PASS The property was present - the JS wrapper of customElements was not collected
>+PASS The property was present - the JS wrapper of customElements was not collected
>+PASS The property was present - the JS wrapper of customElements was not collected
>+PASS The property was present - the JS wrapper of customElements was not collected
>+PASS The property was present - the JS wrapper of customElements was not collected
>+PASS The property was present - the JS wrapper of customElements was not collected
>+PASS successfullyParsed is true
>+
>+TEST COMPLETE
>+
>Index: LayoutTests/fast/custom-elements/custom-element-registry-wrapper-should-stay-alive.html
>===================================================================
>--- LayoutTests/fast/custom-elements/custom-element-registry-wrapper-should-stay-alive.html	(nonexistent)
>+++ LayoutTests/fast/custom-elements/custom-element-registry-wrapper-should-stay-alive.html	(working copy)
>@@ -0,0 +1,33 @@
>+<!DOCTYPE html>
>+<html>
>+<body>
>+<script src="../../resources/js-test.js"></script>
>+<script>
>+
>+description('This tests that the property added on window.customElements persist after a lot of memory allocation');
>+
>+for (let i = 0; i < 10; i++) {
>+    // Using iframe makes this test more reliable.
>+    const iframe = document.createElement('iframe');
>+    document.body.appendChild(iframe);
>+    iframe.contentWindow.eval(`
>+        window.customElements.someProperty = 'storedValue';
>+        const a = [];
>+        for (let i = 0; i < 1000000; i++)
>+            a.push({});
>+        if (window.GCController)
>+            GCController.collect();
>+        top.check(window.customElements.someProperty);`);
>+    iframe.remove();
>+}
>+
>+function check(value) {
>+    if (value == 'storedValue')
>+        testPassed('The property was present - the JS wrapper of customElements was not collected');
>+    else
>+        testFailed('The property was not present - the JS wrapper of customElements was erroneously collected');
>+}
>+
>+</script>
>+</body>
>+</html>
>\ No newline at end of file

Flags:

saam: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 172575: 311187 | 311188 | 311189 | 346581