1428531 - add a chrome-only function to DOM objects to do a cross-origin instanceof check

Status: RESOLVED FIXED

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

Description

[Cameron McCormack (:heycam)]

Bug 1360715 will make |x instanceof SomeWebIDLInterface| stop working when the x and the interface object are from different contexts.  Having a cross-context instanceof operation is still useful, though, and until there is some spec-defined way to do this we should probably have a chrome-only thing we can call from chrome JS to do this.

Comment 1

[Cameron McCormack (:heycam)]

If we want to add something that would be palatable to spec later, then I think something like:

  Node.isNode(object)

would be OK, since it mirrors Array.isArray(), although that could get quite wordy -- SVGComponentTransferFunctionElement.isSVGComponentTransferFunctionElement(element)?  I haven't thought of any short name that reads well.

Comment 2

[Cameron McCormack (:heycam)]

Attachment: WIP v0.1

bz, WDYT of this approach?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8940427 [details] [diff] [review]
WIP v0.1

>+        return dedent(fill(

fill() already does a dedent(), so no need to dedent() around it.

It would probably be better for codesize to make the entire bit after the isObject() check a shared method which is passed three things: the argument JSObject, the proto id, and the proto depth.  The actual code could then be shared.

There's no real need to use UnwrapObjectInternal here.  Not least because it won't necessarily give you the right answer: if you have a cross-origin Window, Window.isWindow should probably still test true.  Something more similar to what dom::InterfaceHasInstance (the jsnative-argument version) does might be better.

My main concern with this approach is that if we _do_ fail to get this standardized and get something else standardized instead, then finding all callsites will be rather difficult.  I would almost rather we named the function the same easily searchable thing across all interfaces...  Not sure what that should be.

Comment 4

[Cameron McCormack (:heycam)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #3)
> There's no real need to use UnwrapObjectInternal here.  Not least because it
> won't necessarily give you the right answer: if you have a cross-origin
> Window, Window.isWindow should probably still test true.  Something more
> similar to what dom::InterfaceHasInstance (the jsnative-argument version)
> does might be better.

For my own edification (since I'm mostly bumbling around this code here), can you explain why UnwrapObjectInternal doesn't do the right thing for Window objects?

> My main concern with this approach is that if we _do_ fail to get this
> standardized and get something else standardized instead, then finding all
> callsites will be rather difficult.  I would almost rather we named the
> function the same easily searchable thing across all interfaces...  Not sure
> what that should be.

Initially I thought "hasInstance", but that might be misleading since it's exactly [[HasInstance]]'s behaviour of returning false for different-context objects that we're wanting to avoid here.  Maybe that doesn't matter so much, if we're going to rename it later once it's standardised.

Comment 5

[Cameron McCormack (:heycam)]

Attachment: Bug 1428531 - Add chrome-only Foo.isInstance static methods to interface objects.

Review commit: https://reviewboard.mozilla.org/r/211624/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/211624/

Comment 6

[Cameron McCormack (:heycam)]

I copied half of the contents of dom::InterfaceHasInstance here.  I left out the final CallOrdinaryHasInstance -- is that correct, if all I want to do is check the internal branding, and don't want to fall back to prototype chain checks, or is there some case where we have a DOM object but we would fall through to that final CallOrdinaryHasInstance call?

Comment 7

[Boris Zbarsky [:bzbarsky]]

> For my own edification (since I'm mostly bumbling around this code here), can you 
> explain why UnwrapObjectInternal doesn't do the right thing for Window objects?

It does, for same-origin ones.  For cross-origin ones it won't be able to unwrap the security wrapper, and hence would land in the NS_ERROR_XPC_SECURITY_MANAGER_VETO error case.

> I left out the final CallOrdinaryHasInstance

That's correct.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8941318 [details]
Bug 1428531 - Add chrome-only Foo.isInstance static methods to interface objects.

https://reviewboard.mozilla.org/r/211624/#review217392

r=me.  Might be worth posting to .platform to update people on the fact that we did this for now, pending a better branding-check idea.

::: dom/bindings/BindingUtils.cpp:2528
(Diff revision 1)
> +  if (domClass && domClass->mInterfaceChain[depth] == prototypeID) {
> +    args.rval().setBoolean(true);
> +    return true;
> +  }
> +
> +  if (jsipc::IsWrappedCPOW(instance)) {

My gut feeling is you don't need the CPOW bits here.  I really hope we're not using them anymore...

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a2979f591ff
Add chrome-only Foo.isInstance static methods to interface objects. r=bz

Comment 10

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/3a2979f591ff