"in" operator in VM uses [[Get]] instead of [[GetOwnProperty]]

[TimothyGu]

• Version: master
• Platform: all
• Subsystem: vm

const globals = {};
const handlers = {};
const realHandlers = Reflect.ownKeys(Reflect).reduce((handlers, p) => {
  handlers[p] = (t, ...args) => {
    // Avoid printing the Receiver argument, which can lead to an infinite loop.
    console.log(p, ...(p === 'get' || p === 'set' ? args.slice(0, -1) : args));
    return Reflect[p](t, ...args);
  };
  return handlers;
}, {});
const proxy = vm.createContext(new Proxy(globals, handlers));

// Indirection needed to mitigate against #17465
// https://github.com/nodejs/node/issues/17465
const globalProxy = vm.runInContext('this', proxy);
for (const k of Reflect.ownKeys(globalProxy)) {
  Object.defineProperty(globals, k, Object.getOwnPropertyDescriptor(globalProxy, k));
}
Object.assign(handlers, realHandlers);

'a' in proxy;
  // prints "has a"
  // returns false

vm.runInContext('"a" in this', proxy);
  // prints "get a"
  // returns true (because of https://github.com/nodejs/node/issues/17465)

By spec, the in operator uses the object's [[HasProperty]] internal method, which I overrode with a Proxy trap. That's why the bare 'a' in proxy prints has a.

On the other hand, V8 doesn't quite support overriding [[HasProperty]] with the modern version of the NamedPropertyHandlerConfiguration constructor, only

• [[Get]] getter
• [[Set]] setter
• [[GetOwnProperty]] descriptor
• [[Delete]] deleter
• [[OwnPropertyKeys]] enumerator
• [[DefineOwnProperty]] definer

Thus I would expect the default [[HasProperty]] internal method to be used, i.e. OrdinaryHasProperty, which in turns calls the object's [[GetOwnProperty]] internal method. I.e. the descriptor should be called. Yet this isn't the case shown above -- only GenericNamedPropertyGetterCallback is called.

/cc @fhinkel @verwaest

[fhinkel]

Thanks for reporting! I'll have a look.

[TimothyGu]

@fhinkel Thanks. I've got a patch for V8 (and a smaller one for Node.js) that fixes both this issue and #17481. After this patch, vm.runInContext('"a" in this', proxy); prints getOwnPropertyDescriptor a instead, as expected.

Let me know what you think:

Patch

diff --git a/deps/v8/src/objects.cc b/deps/v8/src/objects.cc
index 28c1cd681f..cbc182fe45 100644
--- a/deps/v8/src/objects.cc
+++ b/deps/v8/src/objects.cc
@@ -1798,6 +1798,32 @@ Maybe<PropertyAttributes> GetPropertyAttributesWithInterceptorInternal(
       CHECK(result->ToInt32(&value));
       return Just(static_cast<PropertyAttributes>(value));
     }
