530650 - Local functions named 'arguments' should override the arguments object

Status: RESOLVED DUPLICATE of bug 740446

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

At the moment, SpiderMonkey allows the arguments object to override local functions named 'arguments'.

js> function f() { function arguments() { return 42; }; return arguments; }
js> f(1,2,3).length
3

However, this is contrary to what both ECMA 3 and ECMA 5 (with pending correction) specify.

(For the pending ECMA 5 correction, see: https://mail.mozilla.org/pipermail/es5-discuss/2009-November/003435.html)

Comment 1

[Jim Blandy :jimb]

Attachment: Test that local functions named 'arguments' shadow arguments object.

Comment 2

[Jim Blandy :jimb]

Brendan says this is a "regression from 3.0 for sure (just tested with cvs.m.o js shell).

Comment 3

[Brendan Eich [:brendan]]

cvs.m.o js shell:

js> dis(f)
main:
00000:  deflocalfun 0 null
00005:  nop
00006:  getvar 0
00009:  return
00010:  stop

Source notes:
  0:     5 [   5] funcdef  function 0 (function arguments() {return 42;})

hg.m.o m-c js shell:

main:
00000:  trace
00001:  deflocalfun 0 function arguments() {return 42;}
00006:  nop
00007:  arguments
00008:  return
00009:  stop

Where'd that arguments (JSOP_ARGUMENTS) come from? Sigh, upvar2 damage.

/be

Comment 4

[Jeff Walden [:Waldo]]

Given the late date, and given that anyone who names a function "arguments" deserves worse than a wrong value when trying to use the name "arguments", I don't think this should block.

Comment 5

[Jim Blandy :jimb]

(In reply to comment #3)
> Where'd that arguments (JSOP_ARGUMENTS) come from? Sigh, upvar2 damage.

Indeed:

+ hg bisect --bad
The first bad revision is:
changeset:   26784:2cf0bbe3772a
user:        Brendan Eich <[e-mail]>
date:        Sun Apr 05 21:17:22 2009 -0700
summary:     upvar2, aka the big one take 2 (452598, r=mrbkap).

Comment 6

[Brendan Eich [:brendan]]

The blocking status depends on risk analysis, not attitude. People do lots of silly things in Web JS, including in sites and web apps we can't afford to break. So please add light not heat when commenting on blocking.

That said, we don't have reports of breakage from users of 3.5.x and 3.6 betas. If I can get a small patch, I'll settle for wanted1.9.2 and get it in that way. The important thing is to get a safe fix.

/be

Comment 7

[Brendan Eich [:brendan]]

Eep. The code to eagerly bind JSOP_ARGUMENTS is just wrong:

js> function f(x){ return let (arguments = x) arguments; }
js> f(42).length
1

Not that anyone uses let to bind a block-local named arguments!

/be

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

I agree with Brendan's analysis in comment 6; should fix, would take a safe patch definitely, doesn't block release since we haven't seen a lot of site bustage from this in Firefox 3.5 yet.

Comment 9

[Jeff Walden [:Waldo]]

Which I guess means we all agree.  :-)

Comment 10

[Brendan Eich [:brendan]]

Attachment: patch in progress

As discussed, slots[0] is fought over with loser suffering bugs, between first local var and arguments object. But this is close. Maybe use slots[nfixed-1]? Or something...

/be

Comment 11

[Luke Wagner [:luke]]

Oops, didn't realize this was still assigned to me.  For a while, I was interested in the 'this function uses the arguments object' compiler analysis part of the patch (for bug 539144), but I ended up not needing it.

Comment 12

[Dave Herman [:dherman]]

The bug number in changeset 2cf0bbe3772a seems to be wrong. Anyone know what the correct one is?

Thanks,
Dave

Comment 13

[Dave Herman [:dherman]]

Ah, bug 452498. For reference, changeset 2cf0bbe3772a (mozilla-central, forgot to specify) is at:

    http://hg.mozilla.org/mozilla-central/rev/2cf0bbe3772a

Dave

Comment 14

[Dave Herman [:dherman]]

Attachment: wip; dynamically sets the arg-override bit

I had to set the overrides-args bit in jsinterp and the method-JIT. (I'm not sure I did it right in the latter). Brendan tells me that ideally we should be able to exploit the CallPropertyOp path that sets the bit, so hopefully we can eliminate the added fun->atom == cx->runtime->atomState.argumentsAtom tests.

Dave

Comment 15

[Dave Herman [:dherman]]

FYI: just filed bug 604251 for similar issues of shadowed 'arguments' but with block-scoping.

Dave

Comment 16

[Igor Bukanov]

(In reply to comment #14)
> wip; dynamically sets the arg-override bit

HM, I do not see why we should set anything dynamically here. It seems a bytecode like JSOP_CALLARGUMENTS should solve the issue here.

Comment 17

[Brendan Eich [:brendan]]

Igor, can you take this bug off dherman's hands? Or let me know if not and I'll try to get to it. Your suggestion in comment 16 sounds good.

/be

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

** PRODUCT DRIVERS PLEASE NOTE **

This bug is one of 19 being moved from blocking2.0:betaN+ to blocking2.0:- as we reached the endgame of Firefox 4. The rationale for the move is:

 - the bug had been identified as a "soft" blocker which could be fixed in some follow up release
 - the bug had been identified as one requiring beta coverage, thus is not appropriate for a ".x" stability & security release

The owner of the bug may wish to renominate for .x consideration.

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

(er updating flag to "-" as per previous comment!)

Comment 20

[Jeff Walden [:Waldo]]

S10.1.3_A4_T1 in test262 checks for absence of this bug, amongst other semantics.  Anyone object overmuch if I pick up the patch here and do what needs doing to land it, or want to do that work themselves?

Comment 21

[Jim Blandy :jimb]

No objections from me.

Comment 22

[Dave Herman [:dherman]]

Have at it!

Thanks,
Dave

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Jeff, did you mean to assign this to yourself?

Comment 24

[Jeff Walden [:Waldo]]

Probably.  But I also don't really have time for this right now, alas, due to bug 586842.  Hopefully I can swing back semi-shortly after that's done.  If anyone wants to steal this in the meantime, go ahead.

Comment 25

[Luke Wagner [:luke]]

With great unhackage comes great dup'ing.

Comment 27

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 414108 [details] [diff] [review]
Test that local functions named 'arguments' shadow arguments object.

Obsoleted by dupe.

Comment 28

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 421182 [details] [diff] [review]
patch in progress

Obsoleted by dupe.

Comment 29

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 483000 [details] [diff] [review]
wip; dynamically sets the arg-override bit

Obsoleted by dupe.