[proxies] Refactor JSReceiver::GetKeys()

src/objects.cc

Index: src/objects.cc
diff --git a/src/objects.cc b/src/objects.cc
index 707ace30cac4b67a04843afa564361de485ff8f7..33d430e0ec34a0870d06f6632da90f9c475012a3 100644
--- a/src/objects.cc
+++ b/src/objects.cc
@@ -7965,107 +7965,133 @@ Handle<FixedArray> JSObject::GetEnumPropertyKeys(Handle<JSObject> object,
 }
 
 
+static bool GetKeysFromJSObject(Isolate* isolate, Handle<JSReceiver> receiver,
+                                Handle<JSObject> object, KeyFilter filter,
+                                Enumerability enum_policy,
+                                KeyAccumulator* accumulator) {
+  // Check access rights if required.
+  if (object->IsAccessCheckNeeded() &&
+      !isolate->MayAccess(handle(isolate->context()), object)) {
+    // TODO(jkummerow): Get whitelisted (all-can-read) keys.
+    // It's probably best to implement a "GetKeysWithFailedAccessCheck"
+    // helper, which will need to look at both interceptors and accessors.
+    return false;
+  }
+
+  PropertyAttributes attr_filter = static_cast<PropertyAttributes>(
+      (enum_policy == RESPECT_ENUMERABILITY ? DONT_ENUM : NONE) |
+      PRIVATE_SYMBOL);
+
+  JSObject::CollectOwnElementKeys(object, accumulator, attr_filter);
+
+  // Add the element keys from the interceptor.
+  if (object->HasIndexedInterceptor()) {
+    Handle<JSObject> result;
+    if (JSObject::GetKeysForIndexedInterceptor(object, receiver)
+            .ToHandle(&result)) {
+      accumulator->AddElementKeysFromInterceptor(result);
+    }
+    RETURN_VALUE_IF_SCHEDULED_EXCEPTION(isolate, false);
+  }
+
+  if (filter == SKIP_SYMBOLS) {
+    if (enum_policy == IGNORE_ENUMERABILITY) UNIMPLEMENTED();
+
+    // We can cache the computed property keys if access checks are
+    // not needed and no interceptors are involved.
+    //
+    // We do not use the cache if the object has elements and
+    // therefore it does not make sense to cache the property names
+    // for arguments objects.  Arguments objects will always have
+    // elements.
+    // Wrapped strings have elements, but don't have an elements
+    // array or dictionary.  So the fast inline test for whether to
+    // use the cache says yes, so we should not create a cache.
+    Handle<JSFunction> arguments_function(
+        JSFunction::cast(isolate->sloppy_arguments_map()->GetConstructor()));
+    bool cache_enum_length =
+        ((object->map()->GetConstructor() != *arguments_function) &&
+         !object->IsJSValue() && !object->IsAccessCheckNeeded() &&
+         !object->HasNamedInterceptor() && !object->HasIndexedInterceptor());
+    // Compute the property keys and cache them if possible.
+    Handle<FixedArray> enum_keys =
+        JSObject::GetEnumPropertyKeys(object, cache_enum_length);
+    accumulator->AddKeys(enum_keys);
+  } else {
+    DCHECK(filter == INCLUDE_SYMBOLS);
+    object->CollectOwnPropertyNames(accumulator, attr_filter);
+  }
+
+  // Add the property keys from the interceptor.
+  if (object->HasNamedInterceptor()) {
+    Handle<JSObject> result;
+    if (JSObject::GetKeysForNamedInterceptor(object, receiver)
+            .ToHandle(&result)) {
+      accumulator->AddKeys(result);
+    }
+    RETURN_VALUE_IF_SCHEDULED_EXCEPTION(isolate, false);
+  }
+  return true;
+}
+
+
+static bool GetKeysFromJSProxy(Isolate* isolate, Handle<JSReceiver> receiver,
+                               Handle<JSProxy> proxy, KeyFilter filter,
+                               Enumerability enum_policy,
+                               KeyAccumulator* accumulator) {
+  Handle<Object> args[] = {proxy};
+  Handle<Object> names;
+  ASSIGN_RETURN_ON_EXCEPTION_VALUE(
+      isolate, names, Execution::Call(isolate, isolate->proxy_enumerate(),
+                                      proxy, arraysize(args), args),
+      false);
+  accumulator->AddKeysFromProxy(Handle<JSObject>::cast(names));
+  return true;
+}
+
+
+// Returns false if an exception was thrown, or if prototype iteration
+// should not continue.
+static bool GetKeysFromJSReceiver(Isolate* isolate, Handle<JSReceiver> receiver,

[Jakob Kummerow, 2015/11/25 11:33:47]

We'll need this entry point so that the new GetKeysFromJSProxy() can implement
"Return target.[[OwnPropertyKeys]]()". In other words, this function will
(indirectly) call itself recursively.

+                                  Handle<JSReceiver> object, KeyFilter filter,
+                                  Enumerability enum_policy,
+                                  KeyAccumulator* accumulator) {
+  if (object->IsJSProxy()) {
+    return GetKeysFromJSProxy(isolate, receiver, Handle<JSProxy>::cast(object),
+                              filter, enum_policy, accumulator);
+  }
+  return GetKeysFromJSObject(isolate, receiver, Handle<JSObject>::cast(object),
+                             filter, enum_policy, accumulator);
+}
+
+
 MaybeHandle<FixedArray> JSReceiver::GetKeys(Handle<JSReceiver> object,
                                             KeyCollectionType type,
                                             KeyFilter filter,
-                                            GetKeysConversion getConversion,
+                                            GetKeysConversion keys_conversion,
                                             Enumerability enum_policy) {
   USE(ContainsOnlyValidKeys);
   Isolate* isolate = object->GetIsolate();
   KeyAccumulator accumulator(isolate, filter);
-  Handle<JSFunction> arguments_function(
-      JSFunction::cast(isolate->sloppy_arguments_map()->GetConstructor()));
   PrototypeIterator::WhereToEnd end = type == OWN_ONLY
                                           ? PrototypeIterator::END_AT_NON_HIDDEN
                                           : PrototypeIterator::END_AT_NULL;
-  PropertyAttributes attr_filter = static_cast<PropertyAttributes>(
-      (enum_policy == RESPECT_ENUMERABILITY ? DONT_ENUM : NONE) |
-      PRIVATE_SYMBOL);
-
-  // Only collect keys if access is permitted.
   for (PrototypeIterator iter(isolate, object,
                               PrototypeIterator::START_AT_RECEIVER);
        !iter.IsAtEnd(end); iter.Advance()) {
     accumulator.NextPrototype();
-    if (PrototypeIterator::GetCurrent(iter)->IsJSProxy()) {
-      Handle<JSProxy> proxy = PrototypeIterator::GetCurrent<JSProxy>(iter);
-      Handle<Object> args[] = { proxy };
-      Handle<Object> names;
-      ASSIGN_RETURN_ON_EXCEPTION(
-          isolate, names,
-          Execution::Call(isolate,
-                          isolate->proxy_enumerate(),
-                          object,
-                          arraysize(args),
-                          args),
-          FixedArray);
-      accumulator.AddKeysFromProxy(Handle<JSObject>::cast(names));
-      break;
-    }
-
-    Handle<JSObject> current = PrototypeIterator::GetCurrent<JSObject>(iter);
-
-    // Check access rights if required.
-    if (current->IsAccessCheckNeeded() &&
-        !isolate->MayAccess(handle(isolate->context()), current)) {
-      if (iter.IsAtEnd(PrototypeIterator::END_AT_NON_HIDDEN)) {

[Jakob Kummerow, 2015/11/25 11:33:47]

I've dropped this condition as discussed, because per the "hixie spec"
enumeration should silently return only whitelisted properties on access check
failure (see new TODO comment), and shouldn't throw.

-        isolate->ReportFailedAccessCheck(current);
-        RETURN_EXCEPTION_IF_SCHEDULED_EXCEPTION(isolate, FixedArray);
+    if (!GetKeysFromJSReceiver(isolate, object,
+                               PrototypeIterator::GetCurrent<JSReceiver>(iter),
+                               filter, enum_policy, &accumulator)) {
+      if (isolate->has_pending_exception()) {
+        return MaybeHandle<FixedArray>();
       }
+      // If there was no exception, then "false" means "stop iterating".
       break;
     }
-
-    JSObject::CollectOwnElementKeys(current, &accumulator, attr_filter);
-
-    // Add the element keys from the interceptor.
-    if (current->HasIndexedInterceptor()) {
-      Handle<JSObject> result;
-      if (JSObject::GetKeysForIndexedInterceptor(current, object)
-              .ToHandle(&result)) {
-        accumulator.AddElementKeysFromInterceptor(result);
-      }
-      RETURN_EXCEPTION_IF_SCHEDULED_EXCEPTION(isolate, FixedArray);
-    }
-
-    if (filter == SKIP_SYMBOLS) {
-      if (enum_policy == IGNORE_ENUMERABILITY) UNIMPLEMENTED();
-
-      // We can cache the computed property keys if access checks are
-      // not needed and no interceptors are involved.
-      //
-      // We do not use the cache if the object has elements and
-      // therefore it does not make sense to cache the property names
-      // for arguments objects.  Arguments objects will always have
-      // elements.
-      // Wrapped strings have elements, but don't have an elements
-      // array or dictionary.  So the fast inline test for whether to
-      // use the cache says yes, so we should not create a cache.
-      bool cache_enum_length =
-          ((current->map()->GetConstructor() != *arguments_function) &&
-           !current->IsJSValue() && !current->IsAccessCheckNeeded() &&
-           !current->HasNamedInterceptor() &&
-           !current->HasIndexedInterceptor());
-      // Compute the property keys and cache them if possible.
-      Handle<FixedArray> enum_keys =
-          JSObject::GetEnumPropertyKeys(current, cache_enum_length);
-      accumulator.AddKeys(enum_keys);
-    } else {
-      DCHECK(filter == INCLUDE_SYMBOLS);
-      current->CollectOwnPropertyNames(&accumulator, attr_filter);
-    }
-
-    // Add the property keys from the interceptor.
-    if (current->HasNamedInterceptor()) {
-      Handle<JSObject> result;
-      if (JSObject::GetKeysForNamedInterceptor(current, object)
-              .ToHandle(&result)) {
-        accumulator.AddKeys(result);
-      }
-      RETURN_EXCEPTION_IF_SCHEDULED_EXCEPTION(isolate, FixedArray);
-    }
   }
 
-  Handle<FixedArray> keys = accumulator.GetKeys(getConversion);
+  Handle<FixedArray> keys = accumulator.GetKeys(keys_conversion);
   DCHECK(ContainsOnlyValidKeys(keys));
   return keys;
 }