We can do this once bug 779048 lands and we have native-to-JS conversions for unions done in bug 807224 (needed for onerror).

Please pay particular attention to the changes in nsJSEventListener::IsBlackForCC and nsEventListenerManager::MarkForCC.

Please pay special attention to nsJSEventListener::IsBlackForCC

automatically get the callback objects passed in from binding code.

Note: a sort of try run for this is at https://tbpl.mozilla.org/?tree=Try&rev=0b68d33a6dea

Mn is not happy. (I don't really know what Marionette tests do.)

Oh, it is just "Summary is empty"

Mn is not happy on vanillay pushes too, so I'm ignoring it.

Comment on attachment 678049 [details] [diff] [review] part 2. Change event handlers to store WebIDL callback functions. >@@ -830,17 +835,53 @@ nsEventListenerManager::CompileEventHand > NS_ENSURE_SUCCESS(result, result); > } > > if (handler) { > // Bind it > nsScriptObjectHolder<JSObject> boundHandler(context); > context->BindCompiledEventHandler(mTarget, listener->GetEventScope(), > handler.get(), boundHandler); >- listener->SetHandler(boundHandler.get()); >+ if (listener->EventName() == nsGkAtoms::onerror && win) { >+ bool ok; >+ JSAutoRequest ar(context->GetNativeContext()); >+ nsRefPtr<OnErrorEventHandlerNonNull> handlerCallback = >+ new OnErrorEventHandlerNonNull(context->GetNativeContext(), >+ listener->GetEventScope(), >+ boundHandler.get(), &ok); >+ if (!ok) { >+ return NS_ERROR_OUT_OF_MEMORY; >+ } This looks odd. Could you add some comment about possible causes of OOM >+ } else if (listener->EventName() == nsGkAtoms::onbeforeunload) { >+ // XXXbz Should we really do the special beforeunload handler on >+ // non-Window objects? Per spec, we shouldn't even be compiling >+ // the beforeunload content attribute on random elements! Could you file a followup and add the bug number here. >+ if (aEventName == nsGkAtoms::onerror && win) { >+ bool ok; >+ nsRefPtr<OnErrorEventHandlerNonNull> handlerCallback = >+ new OnErrorEventHandlerNonNull(cx, aScope, callable, &ok); >+ if (!ok) { >+ return NS_ERROR_OUT_OF_MEMORY; >+ } >+ handler.SetHandler(handlerCallback); >+ } else if (aEventName == nsGkAtoms::onbeforeunload) { >+ MOZ_ASSERT(win, >+ "Should not have onbeforeunload handlers on non-Window objects"); >+ bool ok; >+ nsRefPtr<BeforeUnloadEventHandlerNonNull> handlerCallback = >+ new BeforeUnloadEventHandlerNonNull(cx, aScope, callable, &ok); >+ if (!ok) { >+ return NS_ERROR_OUT_OF_MEMORY; >+ } >+ handler.SetHandler(handlerCallback); >+ } else { >+ bool ok; >+ nsRefPtr<EventHandlerNonNull> handlerCallback = >+ new EventHandlerNonNull(cx, aScope, callable, &ok); >+ if (!ok) { >+ return NS_ERROR_OUT_OF_MEMORY; >+ } >+ handler.SetHandler(handlerCallback); Hmm, code duplication. Could you have a helper method which is called in this method and in nsEventListenerManager::CompileEventHandler >- // Set a handler for this event listener. Must not be called if >- // there is already a handler! The handler must already be bound to >- // the right target. >- virtual void SetHandler(JSObject *aHandler) = 0; >+ nsIAtom *EventName() const nsIAtom* >+ void SetHandler(const nsEventHandler& aHandler) { >+ mHandler.SetHandler(aHandler); >+ } >+ void SetHandler(mozilla::dom::EventHandlerNonNull* aHandler) { >+ mHandler.SetHandler(aHandler); >+ } >+ void SetHandler(mozilla::dom::BeforeUnloadEventHandlerNonNull* aHandler) { >+ mHandler.SetHandler(aHandler); >+ } >+ void SetHandler(mozilla::dom::OnErrorEventHandlerNonNull* aHandler) { >+ mHandler.SetHandler(aHandler); >+ } { goes to next line >+ bool HasGrayCallable() const >+ { >+ return xpc_IsGrayGCThing(mCallable); >+ } return mCallable && xpc_IsGrayGCThing(mCallable); I think I could take another look after the code duplication is fixed.

Comment on attachment 678050 [details] [diff] [review] part 3. Change event handlers to call their WebIDL callbacks directly. >+ errorMsg = scriptEvent->errorMsg; >+ msgOrEvent.SetAsString() = static_cast<nsAString*>(&errorMsg); This is somewhat odd looking. Would it be possible to change SetAsString to take a parameter? Though, I guess SetAsString() is consistent with Optional<...>.Value() So, no need to change.

Comment on attachment 678051 [details] [diff] [review] part 4. Allow event handlers to have a null nsIScriptContext if they won't have to compile from a string. Looking good.

Comment on attachment 678049 [details] [diff] [review] part 2. Change event handlers to store WebIDL callback functions. Ah, the code duplication is fixed in another patch.

Comment on attachment 678057 [details] [diff] [review] part 8. Remove the exceptions for EventHandler in WebIDL codegen. This is all great :) Hopefully I haven't missed anything important.

> return mCallable && xpc_IsGrayGCThing(mCallable); mCallable is only null if we've been unlinked (well, of if the constructor returns a false "ok", but people better not be doing anything with the object then)... Will we end up having HasGrayCallable() called after we have been unlinked? > This is somewhat odd looking. Yeah, I'll file a followup on giving unions better ergonomics. Thanks for the reviews!

(In reply to Boris Zbarsky (:bz) from comment #17) > > return mCallable && xpc_IsGrayGCThing(mCallable); > > mCallable is only null if we've been unlinked (well, of if the constructor > returns a false "ok", but people better not be doing anything with the > object then)... Will we end up having HasGrayCallable() called after we > have been unlinked? As of now no, but if we add more skippability, yet have some missing unlinking elsewhere, we might. So I think adding null check would be good there.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a1566a0fa1c8 https://hg.mozilla.org/integration/mozilla-inbound/rev/e2727ec34a16 https://hg.mozilla.org/integration/mozilla-inbound/rev/0ee3b6d2415a https://hg.mozilla.org/integration/mozilla-inbound/rev/f64f70e0649c https://hg.mozilla.org/integration/mozilla-inbound/rev/f244d412e485 https://hg.mozilla.org/integration/mozilla-inbound/rev/741e28cf55ee https://hg.mozilla.org/integration/mozilla-inbound/rev/f9e7d57d435a https://hg.mozilla.org/integration/mozilla-inbound/rev/69eccdae84d7 https://hg.mozilla.org/integration/mozilla-inbound/rev/bd9729ca4de2

https://hg.mozilla.org/mozilla-central/rev/a1566a0fa1c8 https://hg.mozilla.org/mozilla-central/rev/e2727ec34a16 https://hg.mozilla.org/mozilla-central/rev/0ee3b6d2415a https://hg.mozilla.org/mozilla-central/rev/f64f70e0649c https://hg.mozilla.org/mozilla-central/rev/f244d412e485 https://hg.mozilla.org/mozilla-central/rev/741e28cf55ee https://hg.mozilla.org/mozilla-central/rev/f9e7d57d435a https://hg.mozilla.org/mozilla-central/rev/69eccdae84d7 https://hg.mozilla.org/mozilla-central/rev/bd9729ca4de2