The default bug view has changed. See this FAQ.

Support cross-global |... instanceof DOMInterface|

RESOLVED FIXED in mozilla21

Status

()

Core
DOM
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: peterv, Assigned: peterv)

Tracking

(Blocks: 1 bug)

Trunk
mozilla21
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

4 years ago
Created attachment 710305 [details] [diff] [review]
v1
Attachment #710305 - Flags: review?(bzbarsky)
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
> And that's not possible why?

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

Comment 3

4 years ago
(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 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?
Attachment #710305 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 5

4 years ago
(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).
(Assignee)

Updated

4 years ago
Flags: needinfo?(bzbarsky)
Oh, sorry.  I missed the unwrap in the other InterfaceHasInstance.

I'm still not entirely convinced on the proto chain walk bit....
Flags: needinfo?(bzbarsky)
Blocks: 833446
(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 :)
(Assignee)

Comment 8

4 years ago
Created attachment 711222 [details] [diff] [review]
v2
Attachment #710305 - Attachment is obsolete: true
Attachment #711222 - Flags: review?(bzbarsky)
(Assignee)

Comment 9

4 years ago
Created attachment 711335 [details] [diff] [review]
v3
Attachment #711222 - Attachment is obsolete: true
Attachment #711222 - Flags: review?(bzbarsky)
Attachment #711335 - Flags: review?(bzbarsky)
(Assignee)

Updated

4 years ago
Attachment #711335 - Attachment description: v1 → v3
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.
Attachment #711335 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 11

4 years ago
Created attachment 711849 [details] [diff] [review]
v4
Attachment #711335 - Attachment is obsolete: true
Attachment #711849 - Flags: review?(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.
Attachment #711849 - Flags: review?(bzbarsky) → review+
Blocks: 840401
(Assignee)

Comment 13

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a12a0f8c8be
https://hg.mozilla.org/mozilla-central/rev/9a12a0f8c8be
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla21

Updated

4 years ago
Depends on: 842799
(Assignee)

Updated

4 years ago
Duplicate of this bug: 829774
You need to log in before you can comment on or make changes to this bug.