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 2626f9a..4aa334b 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 3601cbb..ad742e1 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 4010601..0eb299a 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);
+})();