Bug 25477 – [ES5] Sorting a non-array creates propreties (spec-violation)

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Lasse Reichstein Holst Nielsen]

Sorting a sparse non-array object will create properties with the value "undefined" instead of deleting existing properties.

Example:
 [].sort.call({length:5,2:42}).hasOwnProperty(2)
The resulting object has the form {length:5, 0:42, 2:undefined}. According to the specification, the "2" property should not exist.

[Alexey Proskuryakov]

Do we also disagree with IE and/or Firefox here?

[Lasse Reichstein Holst Nielsen]

IE 7 and Firefox 3.08 both give "false" on the example expression (the expected result), where JSC gives "true".
It is probably an articfact of the handling of objects with inherited array indices in Array.prototype.sort.

[Gavin Barraclough]

Created attachment 165283 [details]
Fix

[Sam Weinig]

Comment on attachment 165283 [details]
Fix

Tests?

[Oliver Hunt]

Comment on attachment 165283 [details]
Fix

r+ but are we repeatedly retrieving the methodTable?  It seems like we could retrieve it just once and be done...

[Geoffrey Garen]

/me agrees with Sam Weinig.

[Gavin Barraclough]

(In reply to comment #5)
> (From update of attachment 165283 [details])
> r+ but are we repeatedly retrieving the methodTable?  It seems like we could retrieve it just once and be done...

Ugh, I'm going to ignore that. :-)

(1) It goes against the direction I'd like us to go for methodTable() access - we should be able to add wrappers to JSCell that automatically call via the method table, to avoid the code duplication everywhere.
(2) That may not matter – the C compiler may alias the loads.  We should only manually fold the methodTable() accesses if profiling indicates this is important.
(3) This is array::sort applied to a non-array object – if performance matters we should throw away the internet and start again. :-)

You're right, might be a useful optimization in some places – I hope not though.

[Gavin Barraclough]

(In reply to comment #4)
> (From update of attachment 165283 [details])
> Tests?

Ooops, I did add a couple of tests, they seem not to be in the patch I added.  Oh well, they'll be there when I land. :-)

[Gavin Barraclough]

Fixed in r129317