842799 - |Document.prototype instanceof Node| inside <script> returns false

Status: VERIFIED FIXED

Description

[Alice0775 White]

Created attachment 715739 [details]
testcase

Build Identifier:
http://hg.mozilla.org/mozilla-central/rev/e8f8a3f6f1f6
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130219 Firefox/21.0 ID:20130219031055

Zlip792 reported the problem in http://forums.mozillazine.org/posting.php?mode=quote&f=23&p=12679997 .


alert(Document.prototype instanceof Node);

This happens when running the code in <script>.
This also happens when evaluate the code in Error Console.
However, this problem does not happen when evaluate the code in the Scratchpad and Web Console.

Steps to Reproduce:
1. Open testcase

Actual Results:
 |false|

Expected  Results:
 |true|


Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/7bd555e2acfa
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130215 Firefox/21.0 ID:20130215133452
Bad:
http://hg.mozilla.org/mozilla-central/rev/484dbca61133
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130216 Firefox/21.0 ID:20130216064952
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7bd555e2acfa&tochange=484dbca61133


Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/424de8168602
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130215 Firefox/21.0 ID:20130215141251
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/88361e96d89f
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130215 Firefox/21.0 ID:20130215143355
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=424de8168602&tochange=88361e96d89f

Triggered by:
  9a12a0f8c8be	Peter Van der Beken — Fix for bug 838269 (Support cross-global |... instanceof DOMInterface|). r=bz.

Comment 1

[Boris Zbarsky [:bzbarsky, bz on IRC]]

So... This was a purposeful change.  If we do want this to return true that can be done, I guess.  Otherwise we should ask Microsoft to fix their test.

Cameron, thoughts?

Comment 2

[Cameron McCormack (:heycam)]

So this is testing if an interface prototype object is instanceof the instance object?  Web IDL has never required that to be true, AFAICR.  Despite the fact that built-ins like Number, Date, etc. do behave like this, I think I remember TC39 folks saying that it's not a great pattern to follow.  I think we should ask for the test to be fixed.

I have no idea why this being inside a <script> makes a difference here.  The weirdness that the Web Console does with the global might be confusing matters.

Comment 3

[Boris Zbarsky [:bzbarsky, bz on IRC]]

No, this is testing whether an interface prototype object for interface A is instanceof the interface object for interface B where A inherits from B.

I too have no idea what's different in the web console case; presumably some aspect of the sandbox is making this "work" somehow.

Comment 4

[Peter Van der Beken [:peterv]]

Given http://dev.w3.org/2006/webapi/WebIDL/#es-interface-hasinstance we do need to fix this:

A -> Node
O -> Node.prototype
V -> [[Prototype]] of Document.prototype, which is Node.prototype

O and V refer to the same object.

Attachment 710305 [details] [diff] probably didn't suffer from this flaw.

Comment 5

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Oh, hmm.  So that still has the "walk up the proto chain" bit, I guess?  <sigh>.

Well, that's easy enough to add...

Comment 6

[Peter Van der Beken [:peterv]]

Created attachment 716159 [details] [diff] [review]
v1

Comment 7

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Comment on attachment 716159 [details] [diff] [review]
v1

> +        js::GetObjectClass(proto) == interfacePrototypeClasp) {

Why do we want this bit?  The spec just says to check for object identity, no?

But also, how does this deal with cross-compartment __proto__ links?  Seems like we should be comparing "proto" to the reflection of interfacePrototype into the compartment of instance or something.

r=me with those fixed.

Comment 8

[Lukas Blakk [:lsblakk] use ?needinfo]

Does this affect users at all or just running a test case? Not clear why this would be a release blocker.

Comment 9

[Boris Zbarsky [:bzbarsky, bz on IRC]]

It can affect website compat, and it's a correctness issue that shows up on commonly run correctness tests.

Given that we have a patch, I believe we should just make sure we actually get that patch landed.  If by some miracle we get to release day and that hasn't happened, I wouldn't block the release on this, probably, given the info we have so far.

Comment 10

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f2f90b7c78b

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9f2f90b7c78b

Comment 12

[Peter Van der Beken [:peterv]]

Created attachment 722186 [details] [diff] [review]
v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 838269
User impact if declined: broken sites
Testing completed (on m-c, etc.): landed on m-c, includes a test
Risk to taking this patch (and alternatives if risky): low-risk
String or UUID changes made by this patch: none

Comment 13

[bhavana bajaj [:bajaj]]

Comment on attachment 722186 [details] [diff] [review]
v1

low risk fix which may help with web compat issues .Patched is well baked on m-c, approving for uplift

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/25063560925c

Comment 15

[Maria Grazia Alastra [:mga]]

Verified fixed on Windows 7, 64 bits, with Firefox 22 beta 2, Aurora 23.0 and latest Nightly 24.0:

- User agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0, Build ID: 20130521223249

- User agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20130526 Firefox/23.0, Build ID: 20130526004017

- User agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:24.0) Gecko/20130525 Firefox/24.0, Build ID: 20130525062525