Closed Bug 1360715 Opened 5 years ago Closed 8 months ago

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

Categories

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

55 Branch
defect

Tracking

()

RESOLVED FIXED
88 Branch
Tracking Status
firefox88 --- fixed

People

(Reporter: rbyers, Assigned: saschanaz)

References

(Blocks 2 open bugs)

Details

(Keywords: parity-chrome, parity-safari)

Attachments

(3 files)

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.
Originally reported as a Chrome issue https://crbug.com/716423

Reproduced on current nightly (55) and release (52).
Component: Untriaged → JavaScript Engine
Product: Firefox → Core
DOM objects implement their own hasInstance functions that do this kind unwrapping that allows for this behavior.
Component: JavaScript Engine → DOM: Core & HTML
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.
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky) → needinfo?(jstenback+bmo)
Thanks Boris!  I should have known Firefox would be match
Summary: instanceof incorrectly returning true across contexts → instanceof behavior cross-contexts doesn't match any other browser
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
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!
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.
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.
Flags: needinfo?(jstenback+bmo)
Consistency across implementations do of course trump ergonomics on an issue this old though.
Priority: -- → P3
Status: UNCONFIRMED → NEW
Ever confirmed: true
wpt/WebIDL/ecmascript-binding/has-instance.html is a WPT test that Firefox fails and all other browsers pass.
Bug 993030 sounds related or is maybe even a dupe.
See Also: → 993030
> 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.
Component: DOM: Core & HTML → DOM: Bindings (WebIDL)
Assignee: nobody → krosylight

Will this change affect WebExtension content-scripts?

Blocks: 1695435

If they do cross-context instanceofs, yes.

Attachment #9205779 - Attachment description: Bug 1360715 - Part 1: Hide @@hasInstance for IDL interfaces behind a flag r=smaug → Bug 1360715 - Part 1: Hide @@hasInstance for IDL interfaces behind a flag r=edgar
Attachment #9205780 - Attachment description: Bug 1360715 - Part 2: Modify instanceofs in tests to non-cross-context r=smaug → Bug 1360715 - Part 2: Modify instanceofs in tests to non-cross-context r=edgar
Attachment #9205781 - Attachment description: Bug 1360715 - Part 3: Remove remaining cross-context instanceof from tests r=smaug → Bug 1360715 - Part 3: Remove remaining cross-context instanceof from tests r=edgar
Pushed by krosylight@mozilla.com:
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
Flags: needinfo?(krosylight)
Pushed by krosylight@mozilla.com:
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
Status: NEW → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → 88 Branch
Regressions: 1696627
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/27997 for changes under testing/web-platform/tests
Upstream PR merged by moz-wptsync-bot
Regressions: 1697829
You need to log in before you can comment on or make changes to this bug.