Fix Issue 326. Handle sorting of non-array objects correctly.

Change handling of sorting to be the same for all JS-arrays.
Collect undefined values as well while removing holes.

Review URL: http://codereview.chromium.org/92123


git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@1800 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

src/array.js

@@ -709,33 +709,12 @@ function ArraySort(comparefn) {
     QuickSort(a, high_start, to);
   }
 
-  var old_length = ToUint32(this.length);
-  if (old_length < 2) return this;
-
-  %RemoveArrayHoles(this);
-
   var length = ToUint32(this.length);
+  if (length < 2) return this;
 
-  // Move undefined elements to the end of the array.
-  for (var i = 0; i < length; ) {
-    if (IS_UNDEFINED(this[i])) {
-      length--;
-      this[i] = this[length];
-      this[length] = void 0;
-    } else {
-      i++;
-    }
-  }
-
-  QuickSort(this, 0, length);
+  var num_non_undefined = %RemoveArrayHoles(this, length);
 
-  // We only changed the length of the this object (in
-  // RemoveArrayHoles) if it was an array.  We are not allowed to set
-  // the length of the this object if it is not an array because this
-  // might introduce a new length property.
-  if (IS_ARRAY(this)) {
-    this.length = old_length;
-  }
+  QuickSort(this, 0, num_non_undefined);
 
   return this;
 }

src/globals.h

@@ -86,6 +86,8 @@ const int GB = KB * KB * KB;
 const int kMaxInt = 0x7FFFFFFF;
 const int kMinInt = -kMaxInt - 1;
 
+const uint32_t kMaxUInt32 = 0xFFFFFFFFu;
+
 const int kCharSize     = sizeof(char);    // NOLINT
 const int kShortSize    = sizeof(short);   // NOLINT
 const int kIntSize      = sizeof(int);     // NOLINT

src/objects.h

