[proxies] use [[GetPrototypeOf]] trap in for-in key accumulation

With the recent spec change removing the [[Enumerate]] internal method, we now
have to walk the complete prototype chain. This implies that we call the
[[GetPrototypeOf]] trap on proxies.

As a secondary change we now trigger the [[GetOwnProperty]] trap for the for-in
filter step to see whether the properties are still enumerable. Before we did this
in the key-accumulation phase. This way we slightly reduce the number of traps
invoked. Whilst this is not ideal, it comes closer to the Spec's example
implementation.

BUG=v8:1543, v8:4768
LOG=n

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

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

src/keys.cc

@@ -252,9 +252,11 @@ MaybeHandle<FixedArray> FilterProxyKeys(Isolate* isolate, Handle<JSProxy> owner,
 // Returns "nothing" in case of exception, "true" on success.
 Maybe<bool> KeyAccumulator::AddKeysFromProxy(Handle<JSProxy> proxy,
                                              Handle<FixedArray> keys) {
-  ASSIGN_RETURN_ON_EXCEPTION_VALUE(
-      isolate_, keys, FilterProxyKeys(isolate_, proxy, keys, filter_),
-      Nothing<bool>());
+  if (filter_proxy_keys_) {
+    ASSIGN_RETURN_ON_EXCEPTION_VALUE(
+        isolate_, keys, FilterProxyKeys(isolate_, proxy, keys, filter_),
+        Nothing<bool>());
+  }
   // Proxies define a complete list of keys with no distinction of
   // elements and properties, which breaks the normal assumption for the
   // KeyAccumulator.
@@ -438,7 +440,8 @@ MaybeHandle<FixedArray> FastKeyAccumulator::GetKeysFast(
 
 MaybeHandle<FixedArray> FastKeyAccumulator::GetKeysSlow(
     GetKeysConversion convert) {
-  return JSReceiver::GetKeys(receiver_, type_, ENUMERABLE_STRINGS);
+  return JSReceiver::GetKeys(receiver_, type_, ENUMERABLE_STRINGS, KEEP_NUMBERS,
+                             filter_proxy_keys_);
 }
 
 }  // namespace internal

src/keys.h

@@ -52,6 +52,7 @@ class KeyAccumulator final BASE_EMBEDDED {
   Handle<FixedArray> GetKeys(GetKeysConversion convert = KEEP_NUMBERS);
   int length() { return length_; }
   Isolate* isolate() { return isolate_; }
+  void set_filter_proxy_keys(bool filter) { filter_proxy_keys_ = filter; }
 
  private:
   bool AddIntegerKey(uint32_t key);
@@ -62,6 +63,7 @@ class KeyAccumulator final BASE_EMBEDDED {
   Isolate* isolate_;
   KeyCollectionType type_;
   PropertyFilter filter_;
+  bool filter_proxy_keys_ = true;
   // |elements_| contains the sorted element keys (indices) per level.
   std::vector<std::vector<uint32_t>*> elements_;
   // |protoLengths_| contains the total number of keys (elements + properties)
@@ -101,6 +103,7 @@ class FastKeyAccumulator {
 
   bool is_receiver_simple_enum() { return is_receiver_simple_enum_; }
   bool has_empty_prototype() { return has_empty_prototype_; }
+  void set_filter_proxy_keys(bool filter) { filter_proxy_keys_ = filter; }
 
   MaybeHandle<FixedArray> GetKeys(GetKeysConversion convert = KEEP_NUMBERS);
 
@@ -115,6 +118,7 @@ class FastKeyAccumulator {
   PropertyFilter filter_;
   bool is_receiver_simple_enum_ = false;
   bool has_empty_prototype_ = false;
+  bool filter_proxy_keys_ = true;
 
   DISALLOW_COPY_AND_ASSIGN(FastKeyAccumulator);
 };

src/objects-inl.h

@@ -7083,7 +7083,6 @@ NameDictionary* JSReceiver::property_dictionary() {
   return NameDictionary::cast(properties());
 }
 
-
 Maybe<bool> JSReceiver::HasProperty(Handle<JSReceiver> object,
                                     Handle<Name> name) {
   LookupIterator it = LookupIterator::PropertyOrElement(object->GetIsolate(),

src/objects.cc

@@ -684,13 +684,12 @@ Maybe<bool> JSReceiver::HasProperty(LookupIterator* it) {
       case LookupIterator::TRANSITION:
         UNREACHABLE();
       case LookupIterator::JSPROXY:
-        // Call the "has" trap on proxies.
         return JSProxy::HasProperty(it->isolate(), it->GetHolder<JSProxy>(),
                                     it->GetName());
       case LookupIterator::INTERCEPTOR: {
         Maybe<PropertyAttributes> result =
             JSObject::GetPropertyAttributesWithInterceptor(it);
-        if (!result.IsJust()) return Nothing<bool>();
+        if (result.IsNothing()) return Nothing<bool>();
         if (result.FromJust() != ABSENT) return Just(true);
         break;
       }
@@ -698,7 +697,7 @@ Maybe<bool> JSReceiver::HasProperty(LookupIterator* it) {
         if (it->HasAccess()) break;
         Maybe<PropertyAttributes> result =
             JSObject::GetPropertyAttributesWithFailedAccessCheck(it);
-        if (!result.IsJust()) return Nothing<bool>();
+        if (result.IsNothing()) return Nothing<bool>();
         return Just(result.FromJust() != ABSENT);
       }
       case LookupIterator::INTEGER_INDEXED_EXOTIC:
@@ -5144,11 +5143,9 @@ MaybeHandle<Context> JSReceiver::GetFunctionRealm(Handle<JSReceiver> receiver) {
 
 
 Maybe<PropertyAttributes> JSProxy::GetPropertyAttributes(LookupIterator* it) {
-  Isolate* isolate = it->isolate();
-  HandleScope scope(isolate);
   PropertyDescriptor desc;
   Maybe<bool> found = JSProxy::GetOwnPropertyDescriptor(
-      isolate, it->GetHolder<JSProxy>(), it->GetName(), &desc);
+      it->isolate(), it->GetHolder<JSProxy>(), it->GetName(), &desc);
   MAYBE_RETURN(found, Nothing<PropertyAttributes>());
   if (!found.FromJust()) return Just(ABSENT);
   return Just(desc.ToAttributes());
@@ -8515,12 +8512,23 @@ static Maybe<bool> GetKeys_Internal(Isolate* isolate,
                                     KeyCollectionType type,
                                     PropertyFilter filter,
                                     KeyAccumulator* accumulator) {
+  // Proxies have no hidden prototype and we should not trigger the
+  // [[GetPrototypeOf]] trap on the last iteration when using
+  // AdvanceFollowingProxies.
+  if (type == OWN_ONLY && object->IsJSProxy()) {
+    MAYBE_RETURN(JSProxy::OwnPropertyKeys(isolate, receiver,
+                                          Handle<JSProxy>::cast(object), filter,
+                                          accumulator),
+                 Nothing<bool>());
+    return Just(true);
+  }
+
   PrototypeIterator::WhereToEnd end = type == OWN_ONLY
                                           ? PrototypeIterator::END_AT_NON_HIDDEN
                                           : PrototypeIterator::END_AT_NULL;
   for (PrototypeIterator iter(isolate, object,
                               PrototypeIterator::START_AT_RECEIVER, end);
-       !iter.IsAtEnd(); iter.Advance()) {
+       !iter.IsAtEnd();) {
     Handle<JSReceiver> current =
         PrototypeIterator::GetCurrent<JSReceiver>(iter);
     Maybe<bool> result = Just(false);  // Dummy initialization.
@@ -8536,6 +8544,11 @@ static Maybe<bool> GetKeys_Internal(Isolate* isolate,
     }
     MAYBE_RETURN(result, Nothing<bool>());
     if (!result.FromJust()) break;  // |false| means "stop iterating".
+    // Iterate through proxies but ignore access checks for the ALL_CAN_READ
+    // case on API objects for OWN_ONLY keys handlede in GgetKeysFromJSObject.
+    if (!iter.AdvanceFollowingProxiesIgnoringAccessChecks()) {
+      return Nothing<bool>();
+    }
   }
   return Just(true);
 }
@@ -8694,14 +8707,15 @@ Maybe<bool> JSProxy::OwnPropertyKeys(Isolate* isolate,
   return accumulator->AddKeysFromProxy(proxy, trap_result);
 }
 
-
 MaybeHandle<FixedArray> JSReceiver::GetKeys(Handle<JSReceiver> object,
                                             KeyCollectionType type,
                                             PropertyFilter filter,
-                                            GetKeysConversion keys_conversion) {
+                                            GetKeysConversion keys_conversion,
+                                            bool filter_proxy_keys) {
   USE(ContainsOnlyValidKeys);
   Isolate* isolate = object->GetIsolate();
   KeyAccumulator accumulator(isolate, type, filter);
+  accumulator.set_filter_proxy_keys(filter_proxy_keys);
   MAYBE_RETURN(
       GetKeys_Internal(isolate, object, object, type, filter, &accumulator),
       MaybeHandle<FixedArray>());

src/objects.h

@@ -1962,7 +1962,8 @@ class JSReceiver: public HeapObject {
   // "for (n in object) { }".
   MUST_USE_RESULT static MaybeHandle<FixedArray> GetKeys(
       Handle<JSReceiver> object, KeyCollectionType type, PropertyFilter filter,
-      GetKeysConversion keys_conversion = KEEP_NUMBERS);
+      GetKeysConversion keys_conversion = KEEP_NUMBERS,
+      bool filter_proxy_keys_ = true);
 
   MUST_USE_RESULT static MaybeHandle<FixedArray> GetOwnValues(
       Handle<JSReceiver> object, PropertyFilter filter);

src/prototype.h

@@ -125,7 +125,7 @@ class PrototypeIterator {
 
   // Returns false iff a call to JSProxy::GetPrototype throws.
   // TODO(neis): This should probably replace Advance().
-  bool AdvanceFollowingProxies() {
+  MUST_USE_RESULT bool AdvanceFollowingProxies() {
     DCHECK(!(handle_.is_null() && object_->IsJSProxy()));
     if (!HasAccess()) {
       // Abort the lookup if we do not have access to the current object.
@@ -133,10 +133,15 @@ class PrototypeIterator {
       is_at_end_ = true;
       return true;
     }
+    return AdvanceFollowingProxiesIgnoringAccessChecks();
+  }
+
+  MUST_USE_RESULT bool AdvanceFollowingProxiesIgnoringAccessChecks() {
     if (handle_.is_null() || !handle_->IsJSProxy()) {
       AdvanceIgnoringProxies();
       return true;
     }
+
     // Due to possible __proto__ recursion limit the number of Proxies
     // we visit to an arbitrarily chosen large number.
     seen_proxies_++;

src/runtime/runtime-forin.cc

@@ -24,6 +24,7 @@ MaybeHandle<HeapObject> Enumerate(Handle<JSReceiver> receiver) {
   Isolate* const isolate = receiver->GetIsolate();
   FastKeyAccumulator accumulator(isolate, receiver, INCLUDE_PROTOS,
                                  ENUMERABLE_STRINGS);
+  accumulator.set_filter_proxy_keys(false);
   // Test if we have an enum cache for {receiver}.
   if (!accumulator.is_receiver_simple_enum()) {
     Handle<FixedArray> keys;
@@ -35,28 +36,70 @@ MaybeHandle<HeapObject> Enumerate(Handle<JSReceiver> receiver) {
   return handle(receiver->map(), isolate);
 }
 
-
-MaybeHandle<Object> Filter(Handle<JSReceiver> receiver, Handle<Object> key) {
-  Isolate* const isolate = receiver->GetIsolate();
-  Handle<Name> name;
-  ASSIGN_RETURN_ON_EXCEPTION(isolate, name, Object::ToName(isolate, key),
-                             Object);
-  // Directly check for elements if the key is a smi and avoid a conversion
-  // roundtrip (Number -> Name -> Number).
-  if (key->IsNumber() && receiver->map()->OnlyHasSimpleProperties()) {
-    Handle<JSObject> object = Handle<JSObject>::cast(receiver);
-    ElementsAccessor* accessor = object->GetElementsAccessor();
-    DCHECK_LT(key->Number(), kMaxUInt32);
-    if (accessor->HasElement(object, key->Number(), ONLY_ENUMERABLE)) {
-      return name;
+// This is a slight modifcation of JSReceiver::HasProperty, dealing with
+// the oddities of JSProxy in for-in filter.
+MaybeHandle<Object> HasEnumerableProperty(Isolate* isolate,
+                                          Handle<JSReceiver> receiver,
+                                          Handle<Object> key) {
+  bool success = false;
+  Maybe<PropertyAttributes> result = Just(ABSENT);
+  LookupIterator it =
+      LookupIterator::PropertyOrElement(isolate, receiver, key, &success);
+  if (!success) return isolate->factory()->undefined_value();
+  for (; it.IsFound(); it.Next()) {
+    switch (it.state()) {
+      case LookupIterator::NOT_FOUND:
+      case LookupIterator::TRANSITION:
+        UNREACHABLE();
+      case LookupIterator::JSPROXY: {
+        // For proxies we have to invoke the [[GetOwnProperty]] trap.
+        result = JSProxy::GetPropertyAttributes(&it);
+        if (result.IsNothing()) return MaybeHandle<Object>();
+        if (result.FromJust() == ABSENT) {
+          // Continue lookup on the proxy's prototype.
+          Handle<JSProxy> proxy = it.GetHolder<JSProxy>();
+          Handle<Object> prototype;
+          ASSIGN_RETURN_ON_EXCEPTION(isolate, prototype,
+                                     JSProxy::GetPrototype(proxy), Object);
+          if (prototype->IsNull()) break;
+          // We already have a stack-check in JSProxy::GetPrototype.
+          return HasEnumerableProperty(
+              isolate, Handle<JSReceiver>::cast(prototype), key);
+        } else if (result.FromJust() & DONT_ENUM) {
+          return isolate->factory()->undefined_value();
+        } else {
+          return it.GetName();
+        }
+      }
+      case LookupIterator::INTERCEPTOR: {
+        result = JSObject::GetPropertyAttributesWithInterceptor(&it);
+        if (result.IsNothing()) return MaybeHandle<Object>();
+        if (result.FromJust() != ABSENT) return it.GetName();
+        continue;
+      }
+      case LookupIterator::ACCESS_CHECK: {
+        if (it.HasAccess()) continue;
+        result = JSObject::GetPropertyAttributesWithFailedAccessCheck(&it);
+        if (result.IsNothing()) return MaybeHandle<Object>();
+        if (result.FromJust() != ABSENT) return it.GetName();
+        return isolate->factory()->undefined_value();
+      }
+      case LookupIterator::INTEGER_INDEXED_EXOTIC:
+        // TypedArray out-of-bounds access.
+        return isolate->factory()->undefined_value();
+      case LookupIterator::ACCESSOR:
+      case LookupIterator::DATA:
+        return it.GetName();
     }
   }
-  Maybe<bool> result = JSReceiver::HasProperty(receiver, name);
-  MAYBE_RETURN_NULL(result);
-  if (result.FromJust()) return name;
   return isolate->factory()->undefined_value();
 }
 
+MaybeHandle<Object> Filter(Handle<JSReceiver> receiver, Handle<Object> key) {
+  Isolate* const isolate = receiver->GetIsolate();
+  return HasEnumerableProperty(isolate, receiver, key);
+}
+
 }  // namespace
 
 

test/mjsunit/es6/proxies-for.js

@@ -123,16 +123,96 @@ TestForInThrow(new Proxy({}, {
   }
 }));
 
-(function() {
-  var p = new Proxy({}, {ownKeys:function() { return ["0"]; }});
+
+function keys(object) {
+  var keys = [];
+  for (var k in object) {
+    keys.push(k);
+  }
+  return keys;
+}
+
+(function testKeysProxyOnProtoEmpty() {
+  var p = new Proxy({}, {
+    ownKeys() { return []; },
+  });
   var o = [0];
   o.__proto__ = p;
-  var keys = [];
-  for (var k in o) { keys.push(k); };
-  assertEquals(["0"], keys);
+  assertEquals(["0"], keys(o));
+
+  delete o[0];
+  assertEquals([], keys(o));
 })();
 
+(function testKeysProxyOnProto() {
+  var handler = {ownKeys() { return ["0"]; }};
+  var proxy = new Proxy({}, handler);
+  var object = [0];
+  object.__proto__ = proxy;
+  assertEquals(["0"], keys(object));
+
+  // The Proxy doesn't set his ownKeys enumerable.
+  delete object[0];
+  assertEquals([], keys(object));
+
+  // The [[Has]] trap has no influence on which are enumerable properties are
+  // shown in for-in.
+  handler.has = function() { return true };
+  assertEquals([], keys(object));
+
+  handler.getOwnPropertyDescriptor = function() {
+    return {enumerable: true, configurable: true}
+  }
+  assertEquals(["0"], keys(object));
+})();
+
+(function testKeysProxyProto() {
+  var target = {t1:true, t2:true};
+  var handler = {};
+  var proxy = new Proxy(target, handler);
+
+  assertEquals(["t1", "t2"], keys(proxy));
+
+  target.__proto__ = {p1:true, p2:true};
+  assertEquals(["t1", "t2", "p1", "p2"], keys(proxy));
+
+  handler.getPrototypeOf = function(target) {
+    return {p3:true, p4:true};
+  };
+  // for-in walks the prototype chain for the [[Has]] / Enumerable check.
+  assertEquals(["t1", "t2", "p3", "p4"], keys(proxy));
+
+  // [[Has]] is not used in for-in.
+  handler.has = function() { return false };
+  assertEquals(["t1", "t2", "p3", "p4"], keys(proxy));
+
+  // Proxy intercepts enumerability check.
+  handler.getOwnPropertyDescriptor = function() {
+    return {enumerable: false, configurable: true}
+  }
+  assertEquals([], keys(proxy));
+
+  handler.getOwnPropertyDescriptor = function() {
+    return {enumerable: true, configurable: true}
+  }
+  assertEquals(["t1", "t2", "p3", "p4"], keys(proxy));
+
+  handler.getOwnPropertyDescriptor = function(target, key) {
+    return {
+      enumerable: key in target,
+      configurable: true
+    }
+  }
+  assertEquals(["t1", "t2"], keys(proxy));
+
+  handler.getPrototypeOf = function() { throw "error" };
+  assertThrowsEquals(() => {keys(proxy)}, "error");
+})();
+
+
 (function () {
-  var p = new Proxy({}, {ownKeys: function() { return ["1", Symbol(), "2"] }});
+  var symbol = Symbol();
+  var p = new Proxy({}, {ownKeys() { return ["1", symbol, "2"] }});
   assertEquals(["1","2"], Object.getOwnPropertyNames(p));
+  assertEquals([symbol], Object.getOwnPropertySymbols(p));
 })();

test/mjsunit/for-in-opt.js

@@ -23,9 +23,9 @@ assertEquals(["0","1","2"], f("bla"));
 
 // Test the lazy deopt points.
 var keys = ["a", "b", "c", "d"];
-var has_keys = [];
-var deopt_has = false;
+var property_descriptor_keys = [];
 var deopt_enum = false;
+var deopt_property_descriptor = false;
 
 var handler = {
   ownKeys() {
@@ -35,16 +35,14 @@ var handler = {
     }
     return keys;
   },
-  getOwnPropertyDescriptor() { return { enumerable: true, configurable: true }},
-
-  has(target, k) {
-    if (deopt_has) {
+  getOwnPropertyDescriptor(target, k) {
+    if (deopt_property_descriptor) {
       %DeoptimizeFunction(f2);
-      deopt_has = false;
+      deopt_property_descriptor = false;
     }
-    has_keys.push(k);
-    return true;
-  }
+    property_descriptor_keys.push(k);
+    return { enumerable: true, configurable: true }
+  },
 };
 
 
@@ -61,8 +59,8 @@ function f2(o) {
 
 function check_f2() {
   assertEquals(keys, f2(o));
-  assertEquals(keys, has_keys);
-  has_keys.length = 0;
+  assertEquals(keys, property_descriptor_keys);
+  property_descriptor_keys.length = 0;
 }
 
 check_f2();
@@ -75,9 +73,10 @@ check_f2();
 
 // Test lazy deopt after FILTER_KEY
 %OptimizeFunctionOnNextCall(f2);
-deopt_has = true;
+deopt_property_descriptor = true;
 check_f2();
 
+
 function f3(o) {
   for (var i in o) {
   }
@@ -91,14 +90,6 @@ f3(undefined);
 f3(null);
 
 // Reliable repro for an issue previously flushed out by GC stress.
-var handler2 = {
-  getPropertyDescriptor(target, k) {
-    has_keys.push(k);
-    return {value: 10, configurable: true, writable: false, enumerable: true};
-  }
-}
-var proxy2 = new Proxy({}, handler2);
-var o2 = {__proto__: proxy2};
 var p = {x: "x"}
 
 function f4(o, p) {
@@ -112,8 +103,8 @@ function f4(o, p) {
 
 function check_f4() {
   assertEquals(keys, f4(o, p));
-  assertEquals(keys, has_keys);
-  has_keys.length = 0;
+  assertEquals(keys, property_descriptor_keys);
+  property_descriptor_keys.length = 0;
 }
 
 check_f4();
@@ -138,12 +129,10 @@ function listener(event, exec_state, event_data, data) {
 
 var handler3 = {
   ownKeys() { return ["a", "b"] },
-  getOwnPropertyDescriptor() { return { enumerable: true, configurable: true }},
-
-  has(target, k) {
+  getOwnPropertyDescriptor(target, k) {
     if (k == "a") count++;
-    if (x) %ScheduleBreak();
-    return true;
+    if (x) %ScheduleBreak()
+    return { enumerable: true, configurable: true }
   }
 };