+  } else if (!interceptor->descriptor()->IsUndefined(isolate)) {
+    Handle<Object> result;
+    if (it->IsElement()) {
+      uint32_t index = it->index();
+      v8::IndexedPropertyDescriptorCallback descriptorCallback =
+          v8::ToCData<v8::IndexedPropertyDescriptorCallback>(
+              interceptor->descriptor());
+
+      result = args.Call(descriptorCallback, index);
+    } else {
+      Handle<Name> name = it->name();
+      DCHECK(!name->IsPrivate());
+      v8::GenericNamedPropertyDescriptorCallback descriptorCallback =
+          v8::ToCData<v8::GenericNamedPropertyDescriptorCallback>(
+              interceptor->descriptor());
+      result = args.Call(descriptorCallback, name);
+    }
+    if (!result.is_null()) {
+      PropertyDescriptor desc;
+      Utils::ApiCheck(
+          PropertyDescriptor::ToPropertyDescriptor(isolate, result, &desc),
+          it->IsElement() ? "v8::IndexedPropertyDescriptorCallback"
+                          : "v8::NamedPropertyDescriptorCallback",
+          "Invalid property descriptor.");
+      return Just(desc.ToAttributes());
+    }
   } else if (!interceptor->getter()->IsUndefined(isolate)) {
     // TODO(verwaest): Use GetPropertyWithInterceptor?
     Handle<Object> result;
@@ -7444,65 +7470,6 @@ Maybe<bool> JSReceiver::GetOwnPropertyDescriptor(Isolate* isolate,
   return GetOwnPropertyDescriptor(&it, desc);
 }
 
-namespace {
-
-Maybe<bool> GetPropertyDescriptorWithInterceptor(LookupIterator* it,
-                                                 PropertyDescriptor* desc) {
-  bool has_access = true;
-  if (it->state() == LookupIterator::ACCESS_CHECK) {
-    has_access = it->HasAccess() || JSObject::AllCanRead(it);
-    it->Next();
-  }
-
-  if (has_access && it->state() == LookupIterator::INTERCEPTOR) {
-    Isolate* isolate = it->isolate();
-    Handle<InterceptorInfo> interceptor = it->GetInterceptor();
-    if (!interceptor->descriptor()->IsUndefined(isolate)) {
-      Handle<Object> result;
-      Handle<JSObject> holder = it->GetHolder<JSObject>();
-
-      Handle<Object> receiver = it->GetReceiver();
-      if (!receiver->IsJSReceiver()) {
-        ASSIGN_RETURN_ON_EXCEPTION_VALUE(
-            isolate, receiver, Object::ConvertReceiver(isolate, receiver),
-            Nothing<bool>());
-      }
-
-      PropertyCallbackArguments args(isolate, interceptor->data(), *receiver,
-                                     *holder, Object::DONT_THROW);
-      if (it->IsElement()) {
-        uint32_t index = it->index();
-        v8::IndexedPropertyDescriptorCallback descriptorCallback =
-            v8::ToCData<v8::IndexedPropertyDescriptorCallback>(
-                interceptor->descriptor());
-
-        result = args.Call(descriptorCallback, index);
-      } else {
-        Handle<Name> name = it->name();
-        DCHECK(!name->IsPrivate());
-        v8::GenericNamedPropertyDescriptorCallback descriptorCallback =
-            v8::ToCData<v8::GenericNamedPropertyDescriptorCallback>(
-                interceptor->descriptor());
-        result = args.Call(descriptorCallback, name);
-      }
-      if (!result.is_null()) {
-        // Request successfully intercepted, try to set the property
-        // descriptor.
-        Utils::ApiCheck(
-            PropertyDescriptor::ToPropertyDescriptor(isolate, result, desc),
-            it->IsElement() ? "v8::IndexedPropertyDescriptorCallback"
-                            : "v8::NamedPropertyDescriptorCallback",
-            "Invalid property descriptor.");
-
-        return Just(true);
-      }
-    }
-  }
-  it->Restart();
-  return Just(false);
-}
-}  // namespace
-
 // ES6 9.1.5.1
 // Returns true on success, false if the property didn't exist, nothing if
 // an exception was thrown.
@@ -7516,12 +7483,6 @@ Maybe<bool> JSReceiver::GetOwnPropertyDescriptor(LookupIterator* it,
                                              it->GetName(), desc);
   }
 
-  Maybe<bool> intercepted = GetPropertyDescriptorWithInterceptor(it, desc);
-  MAYBE_RETURN(intercepted, Nothing<bool>());
-  if (intercepted.FromJust()) {
-    return Just(true);
-  }
-
   // Request was not intercepted, continue as normal.
   // 1. (Assert)
   // 2. If O does not have an own property with key P, return undefined.
diff --git a/src/node_contextify.cc b/src/node_contextify.cc
index 73c5fe08ab..8451fbe1a9 100644
--- a/src/node_contextify.cc
+++ b/src/node_contextify.cc
@@ -396,10 +396,10 @@ class ContextifyContext {
 
     Local<Object> sandbox = ctx->sandbox();
 
-    if (sandbox->HasOwnProperty(context, property).FromMaybe(false)) {
-      args.GetReturnValue().Set(
-          sandbox->GetOwnPropertyDescriptor(context, property)
-              .ToLocalChecked());
+    auto maybe_desc = sandbox->GetOwnPropertyDescriptor(context, property);
+    Local<Value> desc;
+    if (maybe_desc.ToLocal(&desc) && desc->IsObject()) {
+      args.GetReturnValue().Set(desc);
     }
   }
 

[TimothyGu]

I've submitted a revised version of the patch above as a CL to V8 https://chromium-review.googlesource.com/c/v8/v8/+/816515, which fixes both this bug and #17481.

@fhinkel review requested and appreciated :)

[fhinkel]

Thanks! Landed in https://chromium.googlesource.com/v8/v8/+/d5fbf7c5c3f8f9b46b75f674771f3533c7e3e24d. Let's give it some canary coverage, then we can back port it.

[targos]

The patch was reverted in https://chromium-review.googlesource.com/c/v8/v8/+/850355 because of a performance regression.

[TimothyGu]

Well, actually closed in 85c356c / #22390.