612267 - Methods added to Window.prototype don't show up on window

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[T. Brains]

User-Agent:       Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 6.0; Trident/4.0; SLCC1; .NET CLR 2.0.50727; .NET CLR 3.0.30729; .NET CLR 3.5.30729; .NET4.0C)
Build Identifier: 4.0b7

After adding a method to Window.prototype, the method doesn't show up on the existing window object. Similarily, getters/setters or other fields don't show up either.

This is a regression in 4.0b7, and is confirmed to work as expected in 4.0b6 and 3.5.

Reproducible: Always

Steps to Reproduce:
Use the following code:

  Window.prototype.testMethod=function () { alert("foo") };

  window.testMethod();

Actual Results:  
Throws a "window.testMethod is not a function" error.

Under 4.0b6 and 3.5, alerts "foo".

Expected Results:  
Should always alert "foo".

Comment 1

[Mounir Lamouri (:mounir)]

Working with 3.6 FWIW.

Comment 2

[Mounir Lamouri (:mounir)]

Attachment: Testcase

Testcase should show "foo". Showing an error message on trunk.

Comment 3

[T. Brains]

Sorry, that should've been 3.6 in my original report.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Yeah, Window.prototype is no longer on the __proto__ chain for |window|.  In particular, I see:

  window.__proto__.__proto__.__proto__.__proto__ === null
  window.__proto__.__proto__.__proto__ === Object.prototype
  window.__proto__.__proto__ === global scope polluter
  window.__proto__ == some XPConnect prototype (according to toString()), but not
                      Window.prototype

Blake?

Comment 5

[Johnny Stenback (:jst)]

Whether this is by design (which I suspect it is) or something that needs to be fixed is one question that we at least need answered before we ship.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix

I *think* that the real fix here was to fix up the outer window's prototype, but I also think that the RefreshProtype had been misplaced to begin with.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix v1.1

So, the real problem was that the outer window's (proxy's) prototype was not being updated when we refreshed the prototype of the inner window. Then, when we called ConnectToInner, we would overwrite the new prototype with the old one, leading to the inconsistent state found here.

I also moved the RefreshPrototype call into SetOuterObject since that's called on every pageload instead of just the first (as it should have been from the beginning). The real bug fix is the addition of the JS_SetPrototype in SetOuterObject. The assertion that replaces the code in ConnectToInner would have caught the bug much more easily.

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 496987 [details] [diff] [review]
Proposed fix v1.1

Looks good, but when reading through this I did notice that nsJSContext::InitOuterWindow() is unused now (afaict), so please file a followup bug to delete it (impl and interface definition).

Comment 9

[Ed Morley [:emorley]]

Since this bug had the regressionwindow-wanted keyword set (probably redundant given the r+ patch, but didn't want to remove the keyword without actually giving a regression range in case it was intentional).

Last good nightly: 2010-10-13 
First bad nightly: 2010-10-14

Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f6e81dd5a125&tochange=ad0a0be8be74

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Thanks Ed. That shows that it was the compartments landing that broke this. That which makes sense, since compartments introduced the crazy RefreshProtoype dance that I had to fix here.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/tracemonkey/rev/b445e3e2dff0

Comment 12

[Mounir Lamouri (:mounir)]

Pushed:
https://hg.mozilla.org/mozilla-central/rev/307f6c854845

Comment 13

[Mounir Lamouri (:mounir)]

It's actually:
https://hg.mozilla.org/mozilla-central/rev/b7fbd54c9622