[api] Use query interceptor in Object.keys().

The V8 API provides interceptors. They are not part of the
EcmaScript specification. But their behavior should be consistent.
For example, when an EnumeratorInterceptor is defined, Object.keys(),
Object.entries(), and Object.values() should all have the
same number of entries.

This CL creates consistent behavior among these
functions. If a QueryCallback is present, it is used to
filter the result from the EnumeratorCallback for
enumerable properties.

Bug: v8:6627
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
Change-Id: Ie51e69bb77099d9fafc4b1ea02671eced610edba
Reviewed-on: https://chromium-review.googlesource.com/609068Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Commit-Queue: Franziska Hinkelmann <franzih@chromium.org>
Cr-Commit-Position: refs/heads/master@{#47442}

include/v8.h

@@ -5117,10 +5117,11 @@ typedef void (*NamedPropertyDeleterCallback)(
     Local<String> property,
     const PropertyCallbackInfo<Boolean>& info);
 
-
 /**
  * Returns an array containing the names of the properties the named
  * property getter intercepts.
+ *
+ * Note: The values in the array must be of type v8::Name.
  */
 typedef void (*NamedPropertyEnumeratorCallback)(
     const PropertyCallbackInfo<Array>& info);
@@ -5241,10 +5242,11 @@ typedef void (*GenericNamedPropertyQueryCallback)(
 typedef void (*GenericNamedPropertyDeleterCallback)(
     Local<Name> property, const PropertyCallbackInfo<Boolean>& info);
 
-
 /**
  * Returns an array containing the names of the properties the named
  * property getter intercepts.
+ *
+ * Note: The values in the array must be of type v8::Name.
  */
 typedef void (*GenericNamedPropertyEnumeratorCallback)(
     const PropertyCallbackInfo<Array>& info);
@@ -5325,7 +5327,10 @@ typedef void (*IndexedPropertyDeleterCallback)(
     const PropertyCallbackInfo<Boolean>& info);
 
 /**
- * See `v8::GenericNamedPropertyEnumeratorCallback`.
+ * Returns an array containing the indices of the properties the indexed
+ * property getter intercepts.
+ *
+ * Note: The values in the array must be uint32_t.
  */
 typedef void (*IndexedPropertyEnumeratorCallback)(
     const PropertyCallbackInfo<Array>& info);

src/elements.cc

@@ -615,6 +615,16 @@ class ElementsAccessorBase : public ElementsAccessor {
                                           filter) != kMaxUInt32;
   }
 
+  bool HasEntry(JSObject* holder, uint32_t entry) final {
+    return Subclass::HasEntryImpl(holder->GetIsolate(), holder->elements(),
+                                  entry);
+  }
+
+  static bool HasEntryImpl(Isolate* isolate, FixedArrayBase* backing_store,
+                           uint32_t entry) {
+    UNIMPLEMENTED();
+  }
+
   bool HasAccessors(JSObject* holder) final {
     return Subclass::HasAccessorsImpl(holder, holder->elements());
   }

src/elements.h

@@ -50,6 +50,10 @@ class ElementsAccessor {
     return HasElement(holder, index, holder->elements(), filter);
   }
 
+  // Note: this is currently not implemented for string wrapper and
+  // typed array elements.
+  virtual bool HasEntry(JSObject* holder, uint32_t entry) = 0;
+
   virtual Handle<Object> Get(Handle<JSObject> holder, uint32_t entry) = 0;
 
   virtual PropertyDetails GetDetails(JSObject* holder, uint32_t entry) = 0;

src/keys.cc

@@ -4,7 +4,7 @@
 
 #include "src/keys.h"
 
-#include "src/api-arguments.h"
+#include "src/api-arguments-inl.h"
 #include "src/elements.h"
 #include "src/factory.h"
 #include "src/identity-map.h"
@@ -392,7 +392,7 @@ MaybeHandle<FixedArray> FastKeyAccumulator::GetKeysFast(
     return MaybeHandle<FixedArray>();
   }
 
-  // From this point on we are certiain to only collect own keys.
+  // From this point on we are certain to only collect own keys.
   DCHECK(receiver_->IsJSObject());
   Handle<JSObject> object = Handle<JSObject>::cast(receiver_);
 
@@ -456,6 +456,41 @@ namespace {
 
 enum IndexedOrNamed { kIndexed, kNamed };
 
+template <IndexedOrNamed type>
+void FilterForEnumerableProperties(PropertyCallbackArguments* args,
+                                   Object* query_fun, Handle<JSObject> result,
+                                   KeyAccumulator* accumulator) {
+  DCHECK(result->IsJSArray() || result->HasSloppyArgumentsElements());
+
+  ElementsAccessor* accessor = result->GetElementsAccessor();
+
+  uint32_t length = accessor->GetCapacity(*result, result->elements());
+  for (uint32_t i = 0; i < length; i++) {
+    if (!accessor->HasEntry(*result, i)) continue;
+    Handle<Object> element = accessor->Get(result, i);
+    Handle<Object> attributes;
+    if (type == kIndexed) {
+      uint32_t number;
+      CHECK(element->ToUint32(&number));
+      attributes = args->Call(
+          v8::ToCData<v8::IndexedPropertyQueryCallback>(query_fun), number);
+    } else {
+      CHECK(element->IsName());
+      attributes = args->Call(
+          v8::ToCData<v8::GenericNamedPropertyQueryCallback>(query_fun),
+          Handle<Name>::cast(element));
+    }
+
+    if (!attributes.is_null()) {
+      int32_t value;
+      CHECK(attributes->ToInt32(&value));
+      if ((value & DONT_ENUM) == 0) {
+        accumulator->AddKey(element, DO_NOT_CONVERT);
+      }
+    }
+  }
+}
+
 // Returns |true| on success, |nothing| on exception.
 template <class Callback, IndexedOrNamed type>
 Maybe<bool> CollectInterceptorKeysInternal(Handle<JSReceiver> receiver,
@@ -463,20 +498,30 @@ Maybe<bool> CollectInterceptorKeysInternal(Handle<JSReceiver> receiver,
                                            Handle<InterceptorInfo> interceptor,
                                            KeyAccumulator* accumulator) {
   Isolate* isolate = accumulator->isolate();
-  PropertyCallbackArguments args(isolate, interceptor->data(), *receiver,
-                                 *object, Object::DONT_THROW);
+  PropertyCallbackArguments enum_args(isolate, interceptor->data(), *receiver,
+                                      *object, Object::DONT_THROW);
   Handle<JSObject> result;
   if (!interceptor->enumerator()->IsUndefined(isolate)) {
     Callback enum_fun = v8::ToCData<Callback>(interceptor->enumerator());
     const char* log_tag = type == kIndexed ? "interceptor-indexed-enum"
                                            : "interceptor-named-enum";
     LOG(isolate, ApiObjectAccess(log_tag, *object));
-    result = args.Call(enum_fun);
+    result = enum_args.Call(enum_fun);
   }
   RETURN_VALUE_IF_SCHEDULED_EXCEPTION(isolate, Nothing<bool>());
   if (result.is_null()) return Just(true);
-  accumulator->AddKeys(
-      result, type == kIndexed ? CONVERT_TO_ARRAY_INDEX : DO_NOT_CONVERT);
+
+  if ((accumulator->filter() & ONLY_ENUMERABLE) &&
+      !interceptor->query()->IsUndefined(isolate)) {
+    PropertyCallbackArguments query_args(
+        isolate, interceptor->data(), *receiver, *object, Object::DONT_THROW);
+
+    FilterForEnumerableProperties<type>(&query_args, interceptor->query(),
+                                        result, accumulator);
+  } else {
+    accumulator->AddKeys(
+        result, type == kIndexed ? CONVERT_TO_ARRAY_INDEX : DO_NOT_CONVERT);
+  }
   return Just(true);
 }
 

test/cctest/test-api-interceptors.cc

@@ -3326,12 +3326,18 @@ static void NamedEnum(const v8::PropertyCallbackInfo<v8::Array>& info) {
   ApiTestFuzzer::Fuzz();
   v8::Local<v8::Array> result = v8::Array::New(info.GetIsolate(), 3);
   v8::Local<v8::Context> context = info.GetIsolate()->GetCurrentContext();
-  result->Set(context, v8::Integer::New(info.GetIsolate(), 0), v8_str("foo"))
-      .FromJust();
-  result->Set(context, v8::Integer::New(info.GetIsolate(), 1), v8_str("bar"))
-      .FromJust();
-  result->Set(context, v8::Integer::New(info.GetIsolate(), 2), v8_str("baz"))
-      .FromJust();
+  CHECK(
+      result
+          ->Set(context, v8::Integer::New(info.GetIsolate(), 0), v8_str("foo"))
+          .FromJust());
+  CHECK(
+      result
+          ->Set(context, v8::Integer::New(info.GetIsolate(), 1), v8_str("bar"))
+          .FromJust());
+  CHECK(
+      result
+          ->Set(context, v8::Integer::New(info.GetIsolate(), 2), v8_str("baz"))
+          .FromJust());
   info.GetReturnValue().Set(result);
 }
 
@@ -3340,10 +3346,12 @@ static void IndexedEnum(const v8::PropertyCallbackInfo<v8::Array>& info) {
   ApiTestFuzzer::Fuzz();
   v8::Local<v8::Array> result = v8::Array::New(info.GetIsolate(), 2);
   v8::Local<v8::Context> context = info.GetIsolate()->GetCurrentContext();
-  result->Set(context, v8::Integer::New(info.GetIsolate(), 0), v8_str("0"))
-      .FromJust();
-  result->Set(context, v8::Integer::New(info.GetIsolate(), 1), v8_str("1"))
-      .FromJust();
+  CHECK(
+      result->Set(context, v8::Integer::New(info.GetIsolate(), 0), v8_str("0"))
+          .FromJust());
+  CHECK(
+      result->Set(context, v8::Integer::New(info.GetIsolate(), 1), v8_str("1"))
+          .FromJust());
   info.GetReturnValue().Set(result);
 }
 
@@ -4964,6 +4972,194 @@ THREADED_TEST(NonMaskingInterceptorPrototypePropertyIC) {
   ExpectInt32("f(outer)", 4);
 }
 
+namespace {
+
+void ConcatNamedPropertyGetter(
+    Local<Name> name, const v8::PropertyCallbackInfo<v8::Value>& info) {
+  info.GetReturnValue().Set(
+      // Return the property name concatenated with itself.
+      String::Concat(name.As<String>(), name.As<String>()));
+}
+
+void ConcatIndexedPropertyGetter(
+    uint32_t index, const v8::PropertyCallbackInfo<v8::Value>& info) {
+  info.GetReturnValue().Set(
+      // Return the double value of the index.
+      v8_num(index + index));
+}
+
+void EnumCallbackWithNames(const v8::PropertyCallbackInfo<v8::Array>& info) {
+  ApiTestFuzzer::Fuzz();
+  v8::Local<v8::Array> result = v8::Array::New(info.GetIsolate(), 4);
+  v8::Local<v8::Context> context = info.GetIsolate()->GetCurrentContext();
+  CHECK(
+      result
+          ->Set(context, v8::Integer::New(info.GetIsolate(), 0), v8_str("foo"))
+          .FromJust());
+  CHECK(
+      result
+          ->Set(context, v8::Integer::New(info.GetIsolate(), 1), v8_str("bar"))
+          .FromJust());
+  CHECK(
+      result
+          ->Set(context, v8::Integer::New(info.GetIsolate(), 2), v8_str("baz"))
+          .FromJust());
+  CHECK(
+      result->Set(context, v8::Integer::New(info.GetIsolate(), 3), v8_str("10"))
+          .FromJust());
+
+  //  Create a holey array.
+  CHECK(result->Delete(context, v8::Integer::New(info.GetIsolate(), 1))
+            .FromJust());
+  info.GetReturnValue().Set(result);
+}
+
+void EnumCallbackWithIndices(const v8::PropertyCallbackInfo<v8::Array>& info) {
+  ApiTestFuzzer::Fuzz();
+  v8::Local<v8::Array> result = v8::Array::New(info.GetIsolate(), 4);
+  v8::Local<v8::Context> context = info.GetIsolate()->GetCurrentContext();
+
+  CHECK(result->Set(context, v8::Integer::New(info.GetIsolate(), 0), v8_num(10))
+            .FromJust());
+  CHECK(result->Set(context, v8::Integer::New(info.GetIsolate(), 1), v8_num(11))
+            .FromJust());
+  CHECK(result->Set(context, v8::Integer::New(info.GetIsolate(), 2), v8_num(12))
+            .FromJust());
+  CHECK(result->Set(context, v8::Integer::New(info.GetIsolate(), 3), v8_num(14))
+            .FromJust());
+
+  //  Create a holey array.
+  CHECK(result->Delete(context, v8::Integer::New(info.GetIsolate(), 1))
+            .FromJust());
+  info.GetReturnValue().Set(result);
+}
+
+void RestrictiveNamedQuery(Local<Name> property,
+                           const v8::PropertyCallbackInfo<v8::Integer>& info) {
+  // Only "foo" is enumerable.
+  if (v8_str("foo")
+          ->Equals(info.GetIsolate()->GetCurrentContext(), property)
+          .FromJust()) {
+    info.GetReturnValue().Set(v8::PropertyAttribute::None);
+    return;
+  }
+  info.GetReturnValue().Set(v8::PropertyAttribute::DontEnum);
+}
+
+void RestrictiveIndexedQuery(
+    uint32_t index, const v8::PropertyCallbackInfo<v8::Integer>& info) {
+  // Only index 2 and 12 are enumerable.
+  if (index == 2 || index == 12) {
+    info.GetReturnValue().Set(v8::PropertyAttribute::None);
+    return;
+  }
+  info.GetReturnValue().Set(v8::PropertyAttribute::DontEnum);
+}
+}  // namespace
+
+// Regression test for V8 bug 6627.
+// Object.keys() must return enumerable keys only.
+THREADED_TEST(EnumeratorsAndUnenumerableNamedProperties) {
+  // The enumerator interceptor returns a list
+  // of items which are filtered according to the
+  // properties defined in the query interceptor.
+  v8::Isolate* isolate = CcTest::isolate();
+  v8::HandleScope scope(isolate);
+  v8::Local<v8::ObjectTemplate> obj = ObjectTemplate::New(isolate);
+  obj->SetHandler(v8::NamedPropertyHandlerConfiguration(
+      ConcatNamedPropertyGetter, NULL, RestrictiveNamedQuery, NULL,
+      EnumCallbackWithNames));
+  LocalContext context;
+  context->Global()
+      ->Set(context.local(), v8_str("obj"),
+            obj->NewInstance(context.local()).ToLocalChecked())
+      .FromJust();
+
+  ExpectInt32("Object.getOwnPropertyNames(obj).length", 3);
+  ExpectString("Object.getOwnPropertyNames(obj)[0]", "foo");
+  ExpectString("Object.getOwnPropertyNames(obj)[1]", "baz");
+  ExpectString("Object.getOwnPropertyNames(obj)[2]", "10");
+
+  ExpectTrue("Object.getOwnPropertyDescriptor(obj, 'foo').enumerable");
+  ExpectFalse("Object.getOwnPropertyDescriptor(obj, 'baz').enumerable");
+
+  ExpectInt32("Object.entries(obj).length", 1);
+  ExpectString("Object.entries(obj)[0][0]", "foo");
+  ExpectString("Object.entries(obj)[0][1]", "foofoo");
+
+  ExpectInt32("Object.keys(obj).length", 1);
+  ExpectString("Object.keys(obj)[0]", "foo");
+
+  ExpectInt32("Object.values(obj).length", 1);
+  ExpectString("Object.values(obj)[0]", "foofoo");
+}
+
+THREADED_TEST(EnumeratorsAndUnenumerableIndexedPropertiesArgumentsElements) {
+  v8::Isolate* isolate = CcTest::isolate();
+  v8::HandleScope scope(isolate);
+  v8::Local<v8::ObjectTemplate> obj = ObjectTemplate::New(isolate);
+  obj->SetHandler(v8::IndexedPropertyHandlerConfiguration(
+      ConcatIndexedPropertyGetter, NULL, RestrictiveIndexedQuery, NULL,
+      SloppyArgsIndexedPropertyEnumerator));
+  LocalContext context;
+  context->Global()
+      ->Set(context.local(), v8_str("obj"),
+            obj->NewInstance(context.local()).ToLocalChecked())
+      .FromJust();
+
+  ExpectInt32("Object.getOwnPropertyNames(obj).length", 4);
+  ExpectString("Object.getOwnPropertyNames(obj)[0]", "0");
+  ExpectString("Object.getOwnPropertyNames(obj)[1]", "1");
+  ExpectString("Object.getOwnPropertyNames(obj)[2]", "2");
+  ExpectString("Object.getOwnPropertyNames(obj)[3]", "3");
+
+  ExpectTrue("Object.getOwnPropertyDescriptor(obj, '2').enumerable");
+
+  ExpectInt32("Object.entries(obj).length", 1);
+  ExpectString("Object.entries(obj)[0][0]", "2");
+  ExpectInt32("Object.entries(obj)[0][1]", 4);
+
+  ExpectInt32("Object.keys(obj).length", 1);
+  ExpectString("Object.keys(obj)[0]", "2");
+
+  ExpectInt32("Object.values(obj).length", 1);
+  ExpectInt32("Object.values(obj)[0]", 4);
+}
+
+THREADED_TEST(EnumeratorsAndUnenumerableIndexedProperties) {
+  // The enumerator interceptor returns a list
+  // of items which are filtered according to the
+  // properties defined in the query interceptor.
+  v8::Isolate* isolate = CcTest::isolate();
+  v8::HandleScope scope(isolate);
+  v8::Local<v8::ObjectTemplate> obj = ObjectTemplate::New(isolate);
+  obj->SetHandler(v8::IndexedPropertyHandlerConfiguration(
+      ConcatIndexedPropertyGetter, NULL, RestrictiveIndexedQuery, NULL,
+      EnumCallbackWithIndices));
+  LocalContext context;
+  context->Global()
+      ->Set(context.local(), v8_str("obj"),
+            obj->NewInstance(context.local()).ToLocalChecked())
+      .FromJust();
+
+  ExpectInt32("Object.getOwnPropertyNames(obj).length", 3);
+  ExpectString("Object.getOwnPropertyNames(obj)[0]", "10");
+  ExpectString("Object.getOwnPropertyNames(obj)[1]", "12");
+  ExpectString("Object.getOwnPropertyNames(obj)[2]", "14");
+
+  ExpectFalse("Object.getOwnPropertyDescriptor(obj, '10').enumerable");
+  ExpectTrue("Object.getOwnPropertyDescriptor(obj, '12').enumerable");
+
+  ExpectInt32("Object.entries(obj).length", 1);
+  ExpectString("Object.entries(obj)[0][0]", "12");
+  ExpectInt32("Object.entries(obj)[0][1]", 24);
+
+  ExpectInt32("Object.keys(obj).length", 1);
+  ExpectString("Object.keys(obj)[0]", "12");
+
+  ExpectInt32("Object.values(obj).length", 1);
+  ExpectInt32("Object.values(obj)[0]", 24);
+}
 
 namespace {