DOMWindow properties may get GC'd before their Window object
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 2 Aug 2019 16:43:56 +0000 (16:43 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 2 Aug 2019 16:43:56 +0000 (16:43 +0000)
https://bugs.webkit.org/show_bug.cgi?id=200359

Reviewed by Ryosuke Niwa.

Source/WebCore:

DOMWindow properties may get GC'd before their Window object once their frame is detached. This
is unexpected behavior given that these properties persist on the Window after the frame is
detached. This patch thus updates their bindings so that they live as long as their window, not
their frame.

Note that this also fixes a thread-safety issue since DOMWindowProperty::frame() would get called
from GC threads, although its implementation looks like:
"""
  return m_window ? m_window->frame() : nullptr;
"""

Because m_window is a WeakPtr<DOMWindow> and because windows get destroyed on the main thread,
we could in theory crash when dereferencing m_window->frame() from the GC thread.

Test: fast/dom/dom-window-property-gc-after-frame-detach.html

* bindings/js/JSDOMWindowCustom.cpp:
(WebCore::JSDOMWindow::visitAdditionalChildren):
* bindings/scripts/CodeGeneratorJS.pm:
(GenerateImplementation):
* bindings/scripts/IDLAttributes.json:
* css/StyleMedia.idl:
* loader/appcache/DOMApplicationCache.idl:
* page/BarProp.idl:
* page/DOMSelection.idl:
* page/History.idl:
* page/Location.idl:
* page/Navigator.idl:
* page/Screen.idl:
* page/VisualViewport.idl:
* plugins/DOMMimeTypeArray.idl:
* plugins/DOMPluginArray.idl:
* storage/Storage.idl:

LayoutTests:

Add layout test coverage.

* fast/dom/dom-window-property-gc-after-frame-detach-expected.txt: Added.
* fast/dom/dom-window-property-gc-after-frame-detach.html: Added.

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

19 files changed:
LayoutTests/ChangeLog
LayoutTests/fast/dom/dom-window-property-gc-after-frame-detach-expected.txt [new file with mode: 0644]
LayoutTests/fast/dom/dom-window-property-gc-after-frame-detach.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/bindings/js/JSDOMWindowCustom.cpp
Source/WebCore/bindings/scripts/CodeGeneratorJS.pm
Source/WebCore/bindings/scripts/IDLAttributes.json
Source/WebCore/css/StyleMedia.idl
Source/WebCore/loader/appcache/DOMApplicationCache.idl
Source/WebCore/page/BarProp.idl
Source/WebCore/page/DOMSelection.idl
Source/WebCore/page/History.idl
Source/WebCore/page/Location.idl
Source/WebCore/page/Navigator.idl
Source/WebCore/page/Screen.idl
Source/WebCore/page/VisualViewport.idl
Source/WebCore/plugins/DOMMimeTypeArray.idl
Source/WebCore/plugins/DOMPluginArray.idl
Source/WebCore/storage/Storage.idl

index d4c4d0f..1d528c3 100644 (file)
@@ -1,3 +1,15 @@
+2019-08-02  Chris Dumez  <cdumez@apple.com>
+
+        DOMWindow properties may get GC'd before their Window object
+        https://bugs.webkit.org/show_bug.cgi?id=200359
+
+        Reviewed by Ryosuke Niwa.
+
+        Add layout test coverage.
+
+        * fast/dom/dom-window-property-gc-after-frame-detach-expected.txt: Added.
+        * fast/dom/dom-window-property-gc-after-frame-detach.html: Added.
+
 2019-08-02  Carlos Garcia Campos  <cgarcia@igalia.com>
 
         [SOUP] WebSockets: use SOUP_WEBSOCKET_CLOSE_NO_STATUS when closing with no status
diff --git a/LayoutTests/fast/dom/dom-window-property-gc-after-frame-detach-expected.txt b/LayoutTests/fast/dom/dom-window-property-gc-after-frame-detach-expected.txt
new file mode 100644 (file)
index 0000000..c792fc2
--- /dev/null
@@ -0,0 +1,44 @@
+Tests that Window properties do not get GC'd before their Window.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS frameWindow.applicationCache.foo is 1
+PASS frameWindow.history.foo is 1
+PASS frameWindow.location.foo is 1
+PASS frameWindow.locationbar.foo is 1
+PASS frameWindow.menubar.foo is 1
+PASS frameWindow.navigator.foo is 1
+PASS frameWindow.personalbar.foo is 1
+PASS frameWindow.screen.foo is 1
+PASS frameWindow.scrollbars.foo is 1
+PASS frameWindow.statusbar.foo is 1
+PASS frameWindow.toolbar.foo is 1
+
+PASS frameWindow.applicationCache.foo is 1
+PASS frameWindow.history.foo is 1
+PASS frameWindow.location.foo is 1
+PASS frameWindow.locationbar.foo is 1
+PASS frameWindow.menubar.foo is 1
+PASS frameWindow.navigator.foo is 1
+PASS frameWindow.personalbar.foo is 1
+PASS frameWindow.screen.foo is 1
+PASS frameWindow.scrollbars.foo is 1
+PASS frameWindow.statusbar.foo is 1
+PASS frameWindow.toolbar.foo is 1
+
+PASS frameWindow.applicationCache.foo is 1
+PASS frameWindow.history.foo is 1
+PASS frameWindow.location.foo is 1
+PASS frameWindow.locationbar.foo is 1
+PASS frameWindow.menubar.foo is 1
+PASS frameWindow.navigator.foo is 1
+PASS frameWindow.personalbar.foo is 1
+PASS frameWindow.screen.foo is 1
+PASS frameWindow.scrollbars.foo is 1
+PASS frameWindow.statusbar.foo is 1
+PASS frameWindow.toolbar.foo is 1
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/dom/dom-window-property-gc-after-frame-detach.html b/LayoutTests/fast/dom/dom-window-property-gc-after-frame-detach.html
new file mode 100644 (file)
index 0000000..dc3378e
--- /dev/null
@@ -0,0 +1,32 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script src="../../resources/js-test.js"></script>
+<iframe id="testFrame" src="about:blank"></iframe>
+<script>
+description("Tests that Window properties do not get GC'd before their Window.");
+jsTestIsAsync = true;
+
+var windowProperties = ["applicationCache", "history", "location", "locationbar", "menubar", "navigator", "personalbar", "screen", "scrollbars", "statusbar", "toolbar" ];
+
+onload = function() {
+    frameWindow = document.getElementById("testFrame").contentWindow;
+    for (let windowProperty of windowProperties)
+        eval("frameWindow." + windowProperty + ".foo = 1;");
+    document.getElementById("testFrame").remove();
+    for (let windowProperty of windowProperties)
+        shouldBe("frameWindow." + windowProperty + ".foo", "1");
+    debug("");
+    gc();
+    for (let windowProperty of windowProperties)
+        shouldBe("frameWindow." + windowProperty + ".foo", "1");
+    debug("");
+    setTimeout(() => {
+        gc();
+        for (let windowProperty of windowProperties)
+            shouldBe("frameWindow." + windowProperty + ".foo", "1");
+        finishJSTest();
+    }, 10);
+}
+</script>
+</body>
index f6e087c..ad2a27b 100644 (file)
@@ -1,3 +1,44 @@
+2019-08-02  Chris Dumez  <cdumez@apple.com>
+
+        DOMWindow properties may get GC'd before their Window object
+        https://bugs.webkit.org/show_bug.cgi?id=200359
+
+        Reviewed by Ryosuke Niwa.
+
+        DOMWindow properties may get GC'd before their Window object once their frame is detached. This
+        is unexpected behavior given that these properties persist on the Window after the frame is
+        detached. This patch thus updates their bindings so that they live as long as their window, not
+        their frame.
+
+        Note that this also fixes a thread-safety issue since DOMWindowProperty::frame() would get called
+        from GC threads, although its implementation looks like:
+        """
+          return m_window ? m_window->frame() : nullptr;
+        """
+
+        Because m_window is a WeakPtr<DOMWindow> and because windows get destroyed on the main thread,
+        we could in theory crash when dereferencing m_window->frame() from the GC thread.
+
+        Test: fast/dom/dom-window-property-gc-after-frame-detach.html
+
+        * bindings/js/JSDOMWindowCustom.cpp:
+        (WebCore::JSDOMWindow::visitAdditionalChildren):
+        * bindings/scripts/CodeGeneratorJS.pm:
+        (GenerateImplementation):
+        * bindings/scripts/IDLAttributes.json:
+        * css/StyleMedia.idl:
+        * loader/appcache/DOMApplicationCache.idl:
+        * page/BarProp.idl:
+        * page/DOMSelection.idl:
+        * page/History.idl:
+        * page/Location.idl:
+        * page/Navigator.idl:
+        * page/Screen.idl:
+        * page/VisualViewport.idl:
+        * plugins/DOMMimeTypeArray.idl:
+        * plugins/DOMPluginArray.idl:
+        * storage/Storage.idl:
+
 2019-08-02  Konstantin Tokarev  <annulen@yandex.ru>
 
         Remove constructors and operators of FontPlatformData defined only for Freetype
index a966b55..311e292 100644 (file)
@@ -71,6 +71,8 @@ void JSDOMWindow::visitAdditionalChildren(SlotVisitor& visitor)
 {
     if (Frame* frame = wrapped().frame())
         visitor.addOpaqueRoot(frame);
+
+    visitor.addOpaqueRoot(&wrapped());
     
     // Normally JSEventTargetCustom.cpp's JSEventTarget::visitAdditionalChildren() would call this. But
     // even though DOMWindow is an EventTarget, JSDOMWindow does not subclass JSEventTarget, so we need
index a951bb7..33f4e69 100644 (file)
@@ -4680,6 +4680,12 @@ sub GenerateImplementation
                 $rootString .= "        return false;\n";
                 $rootString .= "    if (UNLIKELY(reason))\n";
                 $rootString .= "        *reason = \"Reachable from Frame\";\n";
+            } elsif (GetGenerateIsReachable($interface) eq "ReachableFromDOMWindow") {
+                $rootString  = "    auto* root = WTF::getPtr(js${interfaceName}->wrapped().window());\n";
+                $rootString .= "    if (!root)\n";
+                $rootString .= "        return false;\n";
+                $rootString .= "    if (UNLIKELY(reason))\n";
+                $rootString .= "        *reason = \"Reachable from Window\";\n";
             } elsif (GetGenerateIsReachable($interface) eq "ImplDocument") {
                 $rootString  = "    Document* root = WTF::getPtr(js${interfaceName}->wrapped().document());\n";
                 $rootString .= "    if (!root)\n";
index 9705c8b..c39bc63 100644 (file)
         },
         "GenerateIsReachable": {
             "contextsAllowed": ["interface"],
-            "values": ["", "Impl", "ImplWebGLRenderingContext", "ImplDocument", "ImplElementRoot", "ImplFrame", "ImplOwnerNodeRoot", "ImplScriptExecutionContext"]
+            "values": ["", "Impl", "ImplWebGLRenderingContext", "ImplDocument", "ImplElementRoot", "ImplFrame", "ImplOwnerNodeRoot", "ImplScriptExecutionContext", "ReachableFromDOMWindow"]
         },
         "Global": {
             "contextsAllowed": ["interface"],
index 1e871f2..191c024 100644 (file)
@@ -26,7 +26,7 @@
 
 [
     NoInterfaceObject,
-    GenerateIsReachable=ImplFrame,
+    GenerateIsReachable=ReachableFromDOMWindow,
     ImplementationLacksVTable,
 ] interface StyleMedia {
     readonly attribute DOMString type;
index 1799f2e..e56038f 100644 (file)
@@ -25,7 +25,7 @@
  
 [
     DoNotCheckConstants,
-    GenerateIsReachable=ImplFrame,
+    GenerateIsReachable=ReachableFromDOMWindow,
     InterfaceName=ApplicationCache,
 ] interface DOMApplicationCache : EventTarget {
     // update status
index f6fbae6..e461d45 100644 (file)
@@ -27,7 +27,7 @@
  */
 
 [
-    GenerateIsReachable=ImplFrame,
+    GenerateIsReachable=ReachableFromDOMWindow,
     ImplementationLacksVTable,
 ] interface BarProp {
     readonly attribute boolean visible;
index 979530a..fd92998 100644 (file)
@@ -29,7 +29,7 @@
 
 // https://www.w3.org/TR/selection-api/#idl-def-Selection
 [
-    GenerateIsReachable=ImplFrame,
+    GenerateIsReachable=ReachableFromDOMWindow,
     ImplementationLacksVTable,
     InterfaceName=Selection,
 ] interface DOMSelection {
index 0b958ee..b9bb0e5 100644 (file)
@@ -24,7 +24,7 @@
  */
 
 [
-    GenerateIsReachable=ImplFrame,
+    GenerateIsReachable=ReachableFromDOMWindow,
     JSCustomMarkFunction,
     ImplementationLacksVTable,
 ] interface History {
index 9dcd723..85cb9ab 100644 (file)
@@ -38,7 +38,7 @@
     CustomPut,
     CustomPutOnPrototype,
     CustomToStringName,
-    GenerateIsReachable=ImplFrame,
+    GenerateIsReachable=ReachableFromDOMWindow,
     IsImmutablePrototypeExoticObject,
     ImplementationLacksVTable,
     Unforgeable
index 45befb8..e442e04 100644 (file)
@@ -18,7 +18,7 @@
 */
 
 [
-    GenerateIsReachable=ImplFrame,
+    GenerateIsReachable=ReachableFromDOMWindow,
     JSCustomMarkFunction,
 ] interface Navigator {
     readonly attribute DOMPluginArray plugins;
index 662d08c..a13e26a 100644 (file)
@@ -28,7 +28,7 @@
 
 
 [
-    GenerateIsReachable=ImplFrame,
+    GenerateIsReachable=ReachableFromDOMWindow,
     ImplementationLacksVTable,
 ] interface Screen {
     readonly attribute unsigned long height;
index 39c9ac9..77f4b3e 100644 (file)
@@ -26,7 +26,7 @@
 // https://wicg.github.io/ViewportAPI/spec.html
 [
 EnabledBySetting=VisualViewportAPI,
-GenerateIsReachable=ImplFrame
+GenerateIsReachable=ReachableFromDOMWindow
 ]
 interface VisualViewport : EventTarget {
     readonly attribute double offsetLeft;
index 67f9132..48462df 100644 (file)
@@ -19,7 +19,7 @@
 */
 
 [
-    GenerateIsReachable=ImplFrame,
+    GenerateIsReachable=ReachableFromDOMWindow,
     LegacyUnenumerableNamedProperties,
     ImplementationLacksVTable,
     InterfaceName=MimeTypeArray,
index 14868ae..6f7032b 100644 (file)
@@ -19,7 +19,7 @@
 */
 
 [
-    GenerateIsReachable=ImplFrame,
+    GenerateIsReachable=ReachableFromDOMWindow,
     LegacyUnenumerableNamedProperties,
     ImplementationLacksVTable,
     InterfaceName=PluginArray,
index a3e370e..06bd6ad 100644 (file)
@@ -24,7 +24,7 @@
  */
 
 [
-    GenerateIsReachable=ImplFrame,
+    GenerateIsReachable=ReachableFromDOMWindow,
     SkipVTableValidation,
 ] interface Storage {
     readonly attribute unsigned long length;