130451 - [ECMA] Array.prototype.sort should not (re-)define .length

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Brendan Eich [:brendan]]

Noticed while fixing the property-tree-perf-problem found at the eleventh hour
by khanson while testing bug 99120: ECMA 15.4.4.11 stipulates that sorting does
not define or redefine or even reset the length property (if any --
Array.prototype methods are generic for the most part, and work even on
non-Array objects, even on non-Array objects lacking a 'length' property -- they
impute length of 0 via [[Get]] and ToUint32(undefined)).  Yet ever since
forever, SpiderMonkey's sort code has (re-)defined length.

Patch coming up to fix this for mozilla1.0.  I doubt anyone counts on getting a
magic length property on a non-Array object by sorting it, but I'm willing to
break them in the name of ECMA.

/be

Comment 1

[Brendan Eich [:brendan]]

Looking for r= and sr= today, so this can go in for pschwartau's RC4a tarball
tomorrow.

/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: proposed fix

Comment 3

[Mike Shaver (:shaver emeritus)]

Comment on attachment 73816 [details] [diff] [review]
proposed fix

Who doesn't love an ECMA fix?  r=shaver.

Comment 4

[John Bandhauer]

Comment on attachment 73816 [details] [diff] [review]
proposed fix

sr=jband

Comment 5

[Mike Shaver (:shaver emeritus)]

Comment on attachment 73816 [details] [diff] [review]
proposed fix

*siiiiiigh*, patch manager.

Re-applying my r=, which was stomped by jband's sr=.

Comment 6

[Phil Schwartau]

My first stab at testing this (not very good). I don't see how
to test for erroneous re-definition of length. Any suggestions? 

/*
 * Test Array.prototype.sort on non-Array objects
 */
var obj = new Object();
obj.sort = Array.prototype.sort;
obj.length = 4;
obj[0] = 0;
obj[1] = 1;
obj[2] = 2;
obj[3] = 3;
myCompare = function(x,y) {return x-y;};
actual = obj.sort(myCompare).length;
expect = 4;


This passes in the current shell (without the patch). 
Not very surprising; it's not a good test!

Comment 7

[Brendan Eich [:brendan]]

Sorry, here's a better case:

var obj = new Object();
obj.sort = Array.prototype.sort;
obj.length = 4;
obj[0] = 3;
obj[1] = 2;
obj[2] = 1;
obj[3] = 0;
obj.sort(function(x,y) {return x-y;});
o.join = Array.prototype.join;
print(o.join());  // prints "0,1,2,3"
print(o.length);  // prints 4
o.length = 2;
print(o.join());  // prints "0,1"

This demonstrates the bug.  The setting of o.length should not cause elements to
be deleted.

/be
expect = 4;

Comment 8

[Asa Dotzler [:asa]]

Comment on attachment 73816 [details] [diff] [review]
proposed fix

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 9

[Brendan Eich [:brendan]]

Fixed.

/be

Comment 10

[Phil Schwartau]

Brendan: if I run the test from Comment #7 (with the small typo "o."
corrected to "obj.") in the current JS shell, its output is the same
as it was before this bug fix. That is, it prints "0,1" at the end,
just as it used to.

Is the test correct? If so, we need to reopen this bug -

NOTE: Rhino also prints out "0,1" on this.

Comment 11

[Phil Schwartau]

FWIW, IE6 also outputs "0,1" on it -

Comment 12

[Brendan Eich [:brendan]]

The test isn't quite complete enough to prove the bug exists, sorry.

The problem is that join will consult the current 'length' property, find 2, and
return only "0,1" -- that is what we should see.  If after this point in the
test, where it ended prematurely in my typo-ridden sketch :-), you set
obj.length = 4 again, then print(obj.join()), you should see "0,1,2,3" -- if you
*still* see "0,1", then the bug is still with us.

This bug is fixed, though, so don't reopen it, but do verify that it was broken
as I describe here before this bug's patch landed.  And thanks for helping get
the test right!

/be

Comment 13

[Brendan Eich [:brendan]]

Er, I should say that when the bug was biting, if you set obj.length = 4 again
after setting it to 2, obj.join() would return "0,1,," -- note the two trailing
commas.

/be

Comment 14

[Phil Schwartau]

Confirming what Brendan said: before this fix went in, obj.join() returned 
"0,1,," instead of "0,1,2,3" after we set obj.length to 4, 2, and then 4.
The bug only occurred if Array.prototype.sort() had been applied to |obj|.
After the fix, however, obj.join() returns "0,1,2,3" as it should.

Array objects behave differently than non-Array objects on this.
With an Array object, the same sequence of steps (done more efficiently,
via its native Array methods) returns "0,1,,".

This was true both before and after this fix.

Comment 15

[Phil Schwartau]

Testcase added to JS testsuite:

          mozilla/js/tests/ecma_3/Array/regress-130451.js


Marking Verified FIXED. The testcase now passes in both the
debug and optimized JS shell.