644015 - In "strict mode" properties of an Arguments object can not be directly modified.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, major)

Description

[Perelandric]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_6_6; en-US) AppleWebKit/534.16 (KHTML, like Gecko) Chrome/10.0.648.127 Safari/534.16
Build Identifier: "Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0) Gecko/20100101 Firefox/4.0"

The values referenced by the FormalParameterList are intended to be unaffected by manipulation of the values referenced by properties of the Arguments object and vice versa when in "strict mode".

The issue is that changing the value referenced by a property of an Arguments object does not have any observable affect on that property  unless you also update a Formal Parameter. It is not required that it be the corresponding Formal Parameter.




Reproducible: Always

Steps to Reproduce:
1. Create a function that has at least 1 parameter defined, and that changes arguments[0] (for example) to a new value and then logs the value of arguments[0] for observation.

2. Invoke the function, passing an argument to the one that is being affected in the function.

3. Observe that the value of arguments[0] has not been updated.

"use strict";

(function(x, y) {
    arguments[0] = 3;
    console.log( arguments[0] );  // still 1
}( 1 ));
Actual Results:  
arguments[0] was not modified in spite of having been updated directly.

Expected Results:  
arguments[0] ought to have been update to its new value.

Making a change to a FormalParameter (even an unrelated one) allows arguments[0] to be correctly modified.

"use strict";

(function(x, y) {
    arguments[0] = 3;
    y = 'pizza';
    console.log( arguments[0] );  // now 3
}( 1 ));

Comment 1

[Jeff Walden [:Waldo]]

js> "use strict"; (function(x, y) {
arguments[0] = 3;
print( x )
print( arguments[Math.random() ? "0" : 0] ); // still 1
}( 1 ));
1
3
js> "use strict"; (function(x, y) {
arguments[0] = 3;
print( x )
print( arguments[0] ); // still 1
}( 1 ));
1
1

Looks like a bug in js_GetArgsProperty, called by JSOP_ARGSUB.  And I'm sad I somehow missed this when writing ecma_5/Function/strict-arguments.js.  :-(

Comment 2

[Jeff Walden [:Waldo]]

js_GetArgsProperty contains this code:

    if (JSID_IS_INT(id)) {
        uint32 arg = uint32(JSID_TO_INT(id));
        JSObject *argsobj = fp->maybeArgsObj();
        if (arg < fp->numActualArgs()) {
            if (argsobj) {
                if (argsobj->getArgsElement(arg).isMagic(JS_ARGS_HOLE))
                    return argsobj->getProperty(cx, id, vp);
            }
            *vp = fp->canonicalActualArg(arg);

StrictArgSetter contains this code:


    if (JSID_IS_INT(id)) {
        uintN arg = uintN(JSID_TO_INT(id));
        if (arg < obj->getArgsInitialLength()) {
            obj->setArgsElement(arg, *vp);
            return true;
        }
    } else {

So this setter just stores a different value in the existing slot so subsequent lookups will get it.  But arguments objects normally rely on setting an argument deleting the existing property, storing JS_ARGS_HOLE in the relevant slot, and adding an entirely new property -- what the isMagic check in the first snippet's supposed to hit.  So these two bits are out of whack.

I've been up long enough at this point that I should leave this til morning, when I'll be less tired and my head will be clearer.  Patch tomorrow.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Patch, add tests

It seems the choice is either to change js_GetArgsProperty or to change StrictArgSetter.  Strict arguments should be less bizarre, so having them keep using their original argument slots when set seems better, all things considered, to me.  But you could make a change either place to make this work correctly, I think.

Comment 4

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/5e5e7e9d1d8a

I think this is 4.0.1 fodder, will wait for a little baking first though just to be safe.

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 521350 [details] [diff] [review]
Patch, add tests

Strict mode is yet young on the web, letting easily-tripped strict mode violations like this to fester may lead to calcification and entrenchment on the web, and people are stumbling on it.  We should fix this in a point release.

Comment 6

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/5e5e7e9d1d8a

Comment 7

[Daniel Veditz [:dveditz]]

jwalden: are you saying it we must block 4.0.1 on this fix? if so please nominate.

Comment 8

[Jeff Walden [:Waldo]]

"must" is perhaps slightly too strong.  But only slightly.  I don't think we should delay in fixing it.

Comment 9

[Daniel Veditz [:dveditz]]

I'm happy to approve the fix  but not going to hold up the release for it.

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 521350 [details] [diff] [review]
Patch, add tests

Approved for the mozilla2.0 repository, a=dveditz for release-drivers

Please land this for the Tumucumaque Macaw release.

Comment 11

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/releases/mozilla-2.0/rev/abab3297c82f