829252 - Install WebIDL EventTarget quickstubs things that implement EventTarget

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

This relies on all event targets inheriting from dom::EventTarget, of course.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Change nsGlobalWindow to use nsIDOMEventTarget as the canonical isupports instead of nsIScriptGlobalObject. . We'll need this to be able to cast to it properly in binding code.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Make nsGlobalWindow inherit from dom::EventTarget and ensure that all the things that inherit from it correctly QI to it.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Turn on the EventTarget quickstubs.

Comment 4

[Boris Zbarsky [:bzbarsky]]

So I ran into an issue with that last patch.

Unfortunately, it's an issue we could hit right now, with slightly different code.

The problem is that when calling alert() we hit this code in toolkit/components/prompts/content/tabprompts.xml in the init() method:

156                 window.addEventListener("resize", this, false);
157                 window.addEventListener("unload", this, false);
158                 linkedTab.addEventListener("TabClose", this, false);
159                 this.args.domWindow.addEventListener("pagehide", this, false);

here "window" is the ChromeWindow and this.args.domWindow is an Xray for the content window that called alert().

So those first two calls go through WebIDL bindings, and create an XPCWrappedJS for the JSObject involved (which is an XPCWrappedNative for an nsXULElement).

But the addEventListener call on the xray calls the XPCOM method, since that's what the Xray knows how to return.  This lansd us in XPCConvert, which sees that we have a wrappednative and tries to QI.  This particular XBL bindings claims implements="nsIDOMEventTarget", so the QI lands us in nsBindingManager::GetBindingImplementation creating an XPCWrappedJS via nsXPConnect::WrapJSAggregatedToNative.  

But if I read the XPCWrappedJS::GetNewOrUsed code correctly, we've already got a "root" XPCWrappedJS for this JSObject (though it was created without aggregation), so we use that as the root and add the new JSObject for this interface we're wrapping for to it.   Then we call AggregatedQueryInterface on the XPCWrappedJS (near the bottom of XPCConvert::JSObject2NativeInterface) and this asserts IsAggregatedToNative().

This can probably be hit on trunk right now by simply adding such an element as an event listener to an element first and then to a window, to trigger the same webidl-then-xpidl call sequence.

So the question is what the right fix is.

Looking at the XPConnect code, it allows an XPCWrappedNative or slimwrapper or WebIDL object that has nsISupports to implement an interface only if they QI to it.  I suppose we could enforce that in the callback interface code we have here for now, so as to ensure that we call QI, an hence do any aggregated XPCWrappedJS witchery needed here, before we try to do our own XPCWrappedJS::GetNewOrUsed call.

Another option is to just pass a non-null aOuter to XPCWrappedJS::GetNewOrUsed in the binding callback interface codegen in the cases when we have an object that has nsISupports.

Peter, does any of that sound reasonable?

Comment 5

[Boris Zbarsky [:bzbarsky]]

> This particular XBL bindings claims implements="nsIDOMEventTarget"

I meant implements="nsIDOMEventListener".

Comment 6

[Peter Van der Beken [:peterv]]

Both seem reasonable, but if there's no corresponding XPCOM interface it might make more sense to do option 2.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. When wrapping things with an nsISupports in XPCWrappedJS for WebIDL callback interfaces, use the aggregated version.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Turn on the EventTarget quickstubs.

Comment 9

[Peter Van der Beken [:peterv]]

Comment on attachment 700819 [details] [diff] [review]
part 2.  Make nsGlobalWindow inherit from dom::EventTarget and ensure that all the things that inherit from it correctly QI to it.

Review of attachment 700819 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsWindowRoot.cpp
@@ +47,5 @@
>  NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(nsWindowRoot)
>    NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIDOMEventTarget)
>    NS_INTERFACE_MAP_ENTRY(nsPIWindowRoot)
>    NS_INTERFACE_MAP_ENTRY(nsIDOMEventTarget)
> +  NS_INTERFACE_MAP_ENTRY(mozilla::dom::EventTarget)

I think you need to add nsWrapperCache to QI and probably traverse/unlink the preserved wrapper.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2 updated to comments

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Interdiff addressing review comments

Comment 12

[Boris Zbarsky [:bzbarsky]]

   https://hg.mozilla.org/integration/mozilla-inbound/rev/9f934db4606a
   https://hg.mozilla.org/integration/mozilla-inbound/rev/0b23beab8038
   https://hg.mozilla.org/integration/mozilla-inbound/rev/aa85bbb41c04
   https://hg.mozilla.org/integration/mozilla-inbound/rev/2b9a689ac459

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Followup fix to deal with WindowRoot having a wrapper cache for real.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Followup pushed to get the tree green: https://hg.mozilla.org/integration/mozilla-inbound/rev/c669c3af96d4

Comment 15

[Peter Van der Beken [:peterv]]

Comment on attachment 707623 [details] [diff] [review]
Followup fix to deal with WindowRoot having a wrapper cache for real.

Hmm, I would have set tryConstructSlimWrapper to |aHelper.GetXPCClassInfo() != nullptr| in XPCConvert::NativeInterface2JSObject, but ok. This should all go away soon enough.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9f934db4606a
https://hg.mozilla.org/mozilla-central/rev/0b23beab8038
https://hg.mozilla.org/mozilla-central/rev/aa85bbb41c04
https://hg.mozilla.org/mozilla-central/rev/2b9a689ac459
https://hg.mozilla.org/mozilla-central/rev/c669c3af96d4