Bug 25477 - [ES5] Sorting a non-array creates propreties (spec-violation)
Summary: [ES5] Sorting a non-array creates propreties (spec-violation)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Minor
Assignee: Gavin Barraclough
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-04-29 23:57 PDT by Lasse Reichstein Holst Nielsen
Modified: 2012-09-23 22:34 PDT (History)
3 users (show)

See Also:


Attachments
Fix (4.20 KB, patch)
2012-09-23 00:10 PDT, Gavin Barraclough
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Lasse Reichstein Holst Nielsen 2009-04-29 23:57:01 PDT
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.
Comment 1 Alexey Proskuryakov 2009-05-01 03:51:56 PDT
Do we also disagree with IE and/or Firefox here?
Comment 2 Lasse Reichstein Holst Nielsen 2009-05-01 04:38:57 PDT
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.
Comment 3 Gavin Barraclough 2012-09-23 00:10:49 PDT
Created attachment 165283 [details]
Fix
Comment 4 Sam Weinig 2012-09-23 00:13:12 PDT
Comment on attachment 165283 [details]
Fix

Tests?
Comment 5 Oliver Hunt 2012-09-23 12:04:14 PDT
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...
Comment 6 Geoffrey Garen 2012-09-23 15:00:52 PDT
/me agrees with Sam Weinig.
Comment 7 Gavin Barraclough 2012-09-23 15:29:12 PDT
(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.
Comment 8 Gavin Barraclough 2012-09-23 15:29:49 PDT
(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. :-)
Comment 9 Gavin Barraclough 2012-09-23 22:34:23 PDT
Fixed in r129317