Bug 209445

Created attachment 394319 [details]
Patch

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());
}

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 on attachment 394319 [details]
Patch

Not a JSC / GC expert so please review carefully :)

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.

(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

(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 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 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.

Committed r258959: <https://trac.webkit.org/changeset/258959>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 394319 [details].

<rdar://problem/60849075>

(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