Last Comment Bug 478174 - arguments[i] should alias parameters even after return
: arguments[i] should alias parameters even after return
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: general
:
:
Mentors:
: 511998 544166 577572 (view as bug list)
Depends on: 659577 684336
Blocks:
  Show dependency treegraph
 
Reported: 2009-02-11 22:46 PST by vacaboja
Modified: 2012-06-04 15:20 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
test case (295 bytes, text/html)
2009-02-11 22:50 PST, vacaboja
no flags Details
rough WIP (3.23 KB, patch)
2011-08-17 17:09 PDT, Paul Biggar
no flags Details | Diff | Splinter Review
WIP (74.16 KB, patch)
2011-09-02 20:05 PDT, Paul Biggar
no flags Details | Diff | Splinter Review
wip3 (20.54 KB, patch)
2011-09-07 15:02 PDT, Paul Biggar
no flags Details | Diff | Splinter Review

Description vacaboja 2009-02-11 22:46:49 PST
User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.2a1pre) Gecko/20090211 Minefield/3.2a1pre
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.2a1pre) Gecko/20090211 Minefield/3.2a1pre

According to ECMA-262 paragraph 10.1.8 any change to a formal parameter of a function must reflect into a change of the corresponding property of the arguments object and vice versa. However, in the following particular case, this fails to happen. Consider the script:

function f(y)
{
	var args = arguments;

	return function()
	{
		y = "no bug";
		alert(args[0]);
	}
}

f("bug!")();

It should pop the alert "no bug", but "bug!" pops instead. The same script works as expected on other major browsers (Safari and Chrome).

Reproducible: Always

Steps to Reproduce:
Run the script given in the Details section.
Actual Results:  
The alert "bug!" pops.

Expected Results:  
The alert "no bug" should pop.
Comment 1 vacaboja 2009-02-11 22:50:58 PST
Created attachment 361952 [details]
test case

This is the script given in the bug description.
Comment 2 Jason Orendorff [:jorendorff] 2010-06-30 11:38:23 PDT
*** Bug 511998 has been marked as a duplicate of this bug. ***
Comment 3 Jason Orendorff [:jorendorff] 2010-06-30 11:39:00 PDT
*** Bug 544166 has been marked as a duplicate of this bug. ***
Comment 4 Jeff Walden [:Waldo] (remove +bmo to email) 2010-12-06 08:13:36 PST
*** Bug 577572 has been marked as a duplicate of this bug. ***
Comment 5 Jeff Walden [:Waldo] (remove +bmo to email) 2011-01-24 14:19:48 PST
Arguments objects carry around stack frame pointers right now.  What we really need is for them to carry around Call object pointers instead -- but not every function with an arguments object also has a Call object.  The fix here isn't obvious.

For now I'll hold out hope that this will be simpler when our scope chain construction is rewritten.  :-)
Comment 6 Brendan Eich [:brendan] 2011-01-25 14:07:00 PST
It doesn't matter whether we replace Call objects with some internal environment record equivalent, the issue remains. If an arguments object survives its function's activation and so does the activation's environment record via upvar refs to formal parameters, then you have to sync both ways.

That means keeping the Call object entrained from the arguments object in the current code. If there's no Call object, then there are no upvars (flat closures copy upvars whose sole and guaranteed initialiser they post-dominate; for formal parameters this means there can be no assignment, e.g., y = "no bug").

/be
Comment 7 AWAY Tom Schuster [:evilpie] 2011-08-17 09:17:46 PDT
*** Bug 679719 has been marked as a duplicate of this bug. ***
Comment 8 Paul Biggar 2011-08-17 16:42:54 PDT
Since both CallObjs and ArgumentsObjects store the arguments, the syncing brendan mentions is necessary. Unless we made Arguments read/write indirectly via CallObj instead of to the stack, or vice-versa.

I don't think the lifetimes of either CallObj/ArgumentsObject is a subset of the lifetime of the other, so I guess a third object is required, which either one can write to/read from, and which syncs to the stackframe if necessary.
Comment 9 Luke Wagner [:luke] 2011-08-17 17:09:10 PDT
Would it work for ArgumentsObject to point to the callobj (via a reserved slot) so that ArgGetter/ArgSetter can refer use the call object's slots for formal parameters?  Thus, there would be a single "canonical" location for formals which is the: (1) the stack frame, if its active, else (2) the callobj, if there is one, else (3) the argsobj.

Another thing to watch out for is places where we poke at the argobj's slots directly, but I think Waldo factored a lot of this behind ArgumentsObject::getElement(s).
Comment 10 Paul Biggar 2011-08-17 17:09:12 PDT
Created attachment 553959 [details] [diff] [review]
rough WIP

Rough cut, works on this test case, fails on some of the dup testcases, probably due to the (expected) lifetime issues.
Comment 11 Paul Biggar 2011-08-17 17:23:46 PDT
(In reply to Luke Wagner [:luke] from comment #9)
> Would it work for ArgumentsObject to point to the callobj (via a reserved
> slot) so that ArgGetter/ArgSetter can refer use the call object's slots for
> formal parameters?  Thus, there would be a single "canonical" location for
> formals which is the: (1) the stack frame, if its active, else (2) the
> callobj, if there is one, else (3) the argsobj.

Ah, the difference being that the ArgumentsObject will keep the CallObj alive. Yeah, that should work.

Can we dynamically create CallObjs after the ArgumentsObject has been created (I think not, amn't sure)?
Comment 12 Luke Wagner [:luke] 2011-08-17 17:37:49 PDT
(In reply to Paul Biggar from comment #11)
> Can we dynamically create CallObjs after the ArgumentsObject has been
> created (I think not, amn't sure)?

Yeah:

> function f() { a = arguments }
> dis(f)
flags: NULL_CLOSURE

Btw, the official IonMonkey plan is to create activation objects eagerly or not at all: bug 659577 (which, incidentally mentions this very bug!).
Comment 13 Paul Biggar 2011-09-02 20:05:40 PDT
Created attachment 558024 [details] [diff] [review]
WIP
Comment 14 Paul Biggar 2011-09-07 15:02:58 PDT
Created attachment 558974 [details] [diff] [review]
wip3

This makes CallObject use an argumentsData to store the slots. Now that both it and ArgumentsObject use an argumentsData object, we can make them share the one object.
Comment 15 AWAY Tom Schuster [:evilpie] 2011-10-30 07:37:39 PDT
Paul are you still working on this?
Comment 16 Paul Biggar 2011-10-30 16:01:55 PDT
No, feel free to steal.
Comment 17 Luke Wagner [:luke] 2012-06-04 15:18:38 PDT
Fixed by bug 659577.
Comment 18 Gary Kwong [:gkw] [:nth10sd] 2012-06-04 15:20:18 PDT
Known patch that fixes this -> FIXED.

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