Install WebIDL EventTarget quickstubs things that implement EventTarget

RESOLVED FIXED in mozilla21

Status

()

defect
RESOLVED FIXED
7 years ago
3 months ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

unspecified
mozilla21
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 2 obsolete attachments)

8.86 KB, patch
peterv
: review+
Details | Diff | Splinter Review
1.42 KB, patch
peterv
: review+
Details | Diff | Splinter Review
5.97 KB, patch
peterv
: review+
Details | Diff | Splinter Review
11.66 KB, patch
peterv
: review+
Details | Diff | Splinter Review
2.01 KB, patch
Details | Diff | Splinter Review
1.33 KB, patch
peterv
: review+
Details | Diff | Splinter Review
This relies on all event targets inheriting from dom::EventTarget, of course.
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?
Flags: needinfo?(peterv)
> This particular XBL bindings claims implements="nsIDOMEventTarget"

I meant implements="nsIDOMEventListener".
Both seem reasonable, but if there's no corresponding XPCOM interface it might make more sense to do option 2.
Flags: needinfo?(peterv)
Attachment #700820 - Attachment is obsolete: true
Attachment #700820 - Flags: review?(peterv)
Whiteboard: [need review]
Attachment #700818 - Flags: review?(peterv) → review+
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.
Attachment #700819 - Flags: review?(peterv) → review-
Attachment #700819 - Attachment is obsolete: true
Attachment #707413 - Flags: review?(peterv) → review+
Attachment #701396 - Flags: review?(peterv) → review+
Attachment #701397 - Flags: review?(peterv) → review+
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.
Attachment #707623 - Flags: review?(peterv) → review+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.