178722 - calling Array.sort() on an empty array returns undefined instead of an empty array

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tommy Morgan]

I am using Mozilla 1.2b but the form did not have an option for that.

The following code illustrates the problem:

var empty = new Array();
var filled = new Array('d', 'b', 'a', 'x', 'g');

alert(empty.length);
alert(filled.length);

empty = empty.sort();
filled = filled.sort();

alert(typeof(empty));
alert(typeof(filled));

Comment 1

[rogerl (gone)]

Attachment: Return original array when length == 0

Comment 2

[Phil Schwartau]

Testcase added to JS testsuite:

      mozilla/js/tests/js1_5/Array/regress-178722.js

Currently passing in Rhino, but not in SpiderMonkey.
Will test out patch next. Meanwhile, cc'ing reviewers -

Comment 3

[Phil Schwartau]

The patch makes the testcase pass. It also passes the JS testsuite
in the optimized shell on WinNT. No test regressions are observed.
Here is how the reporter's example behaves in the patched JS shell:

js> var empty = new Array();
js> empty.length;
0

js> empty = empty.sort();
js> typeof empty;
object
js> empty instanceof Array;
true
js> empty.length;
0

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 105383 [details] [diff] [review]
Return original array when length == 0

r=[e-mail], this is good for the trunk now.	Thanks,

/be

Comment 5

[Brendan Eich [:brendan]]

Fixed with rogerl credit, I couldn't wait.  This should go onto the 1.0 branch
too, ASAP for 1.0.2.

/be

Comment 6

[Asa Dotzler [:asa]]

Comment on attachment 105383 [details] [diff] [review]
Return original array when length == 0

a=asa for checkin to the 1.2 branch (on behalf of drivers)

Comment 7

[Brendan Eich [:brendan]]

How about for 1.0.2, i.e., to the 1.0 branch?

/be

Comment 8

[Phil Schwartau]

Apologies to Roger and Asa; I'm the source of the confusion here.
Now I understand; we're looking for this fix to go into:

1. The trunk (done already)
2. -rMOZILLA_1_0_BRANCH

And it's OK if it goes into -rMOZILLA_1_2_BRANCH also.

Comment 9

[Brendan Eich [:brendan]]

Forgot to say that I did check into the 1.2 branch, it's good there too.

/be

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

Adding adt1.0.2 to make sure it's on their radar.  Brendan and I are strongly in
favor of this for 1.0.2, and for blackbird as well so web designers don't have
to worry as much about compatibility.

Extremely low risk.

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 105383 [details] [diff] [review]
Return original array when length == 0

a=[e-mail] for 1.0 branch (since it's not clear whether Asa approved it
for 1.0 or 1.2)

Comment 12

[Michael Buckland]

Discussed in team meeting.  Minusing for blackbird.  We don't know where sort is
used in our own chrome and don't know what code might depend on this bug.  If we
were to take this we would have to restart certification.

Comment 13

[Brendan Eich [:brendan]]

> We don't know where sort is used in our own chrome

Then use lxr.mozilla.org and the netscape.com commercial tree equivalent to find
all sort( calls in chrome.  You won't find any that depend on this bug, and you
don't need to "restart certification" to take this change, but I give up if you
really insist on shipping this bug.

As IE has no such bug, it is unlikely anything on the web counts on the broken
behavior of SpiderMonkey (which regressed in June of 2000).

However, mozilla1.0.2 will have this fix.

/be

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

Here's a URL for all the .sort() calls in the mozilla tree:
http://lxr.mozilla.org/seamonkey/search?string=%5C.sort%5C%28%5C%29
(i.e. search for "\.sort\(\)")

There are around 10 that aren't testcode, non-default extensions, or calls to
C++ sort() functions.

Comment 15

[Brendan Eich [:brendan]]

jesup: thanks -- http://lxr.mozilla.org/seamonkey/search?string=%5C.sort%5C%28
is a better link, as sort may have an optional compare-func parameter passed to it.

Only the venkman/cview/irc extensions use the return value of sort, and rginda
and I just looked at the venkman ones (Netscape doesn't care about the others).
 All the venkman sorts happen on non-empty arrays.  So there is no need to re-QA
a bunch of stuff, if Netscape takes this change.

/be

Comment 16

[Robert Ginda]

It turns out there is one place in venkman that would throw an exception because
of this change, but it wouldn't have worked correctly before the change either.

A patch is coming up that makes venkman work properly regardless of whether or
not this bug is fixed.

Comment 17

[Robert Ginda]

Attachment: venkman fix

Comment 18

[Brendan Eich [:brendan]]

Comment on attachment 105882 [details] [diff] [review]
venkman fix

>@@ -803,7 +810,8 @@
> {
>     var jsdFrame = console.frames[e.frameIndex];
> 
>-    displayFrame (jsdFrame, e.frameIndex, true);
>+    if ("insInteractive" in e && e.isInteractive)

Oops -- "isInteractive" was fat-fingered.  Fix that and r=me.

/be

Comment 19

[Robert Ginda]

Attachment: venkman fix, take two.

Comment 20

[Brendan Eich [:brendan]]

Comment on attachment 105998 [details] [diff] [review]
venkman fix, take two.

r=me still.

/be

Comment 21

[Brendan Eich [:brendan]]

I checked into the 1.0 branch.

rginda, over to you to get the venkman patches into whatever branches need 'em.

/be

Comment 22

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 105998 [details] [diff] [review]
venkman fix, take two.

approved for 1.2

Comment 23

[Randell Jesup [:jesup] (needinfo me)]

a=[e-mail] for venkman fix for the 1.0 branch as well, assuming it needs it.

Comment 24

[Robert Ginda]

Comment on attachment 105998 [details] [diff] [review]
venkman fix, take two.

checked in to 1.0 and 1.2 branches.  Will check in to trunk when it opens.

Comment 25

[Phil Schwartau]

It looks like Rob was able to check in the trunk Venkman changes today.

1. I have verified the jsarray.c bugfix in Comment #3 above. 
2. I have verified the jsarray.c checkin on all three branches:
   -rHEAD, -rMOZILLA_1_0_BRANCH, -rMOZILLA_1_2_BRANCH
3. I have verified the Venkman checkins on all three branches

So we can mark this bug FIXED now, right?

Comment 26

[Robert Ginda]

sounds fixed to me.

Comment 27

[Phil Schwartau]

Marking Verified per Comment #25 -