299644 - Javascript's concat doesn't verify, if the properties exist.

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[mozilla]

User-Agent:       Mozilla/5.0 (compatible; Konqueror/3.4; Linux) KHTML/3.4.0 (like Gecko)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.6) 

According to ECMAscript spec (section 15.4.4.4 step 9) the concat 
function should test (for each element < array.length), if the array has 
an entry. An concat on an empty array (of non-0 length) should therefore 
produce an empty array. 

Reproducible: Always

Comment 1

[mozilla]

Attachment: a small test-case illustrating the bug.

the array "b", should be empty, as "a" doesn't contain any property.
a "for ... in" should therefore not print anything.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

I think we may actually be following the spec here. Notice that step 9 says to
go to step 13 if the property doesn't exist, but step 13 is incrementing n, so
in the new array, this location would be a "hole" in the new array also. Also
notice that step 22 sets the new array's length to be n, which in this case
would be 10 (per step 13), giving us the correct answer. Am I missing anything?

Note: I have a patch that reflects the spec. a bit better than the current code
(and avoids calling OBJ_SET_PROPERTY() for the holes) that I'll attach in a
second, but it preserves the same behavior (as far as I can tell) on the given
testcase.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: the promised patch

I'm not sure if we want this patch. I *think* it's more "correct" than the
current code, but I guess Brendan or Shaver get the final call on this.

Comment 4

[Brendan Eich [:brendan]]

mrbkap's right, the bug is INVALID as stated in comment 0 -- see ECMA-262 Ed. 3,
15.4.4.4 steps 9, 13, and 22.

mrbkap: do you want to morph this bug, or is there a better bug in which to
patch SpiderMonkey to preserve holes?

/be

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

I'll mark this as INVALID and open a new bug to audit for Spidermonkey plugging
holes in arrays.

Comment 6

[mozilla]

as far as I understood, in the test-case b should be identical to a. But when 
I do "for(var v in a) alert(v);" I will get no alert, but "for(var v in b) 
alert(v)" will alert ten times. Maybe my version is just too old (but I even 
tried on a 1.7.8). 
Maybe the given patch fixes this, but in this case it shouldn't be "invalid". 

Comment 7

[Brendan Eich [:brendan]]

(In reply to comment #6)
> as far as I understood, in the test-case b should be identical to a. But when 
> I do "for(var v in a) alert(v);" I will get no alert, but "for(var v in b) 
> alert(v)" will alert ten times. Maybe my version is just too old (but I even 
> tried on a 1.7.8). 

No, your version is not too old, but the summary in comment 0 is still invalid.
 (new Array(10).concat()).length must be 10 by ECMA-262 Ed. 3.

> Maybe the given patch fixes this, but in this case it shouldn't be "invalid". 

The bug where we fill holes enumerably is valid, you're right.  The claim that
the concat result should be empty as in zero length is still invalid.

mrbkap, let's make progress here with a spot fix, ok?

/be

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 188222 [details] [diff] [review]
the promised patch

r+a=me, thanks.

/be

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked in. I filed bug 299738 on the more general problem of jsarray.c
functions plugging holes in arrays.

Comment 10

[Bob Clary [:bc] (inactive)]

Checking in regress-299644.js;
/cvsroot/mozilla/js/tests/js1_5/Array/regress-299644.js,v  <--  regress-299644.js
initial revision: 1.1

Comment 11

[Bob Clary [:bc] (inactive)]

verified fixed 1.9 20060818