Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should probably spec a hasInstance for DOM interface objects #129

Closed
bzbarsky opened this issue Jun 7, 2016 · 14 comments · Fixed by #356
Closed

Should probably spec a hasInstance for DOM interface objects #129

bzbarsky opened this issue Jun 7, 2016 · 14 comments · Fixed by #356
Labels
☕☕☕ difficulty:hard Excruciating ⌛⌛ duration:medium Shouldn't be too long to fix

Comments

@bzbarsky
Copy link
Collaborator

bzbarsky commented Jun 7, 2016

Right now we have a [[HasInstance]] thing at http://heycam.github.io/webidl/#es-interface-hasinstance but that's gone in ES6.

Proposed prose with similar semantics is something like:

  1. Let F be the this value.
  2. Let V be the first argument we were passed.
  3. If F is not an object, return false.
  4. If F is not an interface object, return false (or delegate to OrdinaryHasInstance?)
  5. If V is not an object, return false.
  6. If V is an object that implements the interface for which F is the interface object (defined in terms of brands, whatever), return true.
  7. Return OrdinaryHasInstance(F, V).

@domenic, thoughts?

An open question is whether Node[Symbol.hasInstance] == Element[Symbol.hasInstance] should test true or false. We could go either way, given the above prose, since they would implement the same algorithm...

@bzbarsky
Copy link
Collaborator Author

bzbarsky commented Jun 7, 2016

If F is not an interface object, return false (or delegate to OrdinaryHasInstance?)

Fwiw, the behavior I'm going to do in Firefox for now is to delegate.

@domenic
Copy link
Member

domenic commented Jun 7, 2016

Can you clarify the intent of the existing [[HasInstance]]? I'm unable to understand in what ways it is different from OrdinaryHasInstance.

Is the test "is a platform object that implements the interface for which O is the interface prototype object" meant to work cross-realm? It seems like youre revised version might be or might not be, depending on whether brands are cross-realm? If that's the desired semantics, how many browsers implement that?

Web IDL's current algorithm performs an observable Get of the "prototype" property, as does OrdinaryHasInstance. Both also do observable proto-walks. Your algorithm omits these. Is changing that a good idea?

@bzbarsky
Copy link
Collaborator Author

bzbarsky commented Jun 7, 2016

Is the test "is a platform object that implements the interface for which O is the interface prototype object" meant to work cross-realm?

Yes. This is in fact the entire intent of the existing [[HasInstance]].

It seems like youre revised version might be or might not be, depending on whether brands are cross-realm?

They better be, because DOM functions can be .call-ed cross-realm, yes?

If that's the desired semantics, how many browsers implement that?

I believe Firefox is the only one that does. That said, this is the behavior people agreed on when this was discussed on es-discuss several years ago... I can't help it if other browsers didn't make implementing Web IDL a very high priority. ;)

I agree that this does cause problems in terms of potential compat issues for whoever switches behavior.

Web IDL's current algorithm performs an observable Get of the "prototype" property

It's not observable: the object the Get is done on in the current algorithm is guaranteed to be a Web IDL interface object, which has such a property defined as a readonly non-configurable value property. At least modulo whatever happens with namespaces.

Both also do observable proto-walks. Your algorithm omits these.

My algorithm does the proto walk. It's right there in step 7.

@annevk
Copy link
Member

annevk commented Jun 7, 2016

