838269 - Support cross-global |... instanceof DOMInterface|

Status: RESOLVED FIXED

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

Description

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 1

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

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

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

::: dom/bindings/BindingUtils.cpp
@@ +1495,5 @@
> +JSBool
> +InterfaceHasInstance(JSContext* cx, JSHandleObject obj, JSObject* instance,
> +                     JSBool* bp)
> +{
> +  jsval protov;

Nit: JS::Value

@@ +1497,5 @@
> +                     JSBool* bp)
> +{
> +  jsval protov;
> +  DebugOnly<bool> ok = JS_GetProperty(cx, obj, "prototype", &protov);
> +  MOZ_ASSERT(ok, "Someone messed with our prototype property?");

And that's not possible why? I'd like to see a test that tries to trip this, at least

Comment 2

[Boris Zbarsky [:bzbarsky]]

> And that's not possible why?

Because the "prototype" property on interface objects is readonly and non-configurable.

Comment 3

[Peter Van der Beken [:peterv]]

(In reply to :Ms2ger from comment #1)
> I'd like to see a test that tries to trip this, at least

You mean like this:

http://mxr.mozilla.org/mozilla-central/source/dom/imptests/idlharness.js#770

?

Comment 4

[Boris Zbarsky [:bzbarsky]]

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

If InterfaceHasInstance is called directly from JS, won't "instance" be a security wrapper for the real object?  Or does JSAPI really call the hasInstance hook with "obj" and "instance" in different compartments?  Seems like there's an unwrap missing here.

>+  if (!JS_GetPrototype(cx, instance, &proto)) {

So what I'd been thinking was that we'd just test that "instance" implements the right thing.  That means just checking for the right protoid in the right place in the array if the interface is castable, and doing some sort of QI to the right thing if it's not...  In particular, that would work even if someone messes with the proto chain on a DOM object.

>     def needsConstructHookHolder(self):
>         assert self.interface.hasInterfaceObject()
>+        return False

Why do we still need this method?

Comment 5

[Peter Van der Beken [:peterv]]

(In reply to Boris Zbarsky (:bz) from comment #4)
> If InterfaceHasInstance is called directly from JS, won't "instance" be a
> security wrapper for the real object?  Or does JSAPI really call the
> hasInstance hook with "obj" and "instance" in different compartments?  Seems
> like there's an unwrap missing here.

The two callers of the function that takes "instance" unwrap, so I'm not sure what you mean.

> Why do we still need this method?

I can remove it if you prefer. I'm not sure if that's best, since we might be switching back to real JS function objects in the future (and would need a construct hook holder then).

Comment 6

[Boris Zbarsky [:bzbarsky]]

Oh, sorry.  I missed the unwrap in the other InterfaceHasInstance.

I'm still not entirely convinced on the proto chain walk bit....

Comment 7

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

(In reply to Boris Zbarsky (:bz) from comment #2)
> > And that's not possible why?
> 
> Because the "prototype" property on interface objects is readonly and
> non-configurable.

(In reply to Peter Van der Beken [:peterv] from comment #3)
> (In reply to :Ms2ger from comment #1)
> > I'd like to see a test that tries to trip this, at least
> 
> You mean like this:
> 
> http://mxr.mozilla.org/mozilla-central/source/dom/imptests/idlharness.js#770
> 
> ?

Excellent, thanks :)

Comment 8

[Peter Van der Beken [:peterv]]

Attachment: v2

Comment 9

[Peter Van der Beken [:peterv]]

Attachment: v3

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 711335 [details] [diff] [review]
v3

I thought we'd decided to add some sort of sanity checking on castability (because testing proto ids won't work for non-castable interfaces)...  What happened to that?

r=me modulo that bit.

Comment 11

[Peter Van der Beken [:peterv]]

Attachment: v4

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 711849 [details] [diff] [review]
v4

>+        # Grab the includes for the things that involve hasXPConnectImpls

This may want to grab things for NeedsGeneratedHasInstance instead.  Or we need to get that header _somehow_ for the case when the descriptor we're actually generating is consequential for some other interface...

>+        elif descriptor.workers:
>+            templateBody += "${declName} = &${val}.toObject();"
>+        elif not descriptor.interface.isConsequential() and not descriptor.interface.isExternal():

I'd prefer we reverse the order of those, so non-consequential non-external interfaces keep unwrapping to the concrete C++ type on workers.

> class FakeCastableDescriptor():

You actually don't need that anymore.  It was only needed so we could pretend to be castable no matter what.  In all other ways it behaves just like its "descriptor" constructor argument, so you can just remove this class and pass that descriptor at the one callsite instead.

>+++ b/dom/bindings/test/TestCodeGen.webidl

Could we replace the tests that used TestNonCastableInterface with tests using IndirectlyImplementedInterface, please?

You should remove the comments describing 'castable' in the big Bindings.conf comment.  

r=me with those nits.

Comment 13

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a12a0f8c8be

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9a12a0f8c8be