1360715 - instanceof behavior cross-contexts doesn't match any other browser

Status: RESOLVED FIXED

(Core :: DOM: Bindings (WebIDL), defect, P3)

Description

[Rick Byers]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/57.0.2987.133 Safari/537.36

Steps to reproduce:

Load this page: http://jsbin.com/yefesuv which basically does:

new MouseEvent("click") instanceof iframe.contentWindow.MouseEvent


Actual results:

instanceof returns true


Expected results:

Expect instanceof to return false because window.MouseEvent.prototype!==iframe.contentWindow.MouseEvent.prototype

MDN even has documentation about this: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/instanceof#instanceof_and_multiple_context_(e.g._frames_or_windows)

I tried current versions of Chrome, Edge and Safari and they all return false as expected.

Comment 1

[Rick Byers]

Originally reported as a Chrome issue https://crbug.com/716423

Reproduced on current nightly (55) and release (52).

Comment 2

[Tom S [:evilpie]]

DOM objects implement their own hasInstance functions that do this kind unwrapping that allows for this behavior.

Comment 3

[Rick Byers]

Any idea why that is done?  I don't see anything in the DOM spec about it.  The lack of consistency with other types and other browsers seems problematic to me.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Any idea why that is done? 

It's been the behavior of Gecko all along, as far as I know.  I _believe_ it was also the Netscape behavior, but I'm not sure.  jst may know for sure there, maybe

That's the backwards compat bit.  The "why would someone ever do it this way?" bit is more to do with being able to tell whether an object is of a certain "type".  The fact that this doesn't really work cross-global for a bunch of ES objects is a problem too; enough of one that ES added isArray() to enable such a cross-global test...

If you ask ES folks about this, they will say that obviously instanceof is the wrong tool for testing "type" and people should write various ad-hoc tests using the fact that certain functions throw when called with the wrong "this" object.  Of course then you better hope that any "type" you want to detect has such functions that are side-effect free when the object _is_ the right type...  Obviously, this is not a great suggestion for authors in practice.

> I don't see anything in the DOM spec about it.

It's in Web IDL, since that defines the interactions with ES.  See https://heycam.github.io/webidl/#es-interface-hasinstance

In ES6 terms, this would be done via @@hasInstance; see step 3 of https://tc39.github.io/ecma262/#sec-instanceofoperator

> The lack of consistency with other types

To be clear, the lack of consistency is between ES-defined things and IDL-defined things, right?

> and other browsers seems problematic to me.

It's definitely problematic, I agree!

The Gecko behavior is pretty simple to spec, and arguably more useful to authors; that's why it ended up in the Web IDL spec initially.  But other engines have been pushing back on implementing it.  See https://github.com/heycam/webidl/issues/129 and various comments (mostly from ES spec committee members) about this stuff that I unfortunately can't locate right now.

It's been many years now that everyone agrees the cross-global object type identification case needs solving and no one is stepping up and offering a solution other than the one Firefox has...

We've considered giving up and removing the behavior on our end, but it hasn't been a high priority, especially since it's depended on by various bits of our browser UI and extensions, so removing it is not a trivial project.  And it would be quite annoying to remove it and then reinstate if people actually sit down to sort out the type-identification problem like they should have years ago and suddenly decide instanceof is the right tool after all.

Comment 5

[Rick Byers]

Thanks Boris!  I should have known Firefox would be match

Comment 6

[Rick Byers]

Whoops, sorry.
As I was saying, I should have known Firefox would be matching the spec.  Updated the summary to indicate that from Firefox's perspective the bug is that Firefox is different from everyone else.  I've filed https://crbug.com/717016 to track that blink doesn't match the spec.

I'll try to find someone on the chromium team to drive debate in https://github.com/heycam/webidl/issues/129

Comment 7

[Boris Zbarsky [:bzbarsky]]

I appreciate the vote of confidence, but we do have just plain bugs at times too.  ;)

Thank you for pushing on the convergence issue.  It would be good to put this whole thing to rest!

Comment 8

[Domenic Denicola]

The spec has updated to remove the custom behavior here. Tests have been merged in https://github.com/w3c/web-platform-tests/pull/5762 and are available at http://w3c-test.org/WebIDL/ecmascript-binding/has-instance.html.

Comment 9

[Johnny Stenback (:jst)]

Yeah, my memory agrees with bz's here wrt this going as far back as Netscape... I do remember dealing with this very issue in the age old XPCDOM bindings work that I, and several others, did back in ~2000, IIRC the two driving functions then was backwards compatibility and also requests from front end engineers who wanted to do instanceof checks on objects in a content window. From an ergonomics perspective I do lean towards the Firefox behavior on this one.

Comment 10

[Johnny Stenback (:jst)]

Consistency across implementations do of course trump ergonomics on an issue this old though.

Comment 11

[Andrew McCreight [:mccr8]]

wpt/WebIDL/ecmascript-binding/has-instance.html is a WPT test that Firefox fails and all other browsers pass.

Comment 12

[Andrew McCreight [:mccr8]]

Bug 993030 sounds related or is maybe even a dupe.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> Bug 993030 sounds related or is maybe even a dupe.

Not even related.  This bug is about instanceof where the RHS is a IDL interface object.  That bug is about the RHS being a CCW for a bound version of a plain JS function.

Comment 14

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: Bug 1360715 - Part 1: Hide @@hasInstance for IDL interfaces behind a flag r=edgar

Comment 15

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: Bug 1360715 - Part 2: Modify instanceofs in tests to non-cross-context r=edgar

Depends on D106661

Comment 16

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: Bug 1360715 - Part 3: Remove remaining cross-context instanceof from tests r=edgar

Depends on D106662

Comment 17

[Tom S [:evilpie]]

Will this change affect WebExtension content-scripts?

Comment 18

[Kagami Rosylight [:saschanaz] (they/them)]

If they do cross-context instanceofs, yes.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fb8b9841d82b
Part 1: Hide @@hasInstance for IDL interfaces behind a flag r=edgar
https://hg.mozilla.org/integration/autoland/rev/0e4b1b65fcbe
Part 2: Modify instanceofs in tests to non-cross-context r=edgar
https://hg.mozilla.org/integration/autoland/rev/b4a14c42313d
Part 3: Remove remaining cross-context instanceof from tests r=edgar

Comment 20

[Cristian Brindusan [:cbrindusan]]

Backed out 3 changesets (bug 1360715) for mochitest failures at test_WebCrypto.html.
https://hg.mozilla.org/integration/autoland/rev/c543a06e106b185101ff28fcceb35890d8ba42c8

Push with failures:
https://treeherder.mozilla.org/jobs?repo=autoland&revision=b4a14c42313d5f77043c045c3f2a7838430a4978&selectedTaskRun=R35SdK6NSAGR-YOpQa0PTw.0

Failure log:
https://treeherder.mozilla.org/logviewer?job_id=332086653&repo=autoland&lineNumber=2526

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/182c7bf906b0
Part 1: Hide @@hasInstance for IDL interfaces behind a flag r=edgar
https://hg.mozilla.org/integration/autoland/rev/a8f556e08545
Part 2: Modify instanceofs in tests to non-cross-context r=edgar
https://hg.mozilla.org/integration/autoland/rev/39aee1028caf
Part 3: Remove remaining cross-context instanceof from tests r=edgar

Comment 22

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/182c7bf906b0
https://hg.mozilla.org/mozilla-central/rev/a8f556e08545
https://hg.mozilla.org/mozilla-central/rev/39aee1028caf

Comment 23

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/27997 for changes under testing/web-platform/tests

Comment 24

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot