Function passed to addEventListener may get garbage collected before the event listener is even added.
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