deps: V8: backport 088b7112e7ab

Original commit message:

    [runtime] Fix contextual stores to global with interceptor

    According to the spec, contextual store in strict mode must first
    check whether property exists and if not, the ReferenceError
    should be thrown instead of calling the interceptor setter. See
    https://tc39.es/ecma262/#sec-object-environment-records-setmutablebinding-n-v-s

    Drive-by:
     - introduce new Api v8::Object::GetPropertyAttributes(..) which is
       able to return "property does not exist" result, which wasn't
       possible with the existing GetPropertyAttributes(..) Api,
     - update GenericInterceptor* callbacks in test-api-interceptors.cc
       to better suite for implementing a proxy-like interceptor.

    Bug: 455600234
    Change-Id: I0986c18c406844f58c453e7aa7513c52a9097e04
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/7718821
    Reviewed-by: Leszek Swirski <[email protected]>
    Commit-Queue: Igor Sheludko <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#106322}

Refs: v8/v8@088b711
Co-authored-by: Michaël Zasso <[email protected]>
PR-URL: #61898
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>

Author: isheludko

common.gypi

@@ -40,7 +40,7 @@
 
     # Reset this number to 0 on major V8 upgrades.
     # Increment by one for each non-official patch applied to deps/v8.
-    'v8_embedder_string': '-node.8',
+    'v8_embedder_string': '-node.9',
 
     ##### V8 defaults for Node.js #####
 

deps/v8/include/v8-object.h

