[proxies] Implement Symbol/DONT_ENUM filtering for GetKeys()

And use it to fix Object.keys() for proxies.

BUG=v8:1543
LOG=n
R=cbruni@chromium.org

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

Cr-Commit-Position: refs/heads/master@{#32496}

src/js/proxy.js

@@ -122,19 +122,6 @@ function DerivedHasOwnTrap(name) {
   return !!this.getOwnPropertyDescriptor(name)
 }
 
-function DerivedKeysTrap() {
-  var names = this.getOwnPropertyNames()
-  var enumerableNames = []
-  for (var i = 0, count = 0; i < names.length; ++i) {
-    var name = names[i]
-    if (IS_SYMBOL(name)) continue
-    var desc = this.getOwnPropertyDescriptor(TO_STRING(name))
-    if (!IS_UNDEFINED(desc) && desc.enumerable) {
-      enumerableNames[count++] = names[i]
-    }
-  }
-  return enumerableNames
-}
 
 // Implements part of ES6 9.5.11 Proxy.[[Enumerate]]:
 // Call the trap, which should return an iterator, exhaust the iterator,
@@ -176,7 +163,6 @@ utils.InstallFunctions(GlobalProxy, DONT_ENUM, [
 utils.Export(function(to) {
   to.ProxyDelegateCallAndConstruct = DelegateCallAndConstruct;
   to.ProxyDerivedHasOwnTrap = DerivedHasOwnTrap;
-  to.ProxyDerivedKeysTrap = DerivedKeysTrap;
 });
 
 %InstallToContext([

src/js/v8natives.js

@@ -27,7 +27,6 @@ var ObserveEndPerformSplice;
 var ObserveEnqueueSpliceRecord;
 var ProxyDelegateCallAndConstruct;
 var ProxyDerivedHasOwnTrap;
-var ProxyDerivedKeysTrap;
 var SameValue = utils.ImportNow("SameValue");
 var StringIndexOf;
 var toStringTagSymbol = utils.ImportNow("to_string_tag_symbol");
@@ -47,7 +46,6 @@ utils.ImportFromExperimental(function(from) {
   FLAG_harmony_tostring = from.FLAG_harmony_tostring;
   ProxyDelegateCallAndConstruct = from.ProxyDelegateCallAndConstruct;
   ProxyDerivedHasOwnTrap = from.ProxyDerivedHasOwnTrap;
-  ProxyDerivedKeysTrap = from.ProxyDerivedKeysTrap;
 });
 
 // ----------------------------------------------------------------------------
@@ -259,11 +257,6 @@ function ObjectLookupSetter(name) {
 
 function ObjectKeys(obj) {
   obj = TO_OBJECT(obj);
-  if (%_IsJSProxy(obj)) {
-    var handler = %GetHandler(obj);
-    var names = CallTrap0(handler, "keys", ProxyDerivedKeysTrap);
-    return ToNameArray(names, "keys", false);
-  }
   return %OwnKeys(obj);
 }
 

src/key-accumulator.cc

@@ -6,7 +6,9 @@
 
 #include "src/elements.h"
 #include "src/factory.h"
+#include "src/isolate-inl.h"
 #include "src/objects-inl.h"
+#include "src/property-descriptor.h"
 
 
 namespace v8 {
@@ -217,7 +219,44 @@ void KeyAccumulator::AddKeysFromProxy(Handle<JSObject> array_like) {
 }
 
 
-void KeyAccumulator::AddKeysFromProxy(Handle<FixedArray> keys) {
+MaybeHandle<FixedArray> FilterProxyKeys(Isolate* isolate, Handle<JSProxy> owner,
+                                        Handle<FixedArray> keys,
+                                        KeyFilter filter,
+                                        Enumerability enum_policy) {
+  if (filter == INCLUDE_SYMBOLS && enum_policy == IGNORE_ENUMERABILITY) {
+    // Nothing to do.
+    return keys;
+  }
+  int store_position = 0;
+  for (int i = 0; i < keys->length(); ++i) {
+    Handle<Name> key(Name::cast(keys->get(i)), isolate);
+    if (filter == SKIP_SYMBOLS && key->IsSymbol()) continue;  // Skip this key.
+    if (enum_policy == RESPECT_ENUMERABILITY) {
+      PropertyDescriptor desc;
+      bool found =
+          JSProxy::GetOwnPropertyDescriptor(isolate, owner, key, &desc);
+      if (isolate->has_pending_exception()) return MaybeHandle<FixedArray>();
+      if (!found || !desc.enumerable()) continue;  // Skip this key.
+    }
+    // Keep this key.
+    if (store_position != i) {
+      keys->set(store_position, *key);
+    }
+    store_position++;
+  }
+  if (store_position == 0) return isolate->factory()->empty_fixed_array();
+  keys->Shrink(store_position);
+  return keys;
+}
+
+
+// Returns "false" in case of exception, "true" on success.
+bool KeyAccumulator::AddKeysFromProxy(Handle<JSProxy> proxy,
+                                      Handle<FixedArray> keys, KeyFilter filter,
+                                      Enumerability enum_policy) {
+  ASSIGN_RETURN_ON_EXCEPTION_VALUE(
+      isolate_, keys,
+      FilterProxyKeys(isolate_, proxy, keys, filter, enum_policy), false);
   // Proxies define a complete list of keys with no distinction of
   // elements and properties, which breaks the normal assumption for the
   // KeyAccumulator.
@@ -226,6 +265,7 @@ void KeyAccumulator::AddKeysFromProxy(Handle<FixedArray> keys) {
   // element keys for this level. Otherwise we would not fully respect the order
   // given by the proxy.
   level_string_length_ = -level_string_length_;
+  return true;
 }
 
 

src/key-accumulator.h

@@ -44,7 +44,8 @@ class KeyAccumulator final BASE_EMBEDDED {
   void AddKeys(Handle<JSObject> array,
                AddKeyConversion convert = DO_NOT_CONVERT);
   void AddKeysFromProxy(Handle<JSObject> array);
-  void AddKeysFromProxy(Handle<FixedArray> keys);
+  bool AddKeysFromProxy(Handle<JSProxy> proxy, Handle<FixedArray> keys,
+                        KeyFilter filter, Enumerability enum_policy);
   void AddElementKeysFromInterceptor(Handle<JSObject> array);
   // Jump to the next level, pushing the current |levelLength_| to
   // |levelLengths_| and adding a new list to |elements_|.

src/objects.cc

@@ -4801,7 +4801,8 @@ Maybe<PropertyAttributes> JSProxy::GetPropertyAttributes(LookupIterator* it) {
   Isolate* isolate = it->isolate();
   HandleScope scope(isolate);
   PropertyDescriptor desc;
-  bool found = JSProxy::GetOwnPropertyDescriptor(it, &desc);
+  bool found = JSProxy::GetOwnPropertyDescriptor(
+      isolate, it->GetHolder<JSProxy>(), it->GetName(), &desc);
   if (isolate->has_pending_exception()) return Nothing<PropertyAttributes>();
   if (!found) return Just(ABSENT);
   return Just(desc.ToAttributes());
@@ -6931,12 +6932,13 @@ bool JSReceiver::GetOwnPropertyDescriptor(Isolate* isolate,
 // static
 bool JSReceiver::GetOwnPropertyDescriptor(LookupIterator* it,
                                           PropertyDescriptor* desc) {
+  Isolate* isolate = it->isolate();
   // "Virtual" dispatch.
   if (it->IsFound() && it->GetHolder<JSReceiver>()->IsJSProxy()) {
-    return JSProxy::GetOwnPropertyDescriptor(it, desc);
+    return JSProxy::GetOwnPropertyDescriptor(isolate, it->GetHolder<JSProxy>(),
+                                             it->GetName(), desc);
   }
 
-  Isolate* isolate = it->isolate();
   // 1. (Assert)
   // 2. If O does not have an own property with key P, return undefined.
   Maybe<PropertyAttributes> maybe = JSObject::GetPropertyAttributes(it);
@@ -6985,13 +6987,11 @@ bool JSReceiver::GetOwnPropertyDescriptor(LookupIterator* it,
 
 // ES6 9.5.5
 // static
-bool JSProxy::GetOwnPropertyDescriptor(LookupIterator* it,
+bool JSProxy::GetOwnPropertyDescriptor(Isolate* isolate, Handle<JSProxy> proxy,
+                                       Handle<Name> name,
                                        PropertyDescriptor* desc) {
-  Handle<JSProxy> proxy = it->GetHolder<JSProxy>();
-  Isolate* isolate = it->isolate();
   Handle<String> trap_name =
       isolate->factory()->getOwnPropertyDescriptor_string();
-  Handle<Name> property_name = it->GetName();
   // 1. (Assert)
   // 2. Let handler be the value of the [[ProxyHandler]] internal slot of O.
   Handle<Object> handler(proxy->handler(), isolate);
@@ -7004,10 +7004,9 @@ bool JSProxy::GetOwnPropertyDescriptor(LookupIterator* it,
   // 4. Assert: Type(handler) is Object.
   DCHECK(handler->IsJSReceiver());
   // If the handler is not null, the target can't be null either.
-  DCHECK(it->GetHolder<JSProxy>()->target()->IsJSReceiver());
+  DCHECK(proxy->target()->IsJSReceiver());
   // 5. Let target be the value of the [[ProxyTarget]] internal slot of O.
-  Handle<JSReceiver> target(
-      JSReceiver::cast(it->GetHolder<JSProxy>()->target()), isolate);
+  Handle<JSReceiver> target(JSReceiver::cast(proxy->target()), isolate);
   // 6. Let trap be ? GetMethod(handler, "getOwnPropertyDescriptor").
   Handle<Object> trap;
   ASSIGN_RETURN_ON_EXCEPTION_VALUE(
@@ -7016,12 +7015,11 @@ bool JSProxy::GetOwnPropertyDescriptor(LookupIterator* it,
   // 7. If trap is undefined, then
   if (trap->IsUndefined()) {
     // 7a. Return target.[[GetOwnProperty]](P).
-    return JSReceiver::GetOwnPropertyDescriptor(isolate, target, property_name,
-                                                desc);
+    return JSReceiver::GetOwnPropertyDescriptor(isolate, target, name, desc);
   }
   // 8. Let trapResultObj be ? Call(trap, handler, «target, P»).
   Handle<Object> trap_result_obj;
-  Handle<Object> args[] = {target, property_name};
+  Handle<Object> args[] = {target, name};
   ASSIGN_RETURN_ON_EXCEPTION_VALUE(
       isolate, trap_result_obj,
       Execution::Call(isolate, trap, handler, arraysize(args), args), false);
@@ -7030,13 +7028,12 @@ bool JSProxy::GetOwnPropertyDescriptor(LookupIterator* it,
   if (!trap_result_obj->IsJSReceiver() && !trap_result_obj->IsUndefined()) {
     isolate->Throw(*isolate->factory()->NewTypeError(
         MessageTemplate::kProxyHandlerReturned, handler, trap_result_obj,
-        property_name));
+        name));
     return false;
   }
   // 10. Let targetDesc be ? target.[[GetOwnProperty]](P).
   PropertyDescriptor target_desc;
-  JSReceiver::GetOwnPropertyDescriptor(isolate, target, property_name,
-                                       &target_desc);
+  JSReceiver::GetOwnPropertyDescriptor(isolate, target, name, &target_desc);
   if (isolate->has_pending_exception()) return false;
   // 11. If trapResultObj is undefined, then
   if (trap_result_obj->IsUndefined()) {
@@ -7046,7 +7043,7 @@ bool JSProxy::GetOwnPropertyDescriptor(LookupIterator* it,
     //      exception.
     if (!target_desc.configurable()) {
       isolate->Throw(*isolate->factory()->NewTypeError(
-          MessageTemplate::kProxyTargetPropNotConfigurable, property_name));
+          MessageTemplate::kProxyTargetPropNotConfigurable, name));
       return false;
     }
     // 11c. Let extensibleTarget be ? IsExtensible(target).
@@ -7078,7 +7075,7 @@ bool JSProxy::GetOwnPropertyDescriptor(LookupIterator* it,
   // 15. Let valid be IsCompatiblePropertyDescriptor (extensibleTarget,
   //     resultDesc, targetDesc).
   bool valid = IsCompatiblePropertyDescriptor(isolate, extensible_target, desc,
-                                              &target_desc, property_name);
+                                              &target_desc, name);
   // 16. If valid is false, throw a TypeError exception.
   if (!valid) {
     DCHECK(isolate->has_pending_exception());
@@ -7090,7 +7087,7 @@ bool JSProxy::GetOwnPropertyDescriptor(LookupIterator* it,
     if (target_desc.is_empty() || target_desc.configurable()) {
       // 17a i. Throw a TypeError exception.
       isolate->Throw(*isolate->factory()->NewTypeError(
-          MessageTemplate::kRedefineDisallowed, property_name));
+          MessageTemplate::kRedefineDisallowed, name));
       return false;
     }
   }
@@ -8326,7 +8323,6 @@ static MaybeHandle<FixedArray> CreateListFromArrayLike_StringSymbol(
 
 // ES6 9.5.12
 // Returns "false" in case of exception.
-// TODO(jkummerow): |filter| and |enum_policy| are currently ignored.
 // static
 bool JSProxy::OwnPropertyKeys(Isolate* isolate, Handle<JSReceiver> receiver,
                               Handle<JSProxy> proxy, KeyFilter filter,
@@ -8413,8 +8409,8 @@ bool JSProxy::OwnPropertyKeys(Isolate* isolate, Handle<JSReceiver> receiver,
   //     then:
   if (extensible_target && nonconfigurable_keys_length == 0) {
     // 15a. Return trapResult.
-    accumulator->AddKeysFromProxy(trap_result);
-    return true;
+    return accumulator->AddKeysFromProxy(proxy, trap_result, filter,
+                                         enum_policy);
   }
   // 16. Let uncheckedResultKeys be a new List which is a copy of trapResult.
   Zone set_zone;
@@ -8443,8 +8439,8 @@ bool JSProxy::OwnPropertyKeys(Isolate* isolate, Handle<JSReceiver> receiver,
   }
   // 18. If extensibleTarget is true, return trapResult.
   if (extensible_target) {
-    accumulator->AddKeysFromProxy(trap_result);
-    return true;
+    return accumulator->AddKeysFromProxy(proxy, trap_result, filter,
+                                         enum_policy);
   }
   // 19. Repeat, for each key that is an element of targetConfigurableKeys:
   for (int i = 0; i < target_configurable_keys->length(); ++i) {
@@ -8470,8 +8466,7 @@ bool JSProxy::OwnPropertyKeys(Isolate* isolate, Handle<JSReceiver> receiver,
     return false;
   }
   // 21. Return trapResult.
-  accumulator->AddKeysFromProxy(trap_result);
-  return true;
+  return accumulator->AddKeysFromProxy(proxy, trap_result, filter, enum_policy);
 }
 
 

src/objects.h

@@ -9513,7 +9513,8 @@ class JSProxy: public JSReceiver {
       Handle<JSProxy> proxy, ShouldThrow should_throw);
 
   // ES6 9.5.5
-  static bool GetOwnPropertyDescriptor(LookupIterator* it,
+  static bool GetOwnPropertyDescriptor(Isolate* isolate, Handle<JSProxy> proxy,
+                                       Handle<Name> name,
                                        PropertyDescriptor* desc);
 
   // ES6 9.5.6

src/runtime/runtime-object.cc

@@ -987,8 +987,7 @@ RUNTIME_FUNCTION(Runtime_GetIndexedInterceptorElementNames) {
 RUNTIME_FUNCTION(Runtime_OwnKeys) {
   HandleScope scope(isolate);
   DCHECK(args.length() == 1);
-  CONVERT_ARG_CHECKED(JSObject, raw_object, 0);
-  Handle<JSObject> object(raw_object);
+  CONVERT_ARG_HANDLE_CHECKED(JSReceiver, object, 0);
 
   Handle<FixedArray> contents;
   ASSIGN_RETURN_FAILURE_ON_EXCEPTION(

test/mjsunit/harmony/proxies-keys.js

+// Copyright 2015 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --harmony-proxies
+
+var target = {
+  target: 1
+};
+target.__proto__ = {
+  target_proto: 2
+};
+
+var handler = {
+  ownKeys: function(target) {
+    return ["foo", "bar", Symbol("baz"), "non-enum", "not-found"];
+  },
+  getOwnPropertyDescriptor: function(target, name) {
+    if (name == "non-enum") return {configurable: true};
+    if (name == "not-found") return undefined;
+    return {enumerable: true, configurable: true};
+  }
+}
+
+var proxy = new Proxy(target, handler);
+
+// Object.keys() ignores symbols and non-enumerable keys.
+assertEquals(["foo", "bar"], Object.keys(proxy));
+
+// Edge case: no properties left after filtering.
+handler.getOwnPropertyDescriptor = undefined;
+assertEquals([], Object.keys(proxy));
+
+// Throwing shouldn't crash.
+handler.getOwnPropertyDescriptor = function() { throw new Number(1); };
+assertThrows("Object.keys(proxy)", Number);
+
+// Fall through to target if there is no trap.
+handler.ownKeys = undefined;
+assertEquals(["target"], Object.keys(proxy));
+assertEquals(["target"], Object.keys(target));