983619 - Add Window.getInterface/QueryInterface to WebIDL

Status: RESOLVED FIXED

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

Description

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8391155 [details] [diff] [review]
v1

>+  JS::Rooted<JSObject*> global(aCx, GetWrapper());

We need a "JSAutoCompartment ac(aCx, global);" after this line.

>+  if (!dom::WrapObject(aCx, global, result, iid, &v)) {

Don't we need to Throw() on aError if that fails?

It _might_ be nice to share code with nsXMLHttpRequest here (e.g. by having a templated utility method, or just a utility method that takes an nsIInterfaceRequestor* and an nsWrapperCache*).  I found both the above issues by comparing to that other method....

r=me with those fixed.

Comment 3

[Peter Van der Beken [:peterv]]

Made an inline templated GetInterface method (which calls GetInterfaceImpl taking nsIInterfaceRequestor* and nsWrapperCache*).

http://hg.mozilla.org/integration/mozilla-inbound/rev/8aca83520a91

Comment 4

[[e-mail]]

Comment on attachment 8391155 [details] [diff] [review]
v1

>+  nsCOMPtr<nsISupports> result;
>+  aError = GetInterface(*iid, getter_AddRefs(result));
In debug builds, ~nsGetterAddRefs calls Assert_NoQueryNeeded on the returned pointer, which will try to query to nsISupports, rather than iid, which might fail. You may want to use nsRefPtr instead.

Comment 5

[Peter Van der Beken [:peterv]]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/3b1a35bd4e3f.

Comment 6

[Peter Van der Beken [:peterv]]

(In reply to Boris Zbarsky [:bz] from comment #2)
> We need a "JSAutoCompartment ac(aCx, global);" after this line.

This actually seems wrong since we really want to wrap in the compartment of the caller, otherwise the security manager will block the wrapping (eg for non-WebIDL objects with no classinfo) if the caller is chrome but global is not.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Hmm.  Can we just pass a null scope then?  Or will that crash instead of using the current compartment of cx?

Basically, it seems really weird to me to be passing a scope and the JSContext in different compartments...

Comment 8

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/39b430a304ca

(In reply to Boris Zbarsky [:bz] from comment #7)
> Hmm.  Can we just pass a null scope then?  Or will that crash instead of
> using the current compartment of cx?

We can't pass in null, but I guess we could pass in |JS::CurrentGlobalOrNull(aCx)| instead (bailing out if that returns null). What do you think?

Comment 9

[Boris Zbarsky [:bzbarsky]]

> bailing out if that returns null

It better not return null!  This should only be called directly from the JS engine, no?

I'm ok passing CurrentGlobalOrNull(aCx).  That matches what I'm doing in bug 991742 anyway.  ;)

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/39b430a304ca

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d7b7dbe3c9d3