@@ -1178,6 +1178,14 @@ class JSObject: public HeapObject {
   inline bool HasFastElements();
   inline Dictionary* element_dictionary();  // Gets slow elements.
 
+  // Collects elements starting at index 0.
+  // Undefined values are placed after non-undefined values.
+  // Returns the number of non-undefined values.
+  Object* PrepareElementsForSort(uint32_t limit);
+  // As PrepareElementsForSort, but only on objects where elements is
+  // a dictionary, and it will stay a dictionary.
+  Object* PrepareSlowElementsForSort(uint32_t limit);
+
   Object* SetProperty(String* key,
                       Object* value,
                       PropertyAttributes attributes);
@@ -2008,7 +2016,6 @@ class Dictionary: public DictionaryBase {
   void RemoveNumberEntries(uint32_t from, uint32_t to);
 
   // Sorting support
-  Object* RemoveHoles();
   void CopyValuesTo(FixedArray* elements);
 
   // Casting.
@@ -3892,9 +3899,6 @@ class JSArray: public JSObject {
   // Set the content of the array to the content of storage.
   inline void SetContent(FixedArray* storage);
 
-  // Support for sorting
-  Object* RemoveHoles();
-
   // Casting.
   static inline JSArray* cast(Object* obj);
 

src/runtime.cc

@@ -5244,12 +5244,16 @@ static Object* Runtime_GlobalPrint(Arguments args) {
   return string;
 }
 
-
+// Moves all own elements of an object, that are below a limit, to positions
+// starting at zero. All undefined values are placed after non-undefined values,
+// and are followed by non-existing element. Does not change the length
+// property.
+// Returns the number of non-undefined elements collected.
 static Object* Runtime_RemoveArrayHoles(Arguments args) {
-  ASSERT(args.length() == 1);
-  // Ignore the case if this is not a JSArray.
-  if (!args[0]->IsJSArray()) return args[0];
-  return JSArray::cast(args[0])->RemoveHoles();
+  ASSERT(args.length() == 2);
+  CONVERT_CHECKED(JSObject, object, args[0]);
+  CONVERT_NUMBER_CHECKED(uint32_t, limit, Uint32, args[1]);
+  return object->PrepareElementsForSort(limit);
 }
 
 

src/runtime.h

@@ -205,7 +205,7 @@ namespace v8 { namespace internal {
   F(IgnoreAttributesAndSetProperty, -1 /* 3 or 4 */) \
   \
   /* Arrays */ \
-  F(RemoveArrayHoles, 1) \
+  F(RemoveArrayHoles, 2) \
   F(GetArrayKeys, 2) \
   F(MoveArrayContents, 2) \
   F(EstimateNumberOfElements, 1) \

test/mjsunit/array-sort.js

@@ -152,3 +152,60 @@ function TestArraySortingWithUnsoundComparisonFunction() {
 }
 
 TestArraySortingWithUnsoundComparisonFunction();
+
+function TestSparseNonArraySorting(length) {
+  assertTrue(length > 101);
+  var obj = {length: length};
+  obj[0] = 42;
+  obj[10] = 37;
+  obj[100] = undefined;
+  obj[length - 1] = null;
+  Array.prototype.sort.call(obj);
+  assertEquals(length, obj.length, "objsort length unaffected");
+  assertEquals(37, obj[0], "objsort smallest number");
+  assertEquals(42, obj[1], "objsort largest number");
+  assertEquals(null, obj[2], "objsort null");
+  assertEquals(undefined, obj[3], "objsort undefined");
+  assertTrue(3 in obj, "objsort undefined retained");
+  assertFalse(4 in obj, "objsort non-existing retained");
+}
+
+TestSparseNonArraySorting(5000);
+TestSparseNonArraySorting(500000);
+TestSparseNonArraySorting(Math.pow(2, 31) + 1);
+
+function TestArrayLongerLength(length) {
+  var x = new Array(4);
+  x[0] = 42;
+  x[2] = 37;
+  x.length = length;
+  Array.prototype.sort.call(x);
+  assertEquals(length, x.length, "longlength length");
+  assertEquals(37, x[0], "longlength first");
+  assertEquals(42, x[1], "longlength second");
+  assertFalse(2 in x,"longlength third");
+}
+
+TestArrayLongerLength(4);
+TestArrayLongerLength(10);
+TestArrayLongerLength(1000);
+TestArrayLongerLength(500000);
+TestArrayLongerLength(Math.pow(2,32) - 1);
+
+function TestNonArrayLongerLength(length) {
+  var x = {};
+  x[0] = 42;
+  x[2] = 37;
+  x.length = length;
+  Array.prototype.sort.call(x);
+  assertEquals(length, x.length, "longlength length");
+  assertEquals(37, x[0], "longlength first");
+  assertEquals(42, x[1], "longlength second");
+  assertFalse(2 in x,"longlength third");
+}
+
+TestNonArrayLongerLength(4);
+TestNonArrayLongerLength(10);
+TestNonArrayLongerLength(1000);
+TestNonArrayLongerLength(500000);
+TestNonArrayLongerLength(Math.pow(2,32) - 1);

test/mjsunit/regress/regress-326.js

+// Copyright 2009 the V8 project authors. All rights reserved.
+// Redistribution and use in source and binary forms, with or without
+// modification, are permitted provided that the following conditions are
+// met:
+//
+//     * Redistributions of source code must retain the above copyright
+//       notice, this list of conditions and the following disclaimer.
+//     * Redistributions in binary form must reproduce the above
+//       copyright notice, this list of conditions and the following
+//       disclaimer in the documentation and/or other materials provided
+//       with the distribution.
+//     * Neither the name of Google Inc. nor the names of its
+//       contributors may be used to endorse or promote products derived
+//       from this software without specific prior written permission.
+//
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+// Should not crash or raise an exception.
+// Should sort non-array into equivalent of [37,42,undefined,,0]
+
+var nonArray = { length: 4, 0: 42, 2: 37, 3: undefined, 4: 0 };
+Array.prototype.sort.call(nonArray);
+
+assertEquals(4, nonArray.length, "preserve length");
+assertEquals(37, nonArray[0], "sort smallest first");
+assertEquals(42, nonArray[1], "sort largest last");
+assertTrue(2 in nonArray, "don't delete undefined");
+assertEquals(undefined, nonArray[2], "sort undefined after largest");
+assertFalse(3 in nonArray, "don't create non-existing");
+assertEquals(0, nonArray[4], "don't affect after length.");