899804 - CPOWs don't handle instanceof with WebIDL interfaces

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

We handle instanceof correctly for XPCOM interfaces, but not for WebIDL interfaces. A lot more stuff is now using WebIDL, so we need a way to do this properly.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: webidl-instanceof

This patch does for WebIDL what we do for XPCOM interfaces (in XPCJSID.cpp). To implement |x instanceof Interface| when |x| is a CPOW and |interface| is in WebIDL, it sends an urgent (synchronous) IPC message to the child process with the interface's prototypeID and depth information. The child does the check on |x|'s counterpart and returns the result.

The normal BindingUtils code for HasInstance also does a proto chain walk. I don't think there's a way we can do that across processes so I left it out. Hopefully it's not too important.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 784501 [details] [diff] [review]
webidl-instanceof

> Hopefully it's not too important.

It can't possibly be if x is a CPOW and Interface is not, since then the proto chain of x in fact will never hit Interface.prototype.

>+++ b/dom/bindings/BindingUtils.cpp
>+  JS::Rooted<JSObject*> unwrapped(cx, js::CheckedUnwrap(instance, true));
>+  if (mozilla::jsipc::JavaScriptParent::IsCPOW(unwrapped)) {

Can't "unwrapped" be null on that second line?  Should probably check that.

r=me as far as it goes, but this leaves broken the instanceof hooks for things like SVGUnitTypes, which don't actually use InterfaceHasInstance...

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 784501 [details] [diff] [review]
webidl-instanceof

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

Nice.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: cpow-fix

I forgot this piece. It's needed if we send a CPOW back to the child process, and the CPOW was wrapped in a CCW. Interestingly, this happens in the context menu code.

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 784501 [details] [diff] [review]
webidl-instanceof

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

::: dom/bindings/BindingUtils.cpp
@@ +1742,5 @@
>      return true;
>    }
>  
> +  JS::Rooted<JSObject*> unwrapped(cx, js::CheckedUnwrap(instance, true));
> +  if (mozilla::jsipc::JavaScriptParent::IsCPOW(unwrapped)) {

There's no need for the 'mozilla::'

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/70ac9672075b

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Backed out for some weird build errors on Android.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e42f623fd574

Comment 8

[Boris Zbarsky [:bzbarsky]]

Please do file a followup for the last part of comment 2, if you don't plan to fix it here.  Again, this patch only fixes instanceof for _some_ WebIDL interfaces.

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/123ae8924326

I filed bug 901164 for the SVG stuff.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/123ae8924326