Fix %GetArrayKeys to not skip non-enumerable indices

src/runtime.cc

Index: src/runtime.cc
diff --git a/src/runtime.cc b/src/runtime.cc
index 38f77d07137eb06acec574d67d409cdfca073cd1..dade9c45d56890a9b6f36ef523407355835df068 100644
--- a/src/runtime.cc
+++ b/src/runtime.cc
@@ -9540,6 +9540,15 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_EstimateNumberOfElements) {
 }
 
 
+static Object* NewSingleInterval(Isolate* isolate, uint32_t length) {

[Michael Starzinger, 2013/03/22 10:12:20]

It is dangerous to have a method returning an unhandlified value, that has a
handlified implementation. Better return Handle<Object> here.

[adamk, 2013/03/22 15:11:18]

Fair point. Going back and forth between JS and C++ makes me wish for
function-scoped functions in the latter...

+  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
@@ -9551,13 +9560,22 @@ 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();
-
+    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);
+    }
     int keys_length = keys->length();

[rossberg, 2013/03/22 14:22:34]

Isn't the filtering below redundant now? You only collect elements anyway.

[adamk, 2013/03/22 15:11:18]

I thought so too, but a test caught me: still need to filter out things >=
length. Or I guess I could do that in JS, would that be a better approach?

[rossberg, 2013/03/22 17:27:22]

Oh dear. Well, since the spec of the function says so, it probably has to be
done here. But you can still remove the first half of the condition below. And
maybe add a brief comment about what's being filtered.

[adamk, 2013/03/22 17:51:34]

Added a comment, and a TODO (as I plan to change this function to be more
sensible).

     for (int i = 0; i < keys_length; i++) {
       Object* key = keys->get(i);
@@ -9571,17 +9589,8 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_GetArrayKeys) {
   } 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));
   }
 }