Closed Bug 577325 Opened 14 years ago Closed 13 years ago

function definition (statement) doesn't call preexisting setter on window, calling it does not invoke the getter

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Tracking Flags:
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: al_9x, Assigned: Waldo)

References

Details

(Whiteboard: [fixed-in-tracemonkey][softblocker] [fx4-fixed-bugday])

Attachments

(2 files, 1 obsolete file)

testcase from al_9x@yahoo.com (inline in comment 0)
387 bytes, text/html
Details
Patch with tests
36.72 KB, patch
igor
: review+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.6) Gecko/20100625 Firefox/3.6.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.6) Gecko/20100625 Firefox/3.6.6

From http://groups.google.com/group/mozilla.dev.tech.js-engine/browse_thread/thread/c3ba6741ea185391

Should one expect a function statement to trigger a call to a preexisting 
setter?  and subsequent calls to the function to invoke the preexisting 
getter?  That is what happens in Chrome and Opera, but not in Fx 3.6.6 & 4.0b1

In the following sample, "function foo()" statement does not call the foo 
setter and what's even stranger, after it, "window.foo()" invokes the getter 
but just "foo()" does not and instead calls the statement defined foo 
function.

If the function statement is replaced by a function expression "var foo = 
function() ..." then it bahves as expected, same as Chrome and Opera.

<html>
<head>
<script>
__defineGetter__('foo', function() {alert('foo getter'); return function() 
{alert('new foo');};});
__defineSetter__('foo', function() {alert('foo setter');});
</script>
<script>
function foo()
{
   alert('statement foo');
}
</script>
</head>
<body>
<button onclick="foo();">foo()</button><br>
<button onclick="window.foo();">window.foo()</button>
</body>
</html>

Here's what the standard (10.1.3) has to say about this:

"For each FunctionDeclaration in the code, in source text order, create a 
property of the variable object whose name is the Identifier in the 
FunctionDeclaration, whose value is the result returned by creating a 
Function object as described in section 13, and whose attributes are 
determined by the type of code. If the variable object already has a 
property with this name, replace its value and attributes. Semantically, 
this step must follow the creation of FormalParameterList properties."

So the value of the existing property is to be replaced.  That appears to be 
consistent with calling the setter with the new value, as Chrome and Opera 
are doing. 


Reproducible: Always
This bug cites ECMA-262 Edition 3, but that spec did not include getters and setters. What's more, its language is clear in my view: replace means the old property is destroyed, not set (if an accessor, i.e., getter/setter pair).

But ECMA-262 Edition 5 both standardizes getters and setters and clarifies this bug's issue, albeit at the price of greater spec complexity:

10.5 Declaration Binding Instantiation
 . . .
5. For each FunctionDeclaration f in code, in source text order do
a. Let fn be the Identifier in FunctionDeclaration f.
b. Let fo be the result of instantiating FunctionDeclaration f as described in Clause 13.
c. Let funcAlreadyDeclared be the result of calling env’s HasBinding concrete method passing fn as the argument.
d. If funcAlreadyDeclared is false, call env’s CreateMutableBinding concrete method passing fn and configurableBindings as the arguments.
e. Call env’s SetMutableBinding concrete method passing fn, fo, and strict as the arguments.

10.2.3 The Global Environment

The global environment is a unique Lexical Environment which is created before any ECMAScript code is executed. The global environment’s Environment Record is an object environment record whose binding object is the global object (15.1). The global environment’s outer environment reference is null.

10.2.1.2.3 SetMutableBinding (N,V,S)

The concrete Environment Record method SetMutableBinding for object environment records attempts to set the value of the environment record’s associated binding object’s property whose name is the value of the argument N to the value of argument V. A property named N should already exist but if it does not or is not currently writable, error handling is determined by the value of the Boolean argument S.

1. Let envRec be the object environment record for which the method was invoked.
2. Let bindings be the binding object for envRec.
3. Call the [[Put]] internal method of bindings with arguments N, V, and S.

