Closed
Bug 644015
Opened 14 years ago
Closed 14 years ago
In "strict mode" properties of an Arguments object can not be directly modified.
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
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)
2.96 KB,
patch
|
dmandelin
:
review+
dveditz
:
approval2.0+
|
Details | Diff | Splinter Review |
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 ));
Assignee | ||
Comment 1•14 years ago
|
||
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
Assignee | ||
Comment 2•14 years ago
|
||
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.
Assignee | ||
Comment 3•14 years ago
|
||
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)
Updated•14 years ago
|
Attachment #521350 -
Flags: review?(dmandelin) → review+
Assignee | ||
Comment 4•14 years ago
|
||
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
Assignee | ||
Comment 5•14 years ago
|
||
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?
Comment 6•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment 7•14 years ago
|
||
jwalden: are you saying it we must block 4.0.1 on this fix? if so please nominate.
Assignee | ||
Comment 8•14 years ago
|
||
"must" is perhaps slightly too strong. But only slightly. I don't think we should delay in fixing it.
blocking2.0: --- → ?
Comment 9•14 years ago
|
||
I'm happy to approve the fix but not going to hold up the release for it.
Comment 10•14 years ago
|
||
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+
Assignee | ||
Comment 11•14 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•