(I'm still wary of this as it complicates moving objects between IDL and ECMAScript a bit. As happened with ArrayBuffer and friends, Promise in Gecko, and might happen for other objects in the future.)

@bzbarsky
Copy link
Collaborator Author

bzbarsky commented Jun 7, 2016

I don't recall where all this was discussed offhand. It's been a long time.

I agree that the insistence of ES on having no way to tell whether an object is branded in particular ways, which is partly a legacy of it pretending there is only one global and partly due to people philosophically objecting to branding afaict is a pain for everyone involved. It's a pain that was solved for Array via isArray and explicitly punted on for all sorts of other objects for years now....

@bzbarsky
Copy link
Collaborator Author

bzbarsky commented Jun 7, 2016

Oh, and what it means in practice is that people start relying on brittle Object.prototype.toString.call hacks, but ES6 is breaking those too, by making that sort of thing hookable...

@travisleithead
Copy link
Member

Just happened to be reviewing some WebIDL tests and found a strange inconsistency. Not sure what behavior is desired (in case 3)...

In Firefox 46:

// "w" is a cross-realm windowproxy
test(function() {
  assert_true(document.implementation instanceof w.DOMImplementation);
}, "a DOMImplementation object is considered to be instanceof anotherWindow.DOMImplementation");
// This passes

This also passes (because of OrdinaryHasInstance?):

test(function() {
  var object = Object.create(document.implementation);
  assert_true(object instanceof DOMImplementation);
}, "an object whose [[Prototype]] is DOMImplementation.prototype is considered to be instanceof window.DOMImplementation");

However--and this is the oddity--the following fails:

test(function() {
  var object = Object.create(document.implementation);
  assert_true(object instanceof w.DOMImplementation);
}, "an object whose [[Prototype]] is DOMImplementation.prototype is considered to be instanceof anotherWindow.DOMImplementation");

This seems like it should also pass, because case 2 passed...We'll need an algorithm tweak to handle this.

@bzbarsky
Copy link
Collaborator Author

@travisleithead the Firefox behavior is basically the one described in #129 (comment). Your case 1 passes because of step 6, and case 2 passes because of step 7. But case 3 doesn't match in step 6 and as far as OrdinaryHasInstance is concerned that object has nothing to do with w.DOMImplementation.

I don't see any need for case 3 to pass in practice, honestly.... I think case 2 passing may be needed for web compat. Or at least I would need some convincing that it's not needed.

@tobie tobie added ⌛⌛⌛ duration:long There goes your week-end ☕☕☕ difficulty:hard Excruciating ⌛⌛ duration:medium Shouldn't be too long to fix and removed ⌛⌛⌛ duration:long There goes your week-end labels Jun 18, 2016
@esprehn
Copy link

esprehn commented Jun 25, 2016

I'd really rather we didn't do this, if we want branding checks we can add some kind of separate system to do the slow thing (Platform.isInstance or something), but I don't want to mess with instanceof at the platform level.

See also my reply here:
https://groups.google.com/a/chromium.org/d/msg/blink-reviews-bindings/6qzaSWen21c/OU792ZXFAwAJ

@travisleithead
Copy link
Member

FWIW, in the case I just heard, there was a need to see if an object was an instance of the Window interface cross-realm. So, if there's to be some sort of Window.isInstance(crossOriginIframe.contentWindow) then it must still return true for my use case (even if @@toStringTag from cross-origin makes it seem otherwise).

@domenic
Copy link
Member

domenic commented Mar 3, 2017

Is your case one that occurs on the web, or just in developer tools that are written by browser vendors?

@annevk
Copy link
Member

annevk commented Apr 7, 2017

This relates to #226 as well. I created a simple test

<script src=/resources/testharness.js></script>
<script src=/resources/testharnessreport.js></script>
<div id=log></div>
<iframe></iframe>
<script>
test(() => {
  assert_true(document.body instanceof self[0].Element)
})
</script>

and other than Firefox nobody passes it. In the threads from 2013 only Travis expressed support as coming from another browser and Allen Wirfs-Brock specifically raised the point that if we want to solve this we should aim to solve it for JavaScript and the platform, not just platform objects. That point still makes a lot of sense and is also echoed above by @esprehn. If we need branding we should not just have it for arrays.

So the current state of things would actually argue for removing this from IDL, since there's only a single implementer.

@annevk
Copy link
Member

annevk commented Apr 7, 2017

See also the feedback on overriding instanceof from back in 2011: https://www.w3.org/Bugs/Public/show_bug.cgi?id=12295 ("please don't").

domenic added a commit that referenced this issue May 1, 2017
This was only ever implemented in Firefox. It was added in 5982ce7. after being removed as a result of https://www.w3.org/Bugs/Public/show_bug.cgi?id=12295.

Closes #129.
domenic added a commit that referenced this issue May 4, 2017
This was only ever implemented in Firefox for most platform interface objects. It was added in 5982ce7. after being removed as a result of https://www.w3.org/Bugs/Public/show_bug.cgi?id=12295.

The behavior for callback interface objects was never implemented anywhere.

Closes #129.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
☕☕☕ difficulty:hard Excruciating ⌛⌛ duration:medium Shouldn't be too long to fix
Development

Successfully merging a pull request may close this issue.

6 participants