Last Comment Bug 608543 - Arguments elements should alias formals even for indexes beyond the range of actuals that were passed (in ES5 non-strict)
: Arguments elements should alias formals even for indexes beyond the range of ...
Status: RESOLVED INVALID
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Windows XP
: -- normal (vote)
: ---
Assigned To: general
:
: Jason Orendorff [:jorendorff]
Mentors:
http://gist.github.com/655672
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-10-30 12:59 PDT by Nicholas C. Zakas
Modified: 2010-11-02 12:20 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments

Description Nicholas C. Zakas 2010-10-30 12:59:10 PDT
User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:2.0b8pre) Gecko/20101030 Firefox/4.0b8pre
Build Identifier: 

Section 10.6 Note 1 says:

"For non-strict mode functions the array index (defined in 15.4) named data properties of an arguments object whose numeric name values are less than the number of formal parameters of the corresponding function object initially share their values with the corresponding argument bindings in the function’s execution context. This means that changing the property changes the corresponding value of the argument binding and vice-versa. "

In strict mode, assigning to an arguments property should not cause changes in the named parameter. However, in the latest Minefield (2010-Oct-30), this behavior is also seen in non-strict mode.


Reproducible: Always

Steps to Reproduce:
1. Run attached test script
Actual Results:  
Result of doAdd(10) is NaN because num2 is undefined.

Expected Results:  
Result of doAdd(10) should be 20, because num2 should reflect the value assigned to arguments[1].
Comment 1 Tom Schuster [:evilpie] 2010-10-30 13:11:57 PDT
Could not really explain over twitter, but i think this is actually correct 
behavior.

10.6
>args the actual arguments passed to the [[Call]] internal method
>1. Let len be the number of elements in args.
...
>10. Let indx = len - 1.
>11. Repeat while indx >= 0, 

So all these fancy getter/setter will never be created even in non strict mode.
Comment 2 Brendan Eich [:brendan] 2010-10-30 13:26:48 PDT
10.6 definition: "... args the actual arguments passed to the [[Call]] internal method...."

10.6 step 1: "Let len be the number of elements in args."

10.6 steps 10 and 11 then loop from len-1 down to 0 (inclusive), defining the magic getter and setter. Thus the call doAdd(10) has args [10] and len 1, so the loop iterates over only arg 0, num1 is its formal parameter, and assignment to arguments[1] does not alias formal paremeter num2.

BTW, ES1-3 spec'ed this too, less formally. SpiderMonkey has followed this part of the spec for quite a while.

/be
Comment 3 Nicholas C. Zakas 2010-11-01 14:15:45 PDT
Thanks for the explanation. Guess I was thrown off by the interop issue.
Comment 4 Brendan Eich [:brendan] 2010-11-01 14:55:37 PDT
(In reply to comment #3)
> Thanks for the explanation. Guess I was thrown off by the interop issue.

Did you file a webkit bug? Please link here so everyone can track. Thanks,

/be
Comment 5 Dmitry Soshnikov 2010-11-02 10:39:47 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > Thanks for the explanation. Guess I was thrown off by the interop issue.
> 
> Did you file a webkit bug? Please link here so everyone can track. Thanks,
> 

Already was filed (for V8) http://code.google.com/p/chromium/issues/detail?id=52484

@Nicholas C. Zakas here's a description of the correct semantics implemented in ES itself http://gist.github.com/539974 

Dmitry.
Comment 6 Jeff Walden [:Waldo] (remove +bmo to email) 2010-11-02 10:45:23 PDT
Either I'm skimming too hard and not seeing the earlier reference, or this was likely from IRC discussion -- but does "webkit" in the context of this bug refer to Safari/WebKit/Nitro or to Chrome/V8?  Of course a bug filed against the latter wouldn't be relevant to the former, assuming "webkit" indeed meant Safari/WebKit/Nitro -- just looking for someone to clarify.
Comment 7 Dmitry Soshnikov 2010-11-02 10:55:51 PDT
(In reply to comment #6)
> does "webkit" in the context of this bug
> refer to Safari/WebKit/Nitro or to Chrome/V8?

Safari (WebKit) has a correct behavior. The bug relates only to Chrome (V8).

Notice: the mentioned V8's bugs refers to incorrect `delete` behavior of index-properties of `arguments`, however it covers also the case with incorrect setting of values (I mentioned it before in variable object analysis article), since the whole behavior of setters/getters is broken.

Dmitry.
Comment 8 Brendan Eich [:brendan] 2010-11-02 11:01:37 PDT
Dmitry: is that v8 issue about delete enough? This bug's testcase at the URL does not involve delete.

/be
Comment 9 Dmitry Soshnikov 2010-11-02 11:14:14 PDT
(In reply to comment #8)
> Dmitry: is that v8 issue about delete enough? This bug's testcase at the URL
> does not involve delete.
> 

Yeah, as I mentioned above -- in its exact case, the V8's bug is about incorrect `delete` behavior on `arguments`, but since it involves common setters/getters issue, it covers assignment (with sharing) too. One can open a separate issue, but when the first bug will be fixed, the later won't be reproducible. However, for the tracking it's possibly good to open the separate one.

Dmitry.
Comment 10 Brendan Eich [:brendan] 2010-11-02 11:41:54 PDT
I think v8 needs another "issue". The effect and cause are different AFAICT. While a fix for both effects might appear as a patch for the existing issue, it is better protocol to file separate bugs to track distinct symptoms that may have distinct fixes.

/be
Comment 11 Dmitry Soshnikov 2010-11-02 12:20:32 PDT
Yes, I think so. Though, it's already also on file, found it (http://code.google.com/p/chromium/issues/detail?id=36350); and V8's team refers it as a kind of duplicate of the issue with `delete` (mentioned above bug refers it too: http://code.google.com/p/chromium/issues/detail?id=32184).

Dmitry.

Note You need to log in before you can comment on or make changes to this bug.