@@ -340,9 +340,24 @@ class V8_EXPORT Object : public Value {
    * Gets the property attributes of a property which can be None or
    * any combination of ReadOnly, DontEnum and DontDelete. Returns
    * None when the property doesn't exist.
+   *
+   * This method will be deprecated soon, since it doesn't provide a way
+   * to return "property does not exist" result. Use GetPropertyAttributes with
+   * PropertyAttribute* instead.
    */
   V8_WARN_UNUSED_RESULT Maybe<PropertyAttribute> GetPropertyAttributes(
       Local<Context> context, Local<Value> key);
+  /**
+   * Gets the property attributes of a property which can be None or
+   * any combination of ReadOnly, DontEnum and DontDelete.
+   *
+   * Returns true and sets *out_attributes if the property exists, false if
+   * not or empty Maybe if an exception is thrown. In the latter two cases,
+   * the value of *out_attributes is not modified.
+   */
+  V8_WARN_UNUSED_RESULT Maybe<bool> GetPropertyAttributes(
+      Local<Context> context, Local<Value> key,
+      PropertyAttribute* out_attributes);
 
   /**
    * Implements Object.getOwnPropertyDescriptor(O, P), see

deps/v8/src/api/api.cc

@@ -4831,6 +4831,16 @@ MaybeLocal<Value> v8::Object::GetPrivate(Local<Context> context,
 
 Maybe<PropertyAttribute> v8::Object::GetPropertyAttributes(
     Local<Context> context, Local<Value> key) {
+  PropertyAttribute attributes = PropertyAttribute::None;
+  auto result = GetPropertyAttributes(context, key, &attributes);
+  if (result.IsNothing()) return {};
+  // This will confusingly return None when the property doesn't exist.
+  return Just(attributes);
+}
+
+Maybe<bool> v8::Object::GetPropertyAttributes(
+    Local<Context> context, Local<Value> key,
+    PropertyAttribute* out_attributes) {
   auto i_isolate = i::Isolate::Current();
   EnterV8Scope<> api_scope{i_isolate, context,
                            RCCId::kAPI_Object_GetPropertyAttributes};
@@ -4843,9 +4853,10 @@ Maybe<PropertyAttribute> v8::Object::GetPropertyAttributes(
   auto result = i::JSReceiver::GetPropertyAttributes(i_isolate, self, key_name);
   if (result.IsNothing()) return {};
   if (result.FromJust() == i::ABSENT) {
-    return Just(static_cast<PropertyAttribute>(i::NONE));
+    return Just(false);
   }
-  return Just(static_cast<PropertyAttribute>(result.FromJust()));
+  *out_attributes = static_cast<PropertyAttribute>(result.FromJust());
+  return Just(true);
 }
 
 MaybeLocal<Value> v8::Object::GetOwnPropertyDescriptor(Local<Context> context,

deps/v8/src/ic/ic.cc

@@ -1899,6 +1899,8 @@ bool StoreIC::LookupForWrite(LookupIterator* it, DirectHandle<Object> value,
       case LookupIterator::NOT_FOUND:
         // If we are in StoreGlobal then check if we should throw on
         // non-existent properties.
+        // TODO(ishell): make store global ic kind reliable, currently it is
+        // not if the feedback vector is not available.
         if (IsStoreGlobalIC() &&
             (GetShouldThrow(it->isolate(), Nothing<ShouldThrow>()) ==
              ShouldThrow::kThrowOnError)) {
@@ -2170,6 +2172,8 @@ MaybeDirectHandle<Object> StoreIC::Store(Handle<JSAny> object,
                                 Nothing<ShouldThrow>(), store_origin));
     }
   } else {
+    // TODO(ishell): deduce should throw from IC kind once it's reliable,
+    // currently it is not if the feedback vector is not available.
     MAYBE_RETURN_NULL(Object::SetProperty(&it, value, store_origin));
   }
   return value;
@@ -2252,6 +2256,20 @@ MaybeObjectHandle StoreIC::ComputeHandler(LookupIterator* lookup) {
                                          isolate());
 
       if (lookup->HolderIsReceiverOrHiddenPrototype()) {
+        // TODO(ishell): make store global ic kind reliable, currently it is
+        // not if the feedback vector is not available.
+        if (IsStoreGlobalIC() &&
+            (GetShouldThrow(isolate(), Nothing<ShouldThrow>()) ==
+             ShouldThrow::kThrowOnError)) {
+          DCHECK(IsJSGlobalObject(*lookup->GetHolder<Object>()));
+          // This is a contextual store to an interceptor object. Use slow
+          // handler because before calling the setter callback we need to
+          // check whether the property exists. See
+          // https://tc39.es/ecma262/#sec-object-environment-records-setmutablebinding-n-v-s
+          Handle<Smi> smi_handler = StoreHandler::StoreSlow(isolate());
+          return MaybeObjectHandle(smi_handler);
+        }
+
         if (info->has_setter() && !IsAnyDefineOwn()) {
           TRACE_HANDLER_STATS(isolate(), StoreIC_StoreInterceptorDH);
           if (info->non_masking()) {

deps/v8/src/objects/objects.cc

@@ -2385,6 +2385,28 @@ Maybe<bool> Object::SetPropertyInternal(LookupIterator* it,
 
       case LookupIterator::INTERCEPTOR: {
         if (it->HolderIsReceiverOrHiddenPrototype()) {
+          // In case we are executing contextual store to a global object with
+          // an interceptor in strict mode, we need to check that the property
+          // actually exists before calling the setter. See
+          // https://tc39.es/ecma262/#sec-object-environment-records-setmutablebinding-n-v-s
+          if (IsJSGlobalObject(*it->GetReceiver())) {
+            Isolate* isolate = it->isolate();
+            auto should_throw_value = GetShouldThrow(isolate, should_throw);
+            should_throw = Just(should_throw_value);
+            if (should_throw_value == kThrowOnError) {
+              Maybe<PropertyAttributes> maybe_attributes =
+                  JSObject::GetPropertyAttributesWithInterceptor(it);
+              if (maybe_attributes.IsNothing()) return Nothing<bool>();
+              if ((maybe_attributes.FromJust() & READ_ONLY) != 0) {
+                return WriteToReadOnlyProperty(it, value, should_throw);
+              }
+              if (maybe_attributes.FromJust() == ABSENT) {
+                // Interceptor doesn't have the property, continue lookup.
+                continue;
+              }
+            }
+          }
+
           InterceptorResult result;
           if (!JSObject::SetPropertyWithInterceptor(it, should_throw, value)
                    .To(&result)) {

deps/v8/test/cctest/test-api-interceptors.cc

@@ -201,7 +201,7 @@ v8::Intercepted GenericInterceptorGetter(
     Local<String> name = generic_name.As<String>();
     String::Utf8Value utf8(isolate, name);
     char* name_str = *utf8;
-    if (*name_str == '_') return v8::Intercepted::kNo;
+    if (*name_str != '$') return v8::Intercepted::kNo;
     str = String::Concat(isolate, v8_str("_str_"), name);
   }
 
@@ -211,6 +211,35 @@ v8::Intercepted GenericInterceptorGetter(
   return v8::Intercepted::kYes;
 }
 
+v8::Intercepted GenericInterceptorQuery(
+    Local<Name> generic_name,
+    const v8::PropertyCallbackInfo<v8::Integer>& info) {
+  v8::Isolate* isolate = info.GetIsolate();
+  Local<String> str;
+  if (generic_name->IsSymbol()) {
+    Local<Value> name = generic_name.As<Symbol>()->Description(isolate);
+    if (name->IsUndefined()) return v8::Intercepted::kNo;
+    str = String::Concat(isolate, v8_str("_sym_"), name.As<String>());
+  } else {
+    Local<String> name = generic_name.As<String>();
+    String::Utf8Value utf8(isolate, name);
+    char* name_str = *utf8;
+    if (*name_str != '$') return v8::Intercepted::kNo;
+    str = String::Concat(isolate, v8_str("_str_"), name);
+  }
+
+  Local<Object> self = info.HolderV2();
+  v8::PropertyAttribute attributes;
+  bool exists;
+  if (self->GetPropertyAttributes(isolate->GetCurrentContext(), str,
+                                  &attributes)
+          .To(&exists)) {
+    if (!exists) return v8::Intercepted::kNo;
+    info.GetReturnValue().Set(attributes);
+  }
+  return v8::Intercepted::kYes;
+}
+
 v8::Intercepted GenericInterceptorSetter(
     Local<Name> generic_name, Local<Value> value,
     const v8::PropertyCallbackInfo<Boolean>& info) {
@@ -224,7 +253,7 @@ v8::Intercepted GenericInterceptorSetter(
     Local<String> name = generic_name.As<String>();
     String::Utf8Value utf8(info.GetIsolate(), name);
     char* name_str = *utf8;
-    if (*name_str == '_') return v8::Intercepted::kNo;
+    if (*name_str != '$') return v8::Intercepted::kNo;
     str = String::Concat(info.GetIsolate(), v8_str("_str_"), name);
   }
 
@@ -1639,9 +1668,9 @@ void InterceptorLoadICGlobalWithInterceptor(bool masking) {
   ExpectInt32(
       "(function() {"
       "  var f = function(obj) { "
-      "    return obj.foo;"
+      "    return obj.$foo;"
       "  };"
-      "  this._str_foo = 42;"
+      "  this._str_$foo = 42;"
       "  var obj = { __proto__: this };"
       "  for (var i = 0; i < 1500; i++) obj['p' + i] = 0;"
       "  /* Ensure that |obj| is in dictionary mode. */"
@@ -2068,6 +2097,138 @@ THREADED_TEST(EmptyInterceptorVsStoreGlobalICs) {
                      120);
 }
 
