Changeset 259009 in webkit


Ignore:
Timestamp:
Mar 25, 2020 2:16:13 PM (4 years ago)
Author:
Chris Dumez
Message:

Event listeners registered with 'once' option may get garbage collected too soon
​https://bugs.webkit.org/show_bug.cgi?id=209504
<rdar://problem/60541567>

Reviewed by Yusuke Suzuki.

Source/JavaScriptCore:

Add EnsureStillAliveScope RAII object for ensureStillAliveHere().

  • runtime/JSCJSValue.h:

(JSC::EnsureStillAliveScope::EnsureStillAliveScope):
(JSC::EnsureStillAliveScope::~EnsureStillAliveScope):

Source/WebCore:

In EventTarget::innerInvokeEventListeners, if the listener we're about to call is a one-time
listener (has 'once' flag set), we would first unregister the event listener and then call
it, as per the DOM specification. However, once unregistered, the event listener is no longer
visited for GC purposes and its internal JS Function may get garbage collected before we get
a chance to call it.

To address the issue, we now make sure the JS Function (and its wrapper) stay alive for the
duration of the scope using ensureStillAliveHere().

Test: http/tests/inspector/network/har/har-page-aggressive-gc.html

  • bindings/js/JSEventListener.h:
  • dom/EventListener.h:

(WebCore::EventListener::jsFunction const):
(WebCore::EventListener::wrapper const):

  • dom/EventTarget.cpp:

(WebCore::EventTarget::innerInvokeEventListeners):

LayoutTests:

Add layout test coverage.

  • http/tests/inspector/network/har/har-page-aggressive-gc-expected.txt: Added.
  • http/tests/inspector/network/har/har-page-aggressive-gc.html: Added.
  • platform/gtk/TestExpectations:
  • platform/mac-wk1/TestExpectations:
  • platform/mac-wk2/TestExpectations:
  • platform/win/TestExpectations:
