Closed Bug 644015 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: JavaScript Engine, defect, major)

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla2.0
Tracking Status
status2.0 --- .1-fixed

People

(Reporter: algorithmicimperative, Assigned: Waldo)

References

()

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file)

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 ));
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.  :-(
Assignee: general → jwalden+bmo
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
OS: Mac OS X → All
Hardware: x86_64 → All
Blocks: 516255
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.
Attached patch Patch, add testsSplinter Review
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.
Attachment #521350 - Flags: review?(dmandelin)
Attachment #521350 - Flags: review?(dmandelin) → review+
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.
Whiteboard: fixed-in-tracemonkey
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.
Attachment #521350 - Flags: approval2.0?
http://hg.mozilla.org/mozilla-central/rev/5e5e7e9d1d8a
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
jwalden: are you saying it we must block 4.0.1 on this fix? if so please nominate.
"must" is perhaps slightly too strong.  But only slightly.  I don't think we should delay in fixing it.
blocking2.0: --- → ?
I'm happy to approve the fix  but not going to hold up the release for it.
blocking2.0: ? → ---
status2.0: --- → wanted
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.
Attachment #521350 - Flags: approval2.0? → approval2.0+
You need to log in before you can comment on or make changes to this bug.