72884 - Arguments object has incorrect [[Prototype]] property

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Phil Schwartau]

This was discovered during the fix for bug 72773. Brendan's comment:

"There's still an ECMA deviation here: our Arguments class does not match 
10.1.8, which says arguments objects have as their prototype the original 
value of the Object.prototype property.  What's more, we delegate properties 
such as length, 0, 1, etc. to Arguments.prototype.  So in SpiderMonkey,

  function f(){delete arguments.length; return arguments.length}
  print(f(1,2,3));

prints 3, rather than undefined as it should per ECMA."

Comment 1

[Phil Schwartau]

I will add coverage to the JS testsuite for this - 

Comment 2

[Brendan Eich [:brendan]]

Attachment: proposed fix

Comment 3

[Brendan Eich [:brendan]]

This patch passes the JS testsuite.

Oops, it doesn't pass the test I included, because after the function
successfully deletes length, args_resolve re-resolves it!  New patch in a sec.

/be

Comment 4

[Brendan Eich [:brendan]]

Attachment: better fix: check for overridden properties in js_GetArgsProperty

Comment 5

[Brendan Eich [:brendan]]

Better test:

function g(){delete arguments.length; return arguments}

a = g(); print(a.length)		// prints undefined
a = g(1,2,3); print(a.length)		// prints undefined
print(a[0]);print(a[1]);print(a[2]);	// prints 1\n2\n3\n

Note that only callee and length can be overridden, so deleting an indexed
property and asking for it again causes it to be recreated by args_resolve:

function g(){delete arguments[0]; return arguments[0]}
g(42)					// prints 42

I'm not positive this violates ECMA, which allows in chapter 16 for extensions
including properties (does it allow for magically reappearing properties?).  The
delete operator successfully deletes arguments[0] and results in true, but that
is not distinguishable from the case where arguments[0] was delegated to
Arguments.prototype[0], which was how the bad old code worked.

I'll ponder this last detail for too many hours tonight.

/be

Comment 6

[Brendan Eich [:brendan]]

Attachment: best fix, but not as small as I'd hoped for

Comment 7

[Brendan Eich [:brendan]]

Review craved.  This patch satisfies ECMA and avoids preallocating arguments
properties.  The diff is only confusing where the Arguments constructor appears
to be replaced by args_delProperty, the JSClass-level delete observer.

/be

Comment 8

[Mike Shaver (:shaver emeritus)]

I like the named-as-"Object" trick.  And I like the #ifdef JS_DOUBLE_HASHING
sneak peek even more!

r=shaver.

Comment 9

[John Bandhauer]

sr=jband. Oh boy! A slick new use for ReservedSlot.

Comment 10

[Brendan Eich [:brendan]]

Fixed.

/be

Comment 11

[Brendan Eich [:brendan]]

I need bugzilla to check for "Fixed." as the only significant line in a comment
by the assignee, and ask whether the luser forgot to resolve the bug as FIXED!

/be

Comment 12

[Phil Schwartau]

Brendan's testcase added to JS testsuite:

         js/tests/ecma_3/Function] cvs ci arguments-001.js


Currently passing in Rhino, but failing in SpiderMonkey on the final
part of the test: 

function g(){delete arguments[0]; return arguments[0]}
g(42)                                   // prints 42


Instead of printing 42, it prints 'undefined' - 

Comment 13

[Phil Schwartau]

Here is a clearer path to the testcase:

             js/tests/ecma_3/Function/arguments-001.js

Comment 14

[Phil Schwartau]

Reopening bug due to the testcase failure: 


In SpiderMonkey: test FAILS
function g(){delete arguments[0]; return arguments[0]}
g(42)
undefined // should print 42


In Rhino : test PASSES
function g(){delete arguments[0]; return arguments[0]}
g(42)
42

Comment 15

[Brendan Eich [:brendan]]

I didn't update the last comment on the topic to say that, per ECMA-262, delete
on an arguments[i] should succeed and remove that property from the arguments
object, leaving any get of it after the delete to evaluate to undefined.  So the
bug is fixed, sorry for the confusion.

Is Rhino busted?

/be

Comment 16

[Phil Schwartau]

I have corrected the testcase per Brendan's comment above. 
Testcase passing on WinNT, Linux in debug/optimized JS shells.
Marking Verified - 

Bug 79568 has been filed against Rhino: 
"delete on an arguments[i] not working correctly"