500691 - Function.prototype no longer extends native functions

Status: VERIFIED FIXED

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

Description

[Mark]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1) Gecko/20090616 Firefox/3.5
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1) Gecko/20090616 Firefox/3.5

This is either a regression or a security change, but I can't find any documentation that it was changed on purpose.  Run:


javascript:Function.prototype.foo=10;alert((function(){}).foo);alert(alert.foo);

You "should" get two alerts, that both say "10".  In FF3.5, the 2nd alert says undefined.

In FF3.0, Safari4, IE7, IE8 you get two alerts that say 10.


Further, all functions under document seem to behave as expected:

document.write
document.writeln
document.createElement
...etc

Reproducible: Always

Comment 1

[Mark]

In short, Function.prototype doesn't seem to extend native functions under the window object.

Comment 2

[Ria Klaassen (not reading all bugmail)]

Regression range: http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2008-08-21+14%3A00%3A00&enddate=2008-08-21+19%3A00%3A00

Comment 3

[Brendan Eich [:brendan]]

WFM on mozilla-central nightly, built from

http://hg.mozilla.org/mozilla-central/rev/92b6868ef3f4

On what release(s) with what exact buildconfig tips is this broken?

/be

Comment 4

[Mark]

OSX: Nightly build fails for me (3.6a1, 20090626), as does 3.5b (20090616).

Windows: 3.5 (20090624) fails.

Comment 5

[Mark]

these are just base installs, no options changed and I didn't build them myself.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Mark, can you attach the testcase you're using?

Comment 7

[Mark]

Literally, it's that javascript: URI, or:

javascript:Function.prototype.foo=10;alert((function(){}).foo + ', ' + alert.foo);

I get:

"10, undefined"

javascript:Function.prototype.foo=10;alert((function(){}).foo + ', ' + document.write.foo);

gives: "10, 10"

Comment 8

[Mark]

or more simply:

javascript:Function.prototype.foo=10;alert(alert.foo?'Success!':'Fail!');

Comment 9

[Jeff Walden [:Waldo]]

javascript:alert(Function===alert.constructor?'Success!':'Fail!')

javascript:alert(Function.prototype===alert.__proto__?'Success!':'Fail!')

Blake is investigating since this is probably more up his alley.

Comment 10

[Jeff Walden [:Waldo]]

Blake said he had a proto-patch that might fix the bug but still needs a good bit of work.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

This is a regression from bug 451154. What happens is that we initialize function and object classes on the *outer* window and (after connecting the inner and outer objects) call XPCWrappedNativeScope::SetGlobal(outer window). This function looks up Function.prototype on the global. Before bug 451154, looking up Function would call the getter nsWindowSH::GetProperty, forwarding the property lookup to the inner window and retrieving the correct value. Now, there is no such getter, and the function is effectively a no-op. I suspect that we can fix this bug by calling ClearScope on the outer window earlier in SetNewDocument.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix

The comments in the patch should hopefully explain this...

Basically, there's a call in XPConnect (XPCWrappedNativeScope::SetGlobal) that looks up Function.prototype and Object.prototype on the global and we were not calling ClearScope in the right spot. A tunnel-vision view of nsGlobalWindow::SetNewDocument with the stuff that mhammond moved out to nsJSEnvironment.cpp inlined looks like:

SetNewDocument():
  - ClearScope(outer window)
  - Maybe free inner objects(now-old inner window)
  * WrapNative(newInnerWindow)
  - outer window.mInnerWindow = new nsGlobalWindow()
  1 ClearScope(outer window)
  2 xpconnect->InitClasses(outer window)
  - Do prototype fixup dance with outer & new inner window

* Causes Function and Object to be re-created on the outer window.

Before this patch, 1 and 2 above were reversed, meaning that the call to SetGlobal was effectively a no-op.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

I'll attach a mochitest tomorrow (out of time tonight).

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: With mochitest

As above, but with a mochitest.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 386324 [details] [diff] [review]
With mochitest

>@@ -1386,16 +1386,17 @@ NS_IMPL_CYCLE_COLLECTION_CLASS(nsJSConte
>+  (void)tmp;

Why?

>+ok(Function===alert.constructor, "alert's constructor is our Function");

Worth it to also check window.Function here?

With those nits, looks scary but ok.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #15)
> >+  (void)tmp;
> Why?

Otherwise I get a warning, "tmp is not used" there.

> Worth it to also check window.Function here?

Sure.

Comment 17

[Johnny Stenback (:jst)]

Comment on attachment 386324 [details] [diff] [review]
With mochitest

r=jst, but leave the first hunk out and fix that in the macros in a new bug.

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/e7cf1bed5a0e

Comment 19

[Samuel Sidler (old account; do not CC)]

Comment on attachment 386324 [details] [diff] [review]
With mochitest

Approved for 1.9.1.1. a=ss for release-drivers.

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

Can we please get this on 1.9.1.1 ASAP?

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/bb79d3a42c01

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed on trunk and 1.9.1 with builds on all platforms like

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090714 Minefield/3.6a1pre ID:20090714050656

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.1pre) Gecko/20090715 Shiretoko/3.5.1pre ID:20090715031842