Closed Bug 1428531 Opened 2 years ago Closed 2 years ago

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

Categories

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

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: heycam, Assigned: heycam)

References

Details

Attachments

(2 files)

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.
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.
Attached patch WIP v0.1Splinter Review
bz, WDYT of this approach?
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #8940427 - Flags: feedback?(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.
Attachment #8940427 - Flags: feedback?(bzbarsky) → feedback+
(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.
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?
> 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 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...
Attachment #8941318 - Flags: review?(bzbarsky) → review+
Blocks: 1427512
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a2979f591ff
Add chrome-only Foo.isInstance static methods to interface objects. r=bz
https://hg.mozilla.org/mozilla-central/rev/3a2979f591ff
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.