+namespace {
+
+void CheckGlobalInterceptorIC(v8::NamedPropertyGetterCallback getter,
+                              v8::NamedPropertySetterCallback setter,
+                              v8::NamedPropertyQueryCallback query,
+                              v8::PropertyHandlerFlags flags,
+                              const char* source, std::optional<int> expected) {
+  v8::Isolate* isolate = CcTest::isolate();
+  v8::HandleScope scope(isolate);
+  v8::Local<v8::ObjectTemplate> templ_global = v8::ObjectTemplate::New(isolate);
+  v8::NamedPropertyHandlerConfiguration config(
+      getter, setter, query, nullptr /* deleter */, nullptr /* enumerator */,
+      nullptr /* definer */, nullptr /* descriptor */, {}, flags);
+  templ_global->SetHandler(config);
+
+  LocalContext context(nullptr, templ_global);
+  i::DirectHandle<i::JSReceiver> global_proxy =
+      v8::Utils::OpenDirectHandle<Object, i::JSReceiver>(context->Global());
+  CHECK(IsJSGlobalProxy(*global_proxy));
+  i::DirectHandle<i::JSGlobalObject> global(
+      i::Cast<i::JSGlobalObject>(global_proxy->map()->prototype()),
+      CcTest::i_isolate());
+  CHECK(global->map()->has_named_interceptor());
+
+  v8::Local<Value> value = CompileRun(source);
+  if (expected) {
+    CHECK_EQ(*expected, value->Int32Value(context.local()).FromJust());
+  } else {
+    CHECK(value.IsEmpty());
+  }
+}
+
+void StoreGlobalICWithGlobalInterceptor(bool masking) {
+  v8::PropertyHandlerFlags flags = v8::PropertyHandlerFlags::kNone;
+  if (!masking) {
+    flags = v8::PropertyHandlerFlags::kNonMasking;
+  }
+
+  // In sloppy mode storing to global must succeed.
+  CheckGlobalInterceptorIC(      //
+      GenericInterceptorGetter,  //
+      GenericInterceptorSetter,  //
+      GenericInterceptorQuery,   //
+      flags,                     //
+      R"(
+        // Make interceptor behave like it has a read-only property "$y".
+        Object.defineProperty(globalThis, '_str_$y',
+                              {value: 153, writable: false});
+
+        let result = 0;
+
+        result += (typeof($x) === 'undefined' ? 0 : 10000);
+        result += ($y === 153                 ? 0 : 10000);
+
+        for (var i = 0; i < 20; i++) {
+          try {
+            $x = i;  // not intercepted, absent variable
+            result++;
+          } catch (e) {
+          }
+        }
+        for (var i = 0; i < 20; i++) {
+          try {
+            $y = i;  // intercepted, read only property
+            result++;
+          } catch (e) {
+          }
+        }
+
+        // Check contextual stores succeeded.
+        result += ($x === 19  ? 0 : 100);
+        result += ($y === 153 ? 0 : 1000);
+
+        result
+      )",
+      40);
+
+  // In strict mode storing to global must throw.
+  CheckGlobalInterceptorIC(      //
+      GenericInterceptorGetter,  //
+      GenericInterceptorSetter,  //
+      GenericInterceptorQuery,   //
+      flags,
+      R"(
+        'use strict';
+
+        // Make interceptor behave like it has a read-only property "$y".
+        Object.defineProperty(globalThis, '_str_$y',
+                              {value: 153, writable: false});
+
+        let result = 0;
+
+        result += (typeof($x) === 'undefined' ? 0 : 10000);
+        result += ($y === 153                 ? 0 : 10000);
+
+        for (var i = 0; i < 20; i++) {
+          try {
+            $x = i;  // not intercepted, absent variable
+          } catch (e) {
+            result++;
+          }
+        }
+        for (var i = 0; i < 20; i++) {
+          try {
+            $y = i;  // intercepted, read only property
+          } catch (e) {
+            result++;
+          }
+        }
+
+        // Check contextual stores did not happen.
+        result += (typeof($x) === 'undefined' ? 0 : 100);
+        result += ($y === 153                 ? 0 : 1000);
+
+        result
+      )",
+      40);
+}
+
+}  // namespace
+
+// Checks correctness of global objects with interceptor vs contextual stores.
+THREADED_TEST(StoreGlobalICWithGlobalNonMaskingInterceptor) {
+  const bool masking = false;
+  StoreGlobalICWithGlobalInterceptor(masking);
+}
+
+THREADED_TEST(StoreGlobalICWithGlobalMaskingInterceptor) {
+  const bool masking = true;
+  StoreGlobalICWithGlobalInterceptor(masking);
+}
+
 THREADED_TEST(LegacyInterceptorDoesNotSeeSymbols) {
   LocalContext env;
   v8::Isolate* isolate = CcTest::isolate();
@@ -2120,9 +2281,9 @@ THREADED_TEST(GenericInterceptorDoesSeeSymbols) {
   ExpectInt32("child._sym_age", 10);
 
   // Check that it also sees strings.
-  CompileRun("child.foo = 47");
-  ExpectInt32("child.foo", 47);
-  ExpectInt32("child._str_foo", 47);
+  CompileRun("child.$foo = 47");
+  ExpectInt32("child.$foo", 47);
+  ExpectInt32("child._str_$foo", 47);
 
   // Check that the interceptor can punt (in this case, on anonymous symbols).
   CompileRun("child[anon] = 31337");