arguments[i] should alias parameters even after return

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
8 years ago
5 years ago

People

(Reporter: vacaboja, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

8 years ago
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.
(Reporter)

Comment 1

8 years ago
Created attachment 361952 [details]
test case

This is the script given in the bug description.
Assignee: nobody → general
Component: General → JavaScript Engine
Product: Firefox → Core
QA Contact: general → general
Duplicate of this bug: 511998
Duplicate of this bug: 544166
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Parameters of a javascript function may not be aliased by the arguments object. → arguments[i] should alias parameters even after return
OS: Mac OS X → All
Hardware: x86 → All
Duplicate of this bug: 577572
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.  :-)
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
Duplicate of this bug: 679719

Updated

6 years ago
Assignee: general → pbiggar

Comment 8

6 years ago
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

6 years ago
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

6 years ago
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

6 years ago
(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

6 years ago
(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!).

Updated

6 years ago
Depends on: 684336

Comment 13

6 years ago
Created attachment 558024 [details] [diff] [review]
WIP
Attachment #553959 - Attachment is obsolete: true

Comment 14

6 years ago
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.
Attachment #558024 - Attachment is obsolete: true
Paul are you still working on this?

Comment 16

6 years ago
No, feel free to steal.
Assignee: paul.biggar → general

Comment 17

5 years ago
Fixed by bug 659577.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Depends on: 659577
Resolution: --- → WORKSFORME
Known patch that fixes this -> FIXED.
Resolution: WORKSFORME → FIXED
Attachment #558974 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.