Changeset 248155 in webkit


Ignore:
Timestamp:
Aug 2, 2019 9:43:56 AM (5 years ago)
Author:
Chris Dumez
Message:

DOMWindow properties may get GC'd before their Window object
​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.
Location:
trunk
Files:
2 added
17 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r248150 r248155  
     12019-08-02  Chris Dumez  <cdumez@apple.com>
     2
     3        DOMWindow properties may get GC'd before their Window object
     4        https://bugs.webkit.org/show_bug.cgi?id=200359
     5
     6        Reviewed by Ryosuke Niwa.
     7
     8        Add layout test coverage.
     9
     10        * fast/dom/dom-window-property-gc-after-frame-detach-expected.txt: Added.
     11        * fast/dom/dom-window-property-gc-after-frame-detach.html: Added.
     12
    1132019-08-02  Carlos Garcia Campos  <cgarcia@igalia.com>
    214

  • trunk/Source/WebCore/ChangeLog

    r248152 r248155  
     12019-08-02  Chris Dumez  <cdumez@apple.com>
     2
     3        DOMWindow properties may get GC'd before their Window object
     4        https://bugs.webkit.org/show_bug.cgi?id=200359
     5
     6        Reviewed by Ryosuke Niwa.
     7
     8        DOMWindow properties may get GC'd before their Window object once their frame is detached. This
     9        is unexpected behavior given that these properties persist on the Window after the frame is
     10        detached. This patch thus updates their bindings so that they live as long as their window, not
     11        their frame.
     12
     13        Note that this also fixes a thread-safety issue since DOMWindowProperty::frame() would get called
     14        from GC threads, although its implementation looks like:
     15        """
     16          return m_window ? m_window->frame() : nullptr;
     17        """
     18
     19        Because m_window is a WeakPtr<DOMWindow> and because windows get destroyed on the main thread,
     20        we could in theory crash when dereferencing m_window->frame() from the GC thread.
     21
     22        Test: fast/dom/dom-window-property-gc-after-frame-detach.html
     23
     24        * bindings/js/JSDOMWindowCustom.cpp:
     25        (WebCore::JSDOMWindow::visitAdditionalChildren):
     26        * bindings/scripts/CodeGeneratorJS.pm:
     27        (GenerateImplementation):
     28        * bindings/scripts/IDLAttributes.json:
     29        * css/StyleMedia.idl:
     30        * loader/appcache/DOMApplicationCache.idl:
     31        * page/BarProp.idl:
     32        * page/DOMSelection.idl:
     33        * page/History.idl:
     34        * page/Location.idl:
     35        * page/Navigator.idl:
     36        * page/Screen.idl:
     37        * page/VisualViewport.idl:
     38        * plugins/DOMMimeTypeArray.idl:
     39        * plugins/DOMPluginArray.idl:
     40        * storage/Storage.idl:
     41
    1422019-08-02  Konstantin Tokarev  <annulen@yandex.ru>
    243

  • trunk/Source/WebCore/bindings/js/JSDOMWindowCustom.cpp

    r247434 r248155  
    7272    if (Frame* frame = wrapped().frame())
    7373        visitor.addOpaqueRoot(frame);
     74
     75    visitor.addOpaqueRoot(&wrapped());
    7476   
    7577    // Normally JSEventTargetCustom.cpp's JSEventTarget::visitAdditionalChildren() would call this. But

  • trunk/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm

    r248105 r248155  
    46814681                $rootString .= "    if (UNLIKELY(reason))\n";
    46824682                $rootString .= "        *reason = \"Reachable from Frame\";\n";
     4683            } elsif (GetGenerateIsReachable($interface) eq "ReachableFromDOMWindow") {
     4684                $rootString  = "    auto* root = WTF::getPtr(js${interfaceName}->wrapped().window());\n";
     4685                $rootString .= "    if (!root)\n";
     4686                $rootString .= "        return false;\n";
     4687                $rootString .= "    if (UNLIKELY(reason))\n";
     4688                $rootString .= "        *reason = \"Reachable from Window\";\n";
    46834689            } elsif (GetGenerateIsReachable($interface) eq "ImplDocument") {
    46844690                $rootString  = "    Document* root = WTF::getPtr(js${interfaceName}->wrapped().document());\n";

  • trunk/Source/WebCore/bindings/scripts/IDLAttributes.json

    r246768 r248155  
    227227        "GenerateIsReachable": {
    228228            "contextsAllowed": ["interface"],
    229             "values": ["", "Impl", "ImplWebGLRenderingContext", "ImplDocument", "ImplElementRoot", "ImplFrame", "ImplOwnerNodeRoot", "ImplScriptExecutionContext"]
     229            "values": ["", "Impl", "ImplWebGLRenderingContext", "ImplDocument", "ImplElementRoot", "ImplFrame", "ImplOwnerNodeRoot", "ImplScriptExecutionContext", "ReachableFromDOMWindow"]
    230230        },
    231231        "Global": {

  • trunk/Source/WebCore/css/StyleMedia.idl

    r242676 r248155  
    2727[
    2828    NoInterfaceObject,
    29     GenerateIsReachable=ImplFrame,
     29    GenerateIsReachable=ReachableFromDOMWindow,
    3030    ImplementationLacksVTable,
    3131] interface StyleMedia {

  • trunk/Source/WebCore/loader/appcache/DOMApplicationCache.idl

    r207522 r248155  
    2626[
    2727    DoNotCheckConstants,
    28     GenerateIsReachable=ImplFrame,
     28    GenerateIsReachable=ReachableFromDOMWindow,
    2929    InterfaceName=ApplicationCache,
    3030] interface DOMApplicationCache : EventTarget {

  • trunk/Source/WebCore/page/BarProp.idl

    r242676 r248155  
    2828
    2929[
    30     GenerateIsReachable=ImplFrame,
     30    GenerateIsReachable=ReachableFromDOMWindow,
    3131    ImplementationLacksVTable,
    3232] interface BarProp {

  • trunk/Source/WebCore/page/DOMSelection.idl

    r242676 r248155  
    3030// https://www.w3.org/TR/selection-api/#idl-def-Selection
    3131[
    32     GenerateIsReachable=ImplFrame,
     32    GenerateIsReachable=ReachableFromDOMWindow,
    3333    ImplementationLacksVTable,
    3434    InterfaceName=Selection,

  • trunk/Source/WebCore/page/History.idl

    r242676 r248155  
    2525
    2626[
    27     GenerateIsReachable=ImplFrame,
     27    GenerateIsReachable=ReachableFromDOMWindow,
    2828    JSCustomMarkFunction,
    2929    ImplementationLacksVTable,

  • trunk/Source/WebCore/page/Location.idl

    r242676 r248155  
    3939    CustomPutOnPrototype,
    4040    CustomToStringName,
    41     GenerateIsReachable=ImplFrame,
     41    GenerateIsReachable=ReachableFromDOMWindow,
    4242    IsImmutablePrototypeExoticObject,
    4343    ImplementationLacksVTable,

  • trunk/Source/WebCore/page/Navigator.idl

    r246070 r248155  
    1919
    2020[
    21     GenerateIsReachable=ImplFrame,
     21    GenerateIsReachable=ReachableFromDOMWindow,
    2222    JSCustomMarkFunction,
    2323] interface Navigator {

  • trunk/Source/WebCore/page/Screen.idl

    r242676 r248155  
    2929
    3030[
    31     GenerateIsReachable=ImplFrame,
     31    GenerateIsReachable=ReachableFromDOMWindow,
    3232    ImplementationLacksVTable,
    3333] interface Screen {

  • trunk/Source/WebCore/page/VisualViewport.idl

    r226802 r248155  
    2727[
    2828EnabledBySetting=VisualViewportAPI,
    29 GenerateIsReachable=ImplFrame
     29GenerateIsReachable=ReachableFromDOMWindow
    3030]
    3131interface VisualViewport : EventTarget {

  • trunk/Source/WebCore/plugins/DOMMimeTypeArray.idl

    r242676 r248155  
    2020
    2121[
    22     GenerateIsReachable=ImplFrame,
     22    GenerateIsReachable=ReachableFromDOMWindow,
    2323    LegacyUnenumerableNamedProperties,
    2424    ImplementationLacksVTable,

  • trunk/Source/WebCore/plugins/DOMPluginArray.idl

    r242676 r248155  
    2020
    2121[
    22     GenerateIsReachable=ImplFrame,
     22    GenerateIsReachable=ReachableFromDOMWindow,
    2323    LegacyUnenumerableNamedProperties,
    2424    ImplementationLacksVTable,

  • trunk/Source/WebCore/storage/Storage.idl

    r220071 r248155  
    2525
    2626[
    27     GenerateIsReachable=ImplFrame,
     27    GenerateIsReachable=ReachableFromDOMWindow,
    2828    SkipVTableValidation,
    2929] interface Storage {
Note: See TracChangeset for help on using the changeset viewer.