Location:
trunk
Files:
2 added
15 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r259007 r259009  
     12020-03-25  Chris Dumez  <cdumez@apple.com>
     2
     3        Event listeners registered with 'once' option may get garbage collected too soon
     4        https://bugs.webkit.org/show_bug.cgi?id=209504
     5        <rdar://problem/60541567>
     6
     7        Reviewed by Yusuke Suzuki.
     8
     9        Add layout test coverage.
     10
     11        * http/tests/inspector/network/har/har-page-aggressive-gc-expected.txt: Added.
     12        * http/tests/inspector/network/har/har-page-aggressive-gc.html: Added.
     13        * platform/gtk/TestExpectations:
     14        * platform/mac-wk1/TestExpectations:
     15        * platform/mac-wk2/TestExpectations:
     16        * platform/win/TestExpectations:
     17
    1182020-03-25  Jason Lawrence  <lawrence.j@apple.com>
    219

  • trunk/LayoutTests/platform/gtk/TestExpectations

    r258979 r259009  
    20692069webkit.org/b/191497 http/tests/inspector/network/getSerializedCertificate.html [ Skip ]
    20702070webkit.org/b/179173 [ Release ] http/tests/inspector/network/har/har-page.html [ Failure Pass ]
     2071webkit.org/b/179173 [ Release ] http/tests/inspector/network/har/har-page-aggressive-gc.html [ Failure Pass ]
    20712072
    20722073webkit.org/b/186851 imported/w3c/web-platform-tests/xhr/formdata.htm [ Pass Failure ]

  • trunk/LayoutTests/platform/mac-wk1/TestExpectations

    r259007 r259009  
    563563http/tests/inspector/network/resource-sizes-memory-cache.html [ Failure ]
    564564http/tests/inspector/network/har/har-page.html [ Failure ]
     565http/tests/inspector/network/har/har-page-aggressive-gc.html [ Failure ]
    565566
    566567# Local Overrides not available in WebKit1

  • trunk/LayoutTests/platform/mac-wk2/TestExpectations

    r258942 r259009  
    10141014
    10151015webkit.org/b/207954 http/tests/inspector/network/har/har-page.html [ Pass Failure ]
     1016webkit.org/b/207954 http/tests/inspector/network/har/har-page-aggressive-gc.html [ Pass Failure ]
    10161017
    10171018webkit.org/b/207962 accessibility/mac/aria-menu-item-selected-notification.html [ Slow ]

  • trunk/LayoutTests/platform/win/TestExpectations

    r258990 r259009  
    36573657http/tests/inspector/network/resource-request-headers.html [ Skip ]
    36583658http/tests/inspector/network/har/har-page.html [ Skip ]
     3659http/tests/inspector/network/har/har-page-aggressive-gc.html [ Skip ]
    36593660http/tests/local/blob/send-hybrid-blob-using-open-panel.html [ Skip ]
    36603661http/tests/security/contentSecurityPolicy/cross-origin-plugin-document-allowed-in-child-window.html [ Skip ]

  • trunk/Source/JavaScriptCore/ChangeLog

    r258976 r259009  
     12020-03-25  Chris Dumez  <cdumez@apple.com>
     2
     3        Event listeners registered with 'once' option may get garbage collected too soon
     4        https://bugs.webkit.org/show_bug.cgi?id=209504
     5        <rdar://problem/60541567>
     6
     7        Reviewed by Yusuke Suzuki.
     8
     9        Add EnsureStillAliveScope RAII object for ensureStillAliveHere().
     10
     11        * runtime/JSCJSValue.h:
     12        (JSC::EnsureStillAliveScope::EnsureStillAliveScope):
     13        (JSC::EnsureStillAliveScope::~EnsureStillAliveScope):
     14
    1152020-03-25  Alexey Shvayka  <shvaikalesh@gmail.com>
    216

  • trunk/Source/JavaScriptCore/runtime/JSCJSValue.h

    r258825 r259009  
    3535#include <wtf/MathExtras.h>
    3636#include <wtf/MediaTime.h>
     37#include <wtf/Nonmovable.h>
    3738#include <wtf/StdLibExtras.h>
    3839#include <wtf/TriState.h>
    … …  
    651652#endif
    652653
     654// Use EnsureStillAliveScope when you have a data structure that includes GC pointers, and you need
     655// to remove it from the DOM and then use it in the same scope. For example, a 'once' event listener
     656// needs to be removed from the DOM and then fired.
     657class EnsureStillAliveScope {
     658    WTF_FORBID_HEAP_ALLOCATION;
     659    WTF_MAKE_NONCOPYABLE(EnsureStillAliveScope);
     660    WTF_MAKE_NONMOVABLE(EnsureStillAliveScope);
     661public:
     662    EnsureStillAliveScope(JSValue value)
     663        : m_value(value)
     664    {
     665    }
     666
     667    ~EnsureStillAliveScope()
     668    {
     669        ensureStillAliveHere(m_value);
     670    }
     671
     672private:
     673    JSValue m_value;
     674};
     675
    653676} // namespace JSC

  • trunk/Source/WebCore/ChangeLog

    r259008 r259009  
     12020-03-25  Chris Dumez  <cdumez@apple.com>
     2
     3        Event listeners registered with 'once' option may get garbage collected too soon
     4        https://bugs.webkit.org/show_bug.cgi?id=209504
     5        <rdar://problem/60541567>
     6
     7        Reviewed by Yusuke Suzuki.
     8
     9        In EventTarget::innerInvokeEventListeners, if the listener we're about to call is a one-time
     10        listener (has 'once' flag set), we would first unregister the event listener and then call
     11        it, as per the DOM specification. However, once unregistered, the event listener is no longer
     12        visited for GC purposes and its internal JS Function may get garbage collected before we get
     13        a chance to call it.
     14
     15        To address the issue, we now make sure the JS Function (and its wrapper) stay alive for the
     16        duration of the scope using ensureStillAliveHere().
     17
     18        Test: http/tests/inspector/network/har/har-page-aggressive-gc.html
     19
     20        * bindings/js/JSEventListener.h:
     21        * dom/EventListener.h:
     22        (WebCore::EventListener::jsFunction const):
     23        (WebCore::EventListener::wrapper const):
     24        * dom/EventTarget.cpp:
     25        (WebCore::EventTarget::innerInvokeEventListeners):
     26
    1272020-03-25  Wenson Hsieh  <wenson_hsieh@apple.com>
    228

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

    r251425 r259009  
    6868    JSLockHolder lock(vm);
    6969
    70     JSObject* jsFunction = this->jsFunction(scriptExecutionContext);
     70    JSObject* jsFunction = this->ensureJSFunction(scriptExecutionContext);
    7171    if (!jsFunction)
    7272        return;

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

    r258959 r259009  
    111111    // exception.
    112112
    113     JSObject* jsFunction = this->jsFunction(scriptExecutionContext);
     113    JSObject* jsFunction = ensureJSFunction(scriptExecutionContext);
    114114    if (!jsFunction)
    115115        return;
    … …  
    243243        return jsNull();
    244244
    245     auto* function = downcast<JSEventListener>(*abstractListener).jsFunction(context);
     245    auto* function = downcast<JSEventListener>(*abstractListener).ensureJSFunction(context);
    246246    if (!function)
    247247        return jsNull();

  • trunk/Source/WebCore/bindings/js/JSEventListener.h

    r258189 r259009  
    4949    bool isAttribute() const final { return m_isAttribute; }
    5050
    51     JSC::JSObject* jsFunction(ScriptExecutionContext&) const;
     51    JSC::JSObject* ensureJSFunction(ScriptExecutionContext&) const;
    5252    DOMWrapperWorld& isolatedWorld() const { return m_isolatedWorld; }
    5353
    54     JSC::JSObject* wrapper() const { return m_wrapper.get(); }
     54
     55    JSC::JSObject* jsFunction() const final { return m_jsFunction.get(); }
     56    JSC::JSObject* wrapper() const final { return m_wrapper.get(); }
    5557
    5658    virtual String sourceURL() const { return String(); }
    … …  
    9395void setDocumentEventHandlerAttribute(JSC::JSGlobalObject&, JSC::JSObject&, Document&, const AtomString& eventType, JSC::JSValue);
    9496
    95 inline JSC::JSObject* JSEventListener::jsFunction(ScriptExecutionContext& scriptExecutionContext) const
     97inline JSC::JSObject* JSEventListener::ensureJSFunction(ScriptExecutionContext& scriptExecutionContext) const
    9698{
    9799    // initializeJSFunction can trigger code that deletes this event listener

  • trunk/Source/WebCore/dom/EventListener.h

    r254087 r259009  
    6161#endif
    6262
     63    virtual JSC::JSObject* jsFunction() const { return nullptr; }
     64    virtual JSC::JSObject* wrapper() const { return nullptr; }
     65
    6366protected:
    6467    explicit EventListener(Type type)

  • trunk/Source/WebCore/dom/EventTarget.cpp

    r258159 r259009  
    305305            break;
    306306
     307        // Make sure the JS wrapper and function stay alive until the end of this scope. Otherwise,
     308        // event listeners with 'once' flag may get collected as soon as they get unregistered below,
     309        // before we call the js function.
     310        JSC::EnsureStillAliveScope wrapperProtector(registeredListener->callback().wrapper());
     311        JSC::EnsureStillAliveScope jsFunctionProtector(registeredListener->callback().jsFunction());
     312
    307313        // Do this before invocation to avoid reentrancy issues.
    308314        if (registeredListener->isOnce())

  • trunk/Source/WebCore/inspector/CommandLineAPIHost.cpp

    r251425 r259009  
    114114                continue;
    115115
    116             auto* function = jsListener.jsFunction(*scriptExecutionContext);
     116            auto* function = jsListener.ensureJSFunction(*scriptExecutionContext);
    117117            if (!function)
    118118                continue;

  • trunk/Source/WebCore/inspector/agents/InspectorDOMAgent.cpp

    r257188 r259009  
    17711771
    17721772        if (document) {
    1773             handlerObject = scriptListener.jsFunction(*document);
     1773            handlerObject = scriptListener.ensureJSFunction(*document);
    17741774            exec = execStateFromNode(scriptListener.isolatedWorld(), document);
    17751775        }
Note: See TracChangeset for help on using the changeset viewer.