Fix %GetArrayKeys to not skip non-enumerable indices

This is one step in the direction of fixing a range of small bugs in the array methods when dealing with non-standard element attributes.

Added tests exercising this behavior for shift and unshift.

For Proxies and Interceptors, the behavior of %GetArrayKeys is now to just return an interval, rather than trying to list all their indexed properties. In the Proxy case, this seems like the only way to avoid an observable difference between smart and non-smart array methods. For Interceptors, the usual case (in WebKit, anyway) is for them to have all indices in [0, length), so enumerating them won't be any better than simply iterating over that range.

Review URL: https://codereview.chromium.org/12653010

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

diff --git a/src/runtime.cc b/src/runtime.cc
index 2626f9af666f0b412c6c7f4ea0c9dc92691a6391..4aa334b7b5c4641bd4ec2e83ffb56a5348e5e861 100644 (file)
--- a/src/runtime.cc
+++ b/src/runtime.cc
@@ -9558,6 +9558,15 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_EstimateNumberOfElements) {
 }
 
 
+static Handle<Object> NewSingleInterval(Isolate* isolate, uint32_t length) {
+  Handle<FixedArray> single_interval = isolate->factory()->NewFixedArray(2);
+  // -1 means start of array.
+  single_interval->set(0, Smi::FromInt(-1));
+  single_interval->set(1, *isolate->factory()->NewNumberFromUint(length));
+  return isolate->factory()->NewJSArrayWithElements(single_interval);
+}
+
+
 // Returns an array that tells you where in the [0, length) interval an array
 // might have elements.  Can either return keys (positive integers) or
 // intervals (pair of a negative integer (-start-1) followed by a
@@ -9569,37 +9578,34 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_GetArrayKeys) {
   CONVERT_ARG_HANDLE_CHECKED(JSObject, array, 0);
   CONVERT_NUMBER_CHECKED(uint32_t, length, Uint32, args[1]);
   if (array->elements()->IsDictionary()) {
-    // Create an array and get all the keys into it, then remove all the
-    // keys that are not integers in the range 0 to length-1.
-    bool threw = false;
-    Handle<FixedArray> keys =
-        GetKeysInFixedArrayFor(array, INCLUDE_PROTOS, &threw);
-    if (threw) return Failure::Exception();
-
-    int keys_length = keys->length();
-    for (int i = 0; i < keys_length; i++) {
-      Object* key = keys->get(i);
-      uint32_t index = 0;
-      if (!key->ToArrayIndex(&index) || index >= length) {
-        // Zap invalid keys.
-        keys->set_undefined(i);
+    Handle<FixedArray> keys = isolate->factory()->empty_fixed_array();
+    for (Handle<Object> p = array;
+         !p->IsNull();
+         p = Handle<Object>(p->GetPrototype(isolate), isolate)) {
+      if (p->IsJSProxy() || JSObject::cast(*p)->HasIndexedInterceptor()) {
+        // Bail out if we find a proxy or interceptor, likely not worth
+        // collecting keys in that case.
+        return *NewSingleInterval(isolate, length);
       }
+      Handle<JSObject> current = Handle<JSObject>::cast(p);
+      Handle<FixedArray> current_keys =
+          isolate->factory()->NewFixedArray(
+              current->NumberOfLocalElements(NONE));
+      current->GetLocalElementKeys(*current_keys, NONE);
+      keys = UnionOfKeys(keys, current_keys);
+    }
+    // Erase any keys >= length.
+    // TODO(adamk): Remove this step when the contract of %GetArrayKeys
+    // is changed to let this happen on the JS side.
+    for (int i = 0; i < keys->length(); i++) {
+      if (NumberToUint32(keys->get(i)) >= length) keys->set_undefined(i);
     }
     return *isolate->factory()->NewJSArrayWithElements(keys);
   } else {
     ASSERT(array->HasFastSmiOrObjectElements() ||
            array->HasFastDoubleElements());
-    Handle<FixedArray> single_interval = isolate->factory()->NewFixedArray(2);
-    // -1 means start of array.
-    single_interval->set(0, Smi::FromInt(-1));
-    FixedArrayBase* elements = FixedArrayBase::cast(array->elements());
-    uint32_t actual_length =
-        static_cast<uint32_t>(elements->length());
-    uint32_t min_length = actual_length < length ? actual_length : length;
-    Handle<Object> length_object =
-        isolate->factory()->NewNumber(static_cast<double>(min_length));
-    single_interval->set(1, *length_object);
-    return *isolate->factory()->NewJSArrayWithElements(single_interval);
+    uint32_t actual_length = static_cast<uint32_t>(array->elements()->length());
+    return *NewSingleInterval(isolate, Min(actual_length, length));
   }
 }
 

diff --git a/test/mjsunit/array-shift.js b/test/mjsunit/array-shift.js
index 3601cbbb89aab226e5c090711aeff4a78a84bb4b..ad742e12ee18ca044da7f11be9f7cae82d6fde9b 100644 (file)
--- a/test/mjsunit/array-shift.js
+++ b/test/mjsunit/array-shift.js
@@ -106,3 +106,17 @@
   assertEquals(array[7], array_proto[7]);
   assertFalse(array.hasOwnProperty(7));
 })();
+
+// Check that non-enumerable elements are treated appropriately
+(function() {
+  var array = [1, 2, 3];
+  Object.defineProperty(array, '1', {enumerable: false});
+  assertEquals(1, array.shift());
+  assertEquals([2, 3], array);
+
+  array = [1,,3];
+  array.__proto__[1] = 2;
+  Object.defineProperty(array.__proto__, '1', {enumerable: false});
+  assertEquals(1, array.shift());
+  assertEquals([2, 3], array);
+})();

diff --git a/test/mjsunit/array-unshift.js b/test/mjsunit/array-unshift.js
index 4010601aca0d5d9003ef70d5f53ce1d0e69f105e..0eb299a0cee2a145396c2e7036d2237667c256c7 100644 (file)
--- a/test/mjsunit/array-unshift.js
+++ b/test/mjsunit/array-unshift.js
@@ -213,3 +213,18 @@
     assertEquals([1, 2, 3, 4, 5, 6, 7, 8, 9], a);
   }
 })();
+
+// Check that non-enumerable elements are treated appropriately
+(function() {
+  var array = [2, 3];
+  Object.defineProperty(array, '1', {enumerable: false});
+  array.unshift(1)
+  assertEquals([1, 2, 3], array);
+
+  array = [2];
+  array.length = 2;
+  array.__proto__[1] = 3;
+  Object.defineProperty(array.__proto__, '1', {enumerable: false});
+  array.unshift(1);
+  assertEquals([1, 2, 3], array);
+})();