and finally

8.12.5 [[Put]] ( P, V, Throw )
When the [[Put]] internal method of O is called with property P, value V, and Boolean flag Throw, the following steps are taken:
1. If the result of calling the [[CanPut]] internal method of O with argument P is false, then
a. If Throw is true, then throw a TypeError exception.
b. Else return.
2. Let ownDesc be the result of calling the [[GetOwnProperty]] internal method of O with argument P.
3. If IsDataDescriptor(ownDesc) is true, then
 . . .
4. Let desc be the result of calling the [[GetProperty]] internal method of O with argument P. This may be either an own or inherited accessor property descriptor or an inherited data property descriptor.
5. If IsAccessorDescriptor(desc) is true, then
a. Let setter be desc.[[Set]] which cannot be undefined.
b. Call the [[Call]] internal method of setter providing O as the this value and providing V as the sole argument.

The Clause 15.1 "The Global Object" does not address whether a custom [[Put]] is allowed by a global object, but 8.6.2 constrains even host objects to have some consistency between [[Put]] and [[CanPut]]. Anyway, it seems global objects in other browsers do not customize [[Put]] so as to replace the accessor property.

So this bug is valid per ES5. Thanks for filing it!

/be
Blocks: es5
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Hardware: x86 → All
It's a little nasty for integrity (such as it is in JS) that a function declaration no longer blows away any prior configurable property of the same name. Cc'ing Allen and Mark in case it was unintended.

