304423 - (window instanceof Object) returns false

Status: RESOLVED FIXED

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

Description

[shutdown]

Yet another regression from bug 296639.

Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b4) Gecko/20050730 Firefox/1.0+
javascript: alert(window instanceof Object);
  => true
javascript: p=window; while(q=p.__proto__)p=q; alert(p===Object.prototype);
  => true

Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b4) Gecko/20050731 Firefox/1.0+
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b4) Gecko/20050811 Firefox/1.0+
javascript: alert(window instanceof Object);
  => false
javascript: p=window; while(q=p.__proto__)p=q; alert(p===Object.prototype);
  => false

And something that might be related to the above:

1. load http://www.mozilla.org/
2. load javascript: Object.prototype.x="oops"; void 0;
3. load http://www.google.com/intl/en/
4. load javascript: alert(x);

build 20050730 => "Error: x is not defined" in JS console
build 20050811 => alerts "oops"

Comment 1

[Johnny Stenback (:jst)]

I just split out the Object.prototype leak problem into bug 304459.

Comment 2

[shutdown]

Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b4) Gecko/20050811 Firefox/1.0+

javascript: alert(window.__proto__);
=> alerts "[xpconnect wrapped native prototype]"
javascript: alert(window.__proto__ === Window.prototype);
=> alerts "false" (this should be "true")

Comment 3

[Boris Zbarsky [:bzbarsky]]

I think we need to get this fixed...

Comment 4

[Brendan Eich [:brendan]]

Yeah, we should fix this.

One way would be to add an innerObject hook and use that in instanceof's
underpinnings in the engine (shared with an equivalent API; my point is, the fix
goes in common code under the interpreter and the JS API, not only in the
interpreter).

But that's fixing just the condition mentioned in the Summary, (window
instanceof Object).  What about the other consequences of the prototype chain
formerly (and apparently in other browsers) running from window to
Object.prototype?  Even with the spot-fix described above, Object.prototype.foo
= 42 will not cause (in the absence of shadowing) window.foo === 42.

What standard class objects and constructors does the outer window object bind
as properties, if any?  To which Object.prototype does the outer window delegate
anyhoo -- a private one?

/be

Comment 5

[Johnny Stenback (:jst)]

I'm *hopeful* that this can be fixed with some JS_ClearScope fun on the outer
window in the right place in nsGlobalWindow::SetNewDocument(), the outer should
never really have any properties on *it* as long as there's an inner window, it
should all be forwarded to the inner, and it should all just work. I'm thinking
on how to get that done...

Comment 6

[Brendan Eich [:brendan]]

The instanceof test, and property delegation too, use the [[Prototype]] (ECMA
name, JSSLOT_PROTO in SpiderMonkey) internal slot, surfaced as __proto__ via a
getter and setter.

So property fwd'ing won't cut it here -- to fix this bug in some similar way,
you would want the outer window to delegate via its proto slot to the inner's
Object.prototype.  This would mean changing the outer window's proto slot value
on each navigation step.

Would this have any bad security implications?  The cross-window checks would
still apply.  The outer window would still be a persistent handle.  Its
__proto__ or properties reached from that object (the current inner window's
Object.prototype) could not be used across different origins anyway, so it
shouldn't matter that the outer window's prototype changes when navigating.

/be

Comment 7

[Johnny Stenback (:jst)]

That *should* be all fine, the otuer window object already gets a new prototype
at every navigation
(http://lxr.mozilla.org/mozilla/source/dom/src/base/nsJSEnvironment.cpp#1620),
but we'd need to mess with the proto's proto and parent to essentially move the
XPConnect prototype into the inner window's scope... Not sure what XPConnect
will think about that, but I plan to experiment...

Comment 8

[Johnny Stenback (:jst)]

Attachment: Share prototypes between inner and outer windows

This fixes all instanceof bugs I know of. It does this by making the inner
window use the outer's prototype (which is re-created when we create a new
inner window). To get this to work right I had to add a new method to
nsIXPConnect to be able to tell XPConnect to restore the old prototype for a
class in a scope so that when we go restore window state we can get the outer
window to use its old XPCWrappedNativeProto. This proto restoring API is not
necessarily conceptially ideal (since it only restores it for a given class,
not scope wide), but it does what we need now and I don't see the need nor have
the time to do this any differently at this point.

Comment 9

[Johnny Stenback (:jst)]

Attachment: Same thing with a few problems found by mrbkap fixed

Comment 10

[Johnny Stenback (:jst)]

*** Bug 305153 has been marked as a duplicate of this bug. ***

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 194493 [details] [diff] [review]
Same thing with a few problems found by mrbkap fixed

r=mrbkap

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 194493 [details] [diff] [review]
Same thing with a few problems found by mrbkap fixed

Keep an eye on Tp, ok?	I'm pretty convinced the extra JS_ClearScope won't
count above noise, but it wrankles.  Maybe for 1.9 we can simplify all this
somehow.

/be

Comment 13

[Johnny Stenback (:jst)]

This landed last night on the trunk, but it turned the tinderboxes that ran the
Txul tests orange. The reason for that is that the Txul page does a
document.write() in its onload handler and that ends up creating a new inner
window. After that, the calling script's scope has a prototype that differs from
the outer's new prototype, and because of that XPConnect fails to find the
wrapped native when it calls window.close(). Because of that this was backed
out. Fix coming up.

No noticable Tp hit apparent in the few cycles that this went through though.

Comment 14

[Johnny Stenback (:jst)]

Attachment: Fix XPCWrappedNative::GetWrappedNativeOfJSObject().

This makes XPCWrappedNative::GetWrappedNativeOfJSObject() return a wrapper on
the prototype chain even if the wrapper's XPCWrappedNativeProto doesn't match
the one found through funobj as long as the funobj proto and the wrapper are
from the same scope and their classinfo matches.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 194552 [details] [diff] [review]
Fix XPCWrappedNative::GetWrappedNativeOfJSObject().

I can't think of a case where two objects in the same scope will have the same
classinfo, so r=mrbkap

Comment 16

[Brendan Eich [:brendan]]

Comment on attachment 194552 [details] [diff] [review]
Fix XPCWrappedNative::GetWrappedNativeOfJSObject().

This is good for the trunk, but should bake a bit (again -- we are in deep dark
waters these last few days, here and in other hard content-side Gecko bugs!
;-).

/be

Comment 17

[Johnny Stenback (:jst)]

Both patches landed on the trunk.

Comment 18

[Asa Dotzler [:asa]]

Johnny, can you resolve this as Fixed to trigger a trunk verification and then
get it landed on the branch?

Comment 19

[Johnny Stenback (:jst)]

Fixed on the branch now too.

Comment 20

[seva.luchianov]

This is broken again in 66.0b2 (64-bit)
Just running (window instanceof Object) in the console will return false

Comment 21

[seva.luchianov]

Created new bug for this issue here https://bugzilla.mozilla.org/show_bug.cgi?id=1523139