Bug 209445 - Function passed to addEventListener may get garbage collected before the event listener is even added
Summary: Function passed to addEventListener may get garbage collected before the even...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Bindings (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks: 209552
  Show dependency treegraph
 
Reported: 2020-03-23 15:57 PDT by Chris Dumez
Modified: 2020-03-25 14:59 PDT (History)
14 users (show)

See Also:


Attachments
Patch (307.87 KB, patch)
2020-03-23 16:19 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2020-03-23 15:57:59 PDT
Function passed to addEventListener may get garbage collected before the event listener is even added.
Comment 1 Chris Dumez 2020-03-23 16:19:44 PDT
Created attachment 394319 [details]
Patch
Comment 2 Chris Dumez 2020-03-23 16:20:59 PDT
Implementation of addEventListener in the bindings:

static inline JSC::EncodedJSValue jsEventTargetPrototypeFunctionAddEventListenerBody(JSC::JSGlobalObject* lexicalGlobalObject, JSC::CallFrame* callFrame, typename IDLOperation<JSEventTarget>::ClassParameter castedThis, JSC::ThrowScope& throwScope)
{
    UNUSED_PARAM(lexicalGlobalObject);
    UNUSED_PARAM(callFrame);
    UNUSED_PARAM(throwScope);
    auto& impl = castedThis->wrapped();
    if (UNLIKELY(callFrame->argumentCount() < 2))
        return throwVMError(lexicalGlobalObject, throwScope, createNotEnoughArgumentsError(lexicalGlobalObject));
    auto argument0 = callFrame->uncheckedArgument(0);
    auto type = convert<IDLAtomStringAdaptor<IDLDOMString>>(*lexicalGlobalObject, argument0);
    RETURN_IF_EXCEPTION(throwScope, encodedJSValue());
    auto argument1 = callFrame->uncheckedArgument(1);
    auto callback = convert<IDLNullable<IDLEventListener<JSEventListener>>>(*lexicalGlobalObject, argument1, *castedThis);
    RETURN_IF_EXCEPTION(throwScope, encodedJSValue());
    auto argument2 = callFrame->argument(2);
    auto options = argument2.isUndefined() ? false : convert<IDLUnion<IDLDictionary<EventTarget::AddEventListenerOptions>, IDLBoolean>>(*lexicalGlobalObject, argument2);
    RETURN_IF_EXCEPTION(throwScope, encodedJSValue());
    impl.addEventListenerForBindings(WTFMove(type), WTFMove(callback), WTFMove(options));

    // Make sure arguments stay alive until this end of this method.
    ensureStillAliveHere(argument0);
    VM& vm = JSC::getVM(lexicalGlobalObject);
    vm.heap.writeBarrier(&static_cast<JSObject&>(*castedThis), argument1);
    ensureStillAliveHere(argument1);
    ensureStillAliveHere(argument2);

    return JSValue::encode(jsUndefined());
}
Comment 3 Chris Dumez 2020-03-23 16:21:34 PDT
Implementation of window.onmessage setter in the bindings:

static inline bool setJSDOMWindowOnmessageSetter(JSGlobalObject& lexicalGlobalObject, JSDOMWindow& thisObject, JSValue value, ThrowScope& throwScope)
{
    UNUSED_PARAM(lexicalGlobalObject);
    UNUSED_PARAM(throwScope);
    if (!BindingSecurity::shouldAllowAccessToDOMWindow(&lexicalGlobalObject, thisObject.wrapped(), ThrowSecurityError))
        return false;
    setWindowEventHandlerAttribute(lexicalGlobalObject, thisObject, thisObject.wrapped(), eventNames().messageEvent, value);

    VM& vm = JSC::getVM(&lexicalGlobalObject);
    vm.heap.writeBarrier(&thisObject, value);
    ensureStillAliveHere(value);

    return true;
}
Comment 4 Chris Dumez 2020-03-23 17:10:27 PDT
Comment on attachment 394319 [details]
Patch

Not a JSC / GC expert so please review carefully :)
Comment 5 Yusuke Suzuki 2020-03-23 17:38:37 PDT
Comment on attachment 394319 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394319&action=review

Can you add write-barriers for setWindowEventHandlerAttribute etc.?

> Source/WebCore/ChangeLog:22
> +        2. At the end of the operation implementation, we call ensureStillAliveHere() on each JSValue argument
> +           to make sure they stay alive until the end of the operation

Note that in binding code, arguments are in conservative stack so this is already kept alive.
For setWindowEventHandlerAttribute, I think ensureStillAliveHere is more important.
Comment 6 Chris Dumez 2020-03-23 17:40:17 PDT
(In reply to Yusuke Suzuki from comment #5)
> Comment on attachment 394319 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394319&action=review
> 
> Can you add write-barriers for setWindowEventHandlerAttribute etc.?

It already added them at call sites in generated bindings. See https://bugs.webkit.org/show_bug.cgi?id=209445#c3
Comment 7 Chris Dumez 2020-03-23 17:41:49 PDT
(In reply to Chris Dumez from comment #6)
> (In reply to Yusuke Suzuki from comment #5)
> > Comment on attachment 394319 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=394319&action=review
> > 
> > Can you add write-barriers for setWindowEventHandlerAttribute etc.?
> 
> It already added them at call sites in generated bindings. See
> https://bugs.webkit.org/show_bug.cgi?id=209445#c3

Since this is GC / JSC magic, I like it better in generated bindings code than in our DOM implementation.
Comment 8 Chris Dumez 2020-03-23 17:44:05 PDT
Comment on attachment 394319 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394319&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:5139
> +        push(@$outputArray, "    ensureStillAliveHere(value);\n\n");

This is the bindings change that takes care of setWindowEventHandlerAttribute() and other event handlers.
Comment 9 Yusuke Suzuki 2020-03-24 16:50:03 PDT
Comment on attachment 394319 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394319&action=review

r=me. It would be nice if we can have RAII class for ensureStillAliveHere, like EnsureStillAliveScope but it is OK for future patch.

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:5139
>> +        push(@$outputArray, "    ensureStillAliveHere(value);\n\n");
> 
> This is the bindings change that takes care of setWindowEventHandlerAttribute() and other event handlers.

Looks nice.
Comment 10 EWS 2020-03-24 17:01:33 PDT
Committed r258959: <https://trac.webkit.org/changeset/258959>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 394319 [details].
Comment 11 Radar WebKit Bug Importer 2020-03-24 17:02:20 PDT
<rdar://problem/60849075>
Comment 12 Chris Dumez 2020-03-25 14:59:58 PDT
(In reply to Yusuke Suzuki from comment #9)
> Comment on attachment 394319 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394319&action=review
> 
> r=me. It would be nice if we can have RAII class for ensureStillAliveHere,
> like EnsureStillAliveScope but it is OK for future patch.

Agreed, doing so here:
https://bugs.webkit.org/show_bug.cgi?id=209552