/be
(In reply to comment #1)
> This bug cites ECMA-262 Edition 3, but that spec did not include getters and
> setters. What's more, its language is clear in my view: replace means the old
> property is destroyed, not set (if an accessor, i.e., getter/setter pair).

It does say "replace its value", which suggests set, but I wont argue this further.

If you interpret it as destroying the property, that is still not what seems to happening.  Referring to my test page, after foo definition, the (g|s)setter is still there and calling window.foo() invokes the getter, while just foo() calls the statement defined foo.  How can both exist at the same time? How can window.foo and foo refer to different things?
al_9x: I agree that an extension of ES3 with getters and setters added to it could be read the way you read it, and ES5 specifies things this way.

But ES3 as written leaves no trace of the pre-existing property when processing a FunctionDeclaration of the same name. Notice that the DontDelete attribute of the prior property is not consulted. All attributes and the value are replaced.

This makes the ES5 change substantially different from ES3 even ignoring getters and setters, since it conserves the new equivalent (but with logically inverted sense) attribute [[Configurable]].

Allen, what say you?

In a recent http://hg.mozilla.org/tracemonkey js shell:

js> __defineGetter__('foo', function() {alert('foo getter'); return function() 
{alert('new foo');};});
js> __defineSetter__('foo', function() {alert('foo setter');});
js> alert=print
function print() {[native code]}
js> function foo()
{
   alert('statement foo');
}
js> foo
function foo() {alert("statement foo");}
js> foo()
statement foo
js> foo=42
42
js> foo
42

So the odd preservation of the getter that you note in comment 3 is a separate bug, to do with the Gecko DOM and its outer and inner windows (what the HTML5 spec calls WindowProxy and Window).

mrbkap, is this known and on file?

/be
I left out some js shell lines that show no split-global object bug in that simple embedding:

js> foo
function foo() {print("statement foo");}
js> this.foo
function foo() {print("statement foo");}
js> this.foo=42
42
js> foo
42

Changing the HTML testcase to use |this.| instead of |window.| has no effect, as expected. That bug is a DOM bug.

/be
(In reply to comment #2)
> It's a little nasty for integrity (such as it is in JS) that a function
> declaration no longer blows away any prior configurable property of the same
> name.

Perhaps it's worth mentioning what this makes possible. The NoScript extension has functionality (page level surrogates) to run some javascript before parsing begins and by the use of (g|s)etters(if it worked as ES5) preemptively replace individual content functions.  Doing this replacement on DOMContentLoaded is too late as the original may have been called by then.
I'm going to try to move most of discussion of this to es5-discuss because I think there are issues of broader interest involved. 

I agree with Brendan's analysis both in that the ES5 spec. says that a pre-existing setter is called and also that the ES5 spec. has a significant change WRT the treatment of attributes of existing global properties for global function declaration instantiation.

However, I don't think that the attribute spec. change always reduces integrity.  Consider that ES5 says that undefined is a global property with attributes {writable: false, enumberable: false, configurable: false}.  The ES3 language for instantiating: function undefined() {}; 
would have continued to allow the built-in undefined to be over-written by the function declaration.
How will the changes being proposed/discussed on es5-discuss impact this bug?  Will it be possible to create an accessor property that isn't destroyed by a subsequent function declaration?
Yes. Please stay tuned to the thread starting at https://mail.mozilla.org/pipermail/es5-discuss/2010-July/003606.html
(In reply to comment #9)
> How will the changes being proposed/discussed on es5-discuss impact this bug? 
> Will it be possible to create an accessor property that isn't destroyed by a
> subsequent function declaration?

Yes, a non-configurable pre-existing property won't be replaced by processing a FunctionDeclaration of the same name.

A configurable property will be replaced, though.

/be
blocking2.0: --- → beta6+
Assignee: general → jwalden+bmo
The resolution of the aforementioned thread is in the most recent ES5 errata: a significantly different (and better, I think) algorithm for processing function statements.

http://wiki.ecmascript.org/lib/exe/fetch.php?id=start&cache=cache&media=resources:es5_errata_7-19-10.pdf

So don't look to the published spec for what should happen here, because it's now quite obsolete.

Hoped to get to this today, but reading through the thread and making sure I understood it all took some time -- will probably get to it either over the weekend or on Monday.
Status: NEW → ASSIGNED
blocking2.0: beta6+ → betaN+
Could be.  I may have just missed the es-5?discuss email about the updated errata, or perhaps one wasn't sent.
Blocks: 609832
I think this implements the ES5 requirements.  It also passes shell tests.

Browser JS tests currently fail, because believe it or not, we implement non-deletability of window.document *only* through the delete hook, and not through the property in question not being [[Configurable]].  So function document(){} blows away window.document, and hilarity ensues in that test.  (WebIDL actually says all properties are [[Configurable]] right now, so technically this is "not" a bug, but that's clearly going to need to change at least for some properties -- document and location probably, maybe others.)  There may well be other failures -- if you take a look at the patch, I adjusted a couple tests that stumble because of a bizarre "ToString" object we seem to implement (and probably shouldn't, but that's a separate bug), and there may be more, but tests didn't run far enough to say that's the only remaining failure.

Given the track record of just the JS tests, I won't be surprised if Mochitests and others also trigger failures, but I haven't made any effort to run them to see.

But this is a decent start for now -- just wanted to get it out there to return to later, now back to other bugs after a diversion here.
No longer blocks: 609832
Whiteboard: softblocker
Actually, I think the window.document thing is wrong.  I can overwrite window.document with Object.defineProperty perfectly fine, so I believe the test is just wrong.

I pushed an up-to-date version of the patch to try, will see what the results are and then move onward with fixing this depending on what (if anything) is broken.
Attached patch Patch with tests — — Splinter Review 
The ToString->ConvertToString change is necessary because we apparently have a nsIDOMToString class defined in the browser, and that causes a global ToString property to be defined -- and that property is non-configurable *and* non-enumerable, which triggers the throw case in the spec algorithm.  Renaming the function avoids this problem.  (Better would be to not have the ToString global property at all, but that's a bridge which I understand can't be crossed at this late date in the release cycle.  Next time, maybe!)
Attachment #489005 - Attachment is obsolete: true
Attachment #505967 - Flags: review?(igor)
Doubtless the reader will wonder why regress-406572.js is skipped rather than fixed (the test itself is wrong, as window.document is configurable and therefore should be overwritable by a function statement).  The answer is that, somewhat obliquely, it points out a security vulnerability.  We CheckRedeclaration for function statements in global code, but if there's no getter/setter pair, and if the existing property is writable, we always define for a function statement.

What this lapse enables is a way to overwrite window.location.  This is a hazard because plugins use window.location to determine the page that embedded them.  (No, there's no NPAPI function for this.  Yes, this is inexcusable.  But this is the world that exists now.)  See bug 622199 for another bug where window.location's inviolability is relied upon by plugins, which results in security hazards.  When I discussed this briefly with bz a bit ago he seemed to think there was one, or at least that it was a known problem, but I couldn't find a bug.

What I'm trying to avoid is explicitly fixing the test and thereby potentially pointing out to someone reading the code that this could have been used to overwrite window.location.  Perhaps the insight is too attenuated for the average black-hat to see it here, or it requires too much web security insight to notice.  But I'd rather be excessively paranoid and wrong than right and scrambling to fix a zero-day.

On a somewhat unrelated note, this still suffers from the same problems noticed in bug 624364.  It seems marginally better to me to fix that bug all at once than to fix it piecemeal for function statements, then later for vars.  I could switch this to nativeLookup and all if really desired to do it fully fully right, but since I have the patch already written, and that bug will need work regardless what happens here, I'm ignoring it in this patch.
I filed bug 627891 to specifically address the concern noted in comment 18.
Note that for a replaceable property X I would expect |function X| to NOT call the setter.  That's the whole point of being replaceable, and websites depend on that behavior.

Now I bet we don't have tests for that in our test suite, but I expect that any patch for this bug will add such tests.
Comment on attachment 505967 [details] [diff] [review]
Patch with tests

>diff --git a/js/src/jsinterp.cpp b/js/src/jsinterp.cpp
>@@ -5436,62 +5436,44 @@ BEGIN_CASE(JSOP_DEFFUN)
>+    if (!parent->lookupProperty(cx, id, &pobj, &prop))
>         goto error;
>+    Shape *shape = reinterpret_cast<Shape *>(prop);

We should stop pre-existing practice of converting JSProperty* into a shape before we know that the object is native. So move the cast after the pobj != parent "if" and add isNative assert before that.

>+    if (!prop || pobj != parent) {
>+        if (!parent->defineProperty(cx, id, rval, PropertyStub, PropertyStub, attrs))
>+            goto error;
>+        goto done;

Nit: replace goto done with do-while(false) loop and break.

>+    if (parent->isGlobal()) {
>+        if (shape->configurable()) {
...
>+            goto done;

Nit: same here.

>+
>+    /* Step 5f. */
>+    if (!parent->setProperty(cx, id, &rval, script->strictModeCode))
>         goto error;

Add comments about const declarations in the Call object and how this would produce warnings/errors

>@@ -760,61 +760,40 @@ stubs::DefFun(VMFrame &f, JSFunction *fu
>+    if (!parent->lookupProperty(cx, id, &pobj, &prop))
>         THROW();
>+    Shape *shape = reinterpret_cast<Shape *>(prop);

Again, move cast after we know the prop is a shape. 

>+    /* Step 5f. */
>+    if (!parent->setProperty(cx, id, &rval, strict))
>         THROW();

Add the same comments as above.

>diff --git a/js/src/shell/js.cpp b/js/src/shell/js.cpp
> static JSBool
>+Evaluate(JSContext *cx, uintN argc, jsval *vp)

r+++ for adding this. Finally the shell gains an easy method to test the effect of multiple <script> tags and not to rely on evalcx(, this) hack.

>+
>+    JSObject *thisobj = JS_THIS_OBJECT(cx, vp);
>+    if (!thisobj)
>+        return false;

Throw here if thisobj is not a global here. 

r+ with this fixed.
Attachment #505967 - Flags: review?(igor) → review+
(In reply to comment #17)
> Created attachment 505967 [details] [diff] [review]
> Patch with tests

The tests do not have coverage for function definitions under the with statements. It would be nice to have that for completeness.
Cc'ing Allen re: comment 20. Boris, is replaceable specified by a w3c or whatwg spec, or will it be?

/be
By replaceable I believe bz meant configurable (deletable and re-definable).  The semantics and patch do not call setters in any circumstance, and they do allow properties not directly on the global object to be shadowed by global variables.  That's basically the names listed here (although a few get resolved onto the global object as well):

javascript:Object.getOwnPropertyNames(Window.prototype).join(",\x20")

Right now WebIDL doesn't specify anything about configurability, although there's an open bug to add such to WebIDL:

http://www.w3.org/Bugs/Public/show_bug.cgi?id=11267
> Boris, is replaceable specified by a w3c or whatwg spec

Yes, though the exact syntax may change.

> By replaceable I believe bz meant configurable (deletable and re-definable). 

I mean properties that exist on the global object (some as own properties) for which an unqualified set will, instead of calling the setter, delete and redefine the property, and for which |var propname| or |function propname| will also delete and redefine the property.  At least last I checked that's how it worked.
http://hg.mozilla.org/tracemonkey/rev/e4d449cdd3be

I'll try to get some tests for with wrapped up and pushed in the next day or two.
Whiteboard: softblocker → [fixed-in-tracemonkey][softblocker]
Full disclosure: I was around and involved when, during Netscape 4 development, we pioneered a "replaceable" notion in the DOM. It allowed us to add new window properties without preempting the names from being used by content. It used resolve magic.

If a w3c spec for replaceable exists, can someone cite it? It seems like it may go beyond [[Configurable]] and other ECMA-262 Edition 5 attributes.

/be
http://www.w3.org/TR/2010/WD-WebIDL-20101021/#Replaceable describes what happens for replaceable readonly attributes.

We also have replaceable writable properties; I don't know whether other UAs don't or what.  The things that are writable replaceable for us are "innerWidth/Height" (not marked replaceable in CSSOM draft), "opener" (which html5 defines readonly; it's not clear to me that this is desirable), "outerWidth/Height" (not replaceable in CSSOM draft), "screenX/Y" (marked readonly in CSSOM draft, and NOT marked replaceable), "status" (not sure it's defined in any spec) and "name" (not marked replaceable in HTML5, doesn't seem to be replaceable in Safari/Chrome, is replaceable in Opera/Gecko).
Ccing Anne for the CSSOM bits there.
I filed http://www.w3.org/Bugs/Public/show_bug.cgi?id=11838 on the "name" thing, since it's replaceable in IE as well.
So, most if not all of those could be getter/setter pairs on the prototype, or even data properties on the prototype (which doesn't serve a ton of purpose since every window has its own Window.prototype, but it does exist and can be used this way -- is, even, for many things).  That would address the replaceable concern.
Readonly replaceable can be done with a getter/setter pair on the prototype, where the setter defines the property on the object itself or some such shenanigans, perhaps.

Writable replaceable can't be done that way, of course.
Yes, Web IDL defines that replaceable read only attributes correspond to accessor properties on the prototype with a setter that defines an own property on the object itself:

http://dev.w3.org/2006/webapi/WebIDL/#es-attributes

As Boris says, it's not possible to have writable replaceable properties without going outside the bounds of ES5's native semantics.
What writable properties allegedly need to be replaceable? It's not 1997 anymore.

/be
(In reply to comment #20)
> Note that for a replaceable property X I would expect |function X| to NOT call
> the setter.  That's the whole point of being replaceable, and websites depend
> on that behavior.
> 
 Be sure to see the most recent update to the declaration binding instantiations at https://mail.mozilla.org/pipermail/es5-discuss/2011-January/003882.html 

In accordance to this and the previous ES5.1 change function declarations are never supposed to call a setter. That was the original motivator for the changes from the original ES5 algorithm.

The other thing the change is trying to do is preserve the ES3 attribute semantics for function declarations in the face of possible preexistent user defined non-configurable attributes.  This is the non-enumerable+non-configurable case mentioned in comment 17.  If an ES5 global function declaration cannot establish the attribute states that it would have established in ES3, it throws.

An accessor property seems to be the only plausible implementation of the WebIDL [Replaceable] extended attribute. It the accessor could either be on a proto on an own property as long as its backing store is separate from the actual property.

Regarding comment 35, my reading of the WebIDL spec. is that [Replaceable] must only be used in conjunction with an read only attribute as that is the only combination whose meaning is defined by the specification. No semantics at all is given for [Replaceable] applied to an attribute that is not readonly.
Allen, see comment 28.  The WebIDL spec may need to change here, and define how [Replaceable] works for writable properties (which is by differentiating between qualified and unqualified sets; whether this is needed is still being sorted out).
Based on other browsers' behavior noted in comment 28, can we try to drop support for this writable-replaceable combo?

/be
I have no idea; I didn't investigate browser behavior for most of those properties....  Someone would have to write up some testcases (and note that my "name" testcase was somewhat flawed; it's not replaceable in IE, just writable and on the prototype so that |var name| creates a new property on the object, unlike in Gecko).
In any event, I don't think we should at this point start experimenting with making properties non-replaceable.

Definitely agree that we should look into it for next release though. So if we're lucky we might not need to make any changes to any specs.
(In reply to comment #41)
I agree, this is not the time for experiments...

But it does sound like we should be working with Cameron do try to get these things pinned down for the long run.

I guess we could use an access property to implement writable-replacable but it would be nice to not have too.
Forgive me for not having read the email in comment 37, but if it is indeed possible to have writable-replaceable properties without needing behaviour outside of native ES5 semantics, then it worries me a little less about adding it to Web IDL.

Regardless, I think it needs to be determined whether the properties mentioned in comment 28 really do need to be replaceable for interop before the relevant specs and Web IDL change.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
This seems to have caused bug 628612 (broke facebook messages and chat).
Depends on: 628612
I pushed the with-tests desired in comment 22 to TM:

http://hg.mozilla.org/tracemonkey/rev/c321d4757563
Depends on: 628937
v. Mozilla/5.0 (X11; Linux x86_64; rv:2.0b12pre) Gecko/20110204 Firefox/4.0b12pre
Status: RESOLVED → VERIFIED
Whiteboard: [fixed-in-tracemonkey][softblocker] → [fixed-in-tracemonkey][softblocker] [fx4-fixed-bugday]
See comment 18 for the reason this test was flat-out skipped with no in-test-list comment why, originally.  The test and its semantics are messy enough that I figure it's best to get a review on reenabling it.  (It passed when I ran it locally.)  By way of background, window.document is non-writable but configurable, so the test does successfully blow away the binding (which it didn't, before previous patches in this bug) and replace it with a new (writable, configurable) one.

Bug 622199 was fixed semi-recently enough, and depends on users getting updated Flash and Firefox installed and all that (and 3.6 is still theoretically zombifying along, as are Firefox <10 rapid-releasers), that we probably can't unhide this comment and comment 18 just yet, alas.  :-\  And if the commit message is to be informative, that means the patch can't land yet either.  But I'm fine carrying along a reviewed patch in my queue for awhile longer, it's easy enough to maintain through rebases.
Comment 18 is private: false
Comment 19 is private: false
Comment 48 is private: false
I pushed the patch from comment 48 after a try run to make sure a nine-month-old patch didn't break.  :-)

https://tbpl.mozilla.org/?tree=Try&rev=8b430bc0d27e
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e31a0fc42be
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: