[ic] fix handling of existing properties in Define{Keyed|Named}OwnIC

- When the property being defined with DefineKeyedOwnIC or
  DefineNamedOwnIC already exists, we should use the slow path to
  check if the operation is allowed in case the property is
  non-configurable or Object.preventExtensions() has been called on
  the property.
- Since KeyedStoreIC:Store() reuses StoreIC::Store() when the key is a
  name, we should use Runtime::DefineObjectOwnProperty() for
  DefineKeyedOwnIC too.
- When dealing with public fields, Runtime::DefineObjectOwnProperty()
  should use JSReceiver::CreateDataProperty() instead of
  Object::SetProperty() for the specified semantics. This patch also
  adds JSReceiver::AddPrivateField() for it and StoreIC::Store to
  define private fields without triggering traps or checking
  extensibility.
- To emit a more specific error message when redefining properties
  on non-extensible objects, Object::AddDataProperty() now also takes
  a EnforceDefineSemantics enum to distinguish between set and define.
- Drive-by: fix JSReceiver::CheckIfCanDefine() which should check for
  extensibility even if the configurability check passes.

Bug: chromium:1259950, v8:9888
Change-Id: Ib1bc851ffd4b9c3a0e98cac96dafe743c08ee37e
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3517934Reviewed-by: Shu-yu Guo <syg@chromium.org>
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Commit-Queue: Joyee Cheung <joyee@igalia.com>
Cr-Commit-Position: refs/heads/main@{#79603}

src/ic/accessor-assembler.cc

@@ -1261,18 +1261,21 @@ void AccessorAssembler::HandleStoreICHandlerCase(
     TVARIABLE(IntPtrT, var_name_index);
     Label dictionary_found(this, &var_name_index);
     NameDictionaryLookup<PropertyDictionary>(
-        properties, CAST(p->name()), &dictionary_found, &var_name_index, miss);
+        properties, CAST(p->name()),
+        p->IsAnyDefineOwn() ? &if_slow : &dictionary_found, &var_name_index,
+        miss);
+
+    // When dealing with class fields defined with DefineKeyedOwnIC or
+    // DefineNamedOwnIC, use the slow path to check the existing property.
+    if (!p->IsAnyDefineOwn()) {
       BIND(&dictionary_found);
       {
-      if (p->IsDefineKeyedOwn()) {
-        // Take slow path to throw if a private name already exists.
-        GotoIf(IsPrivateSymbol(CAST(p->name())), &if_slow);
-      }
         Label if_constant(this), done(this);
         TNode<Uint32T> details =
             LoadDetailsByKeyIndex(properties, var_name_index.value());
         // Check that the property is a writable data property (no accessor).
-      const int kTypeAndReadOnlyMask = PropertyDetails::KindField::kMask |
+        const int kTypeAndReadOnlyMask =
+            PropertyDetails::KindField::kMask |
             PropertyDetails::kAttributesReadOnlyMask;
         STATIC_ASSERT(static_cast<int>(PropertyKind::kData) == 0);
         GotoIf(IsSetWord32(details, kTypeAndReadOnlyMask), miss);
@@ -1298,7 +1301,7 @@ void AccessorAssembler::HandleStoreICHandlerCase(
           Return(p->value());
         }
       }
-
+    }
     BIND(&if_fast_smi);
     {
       Label data(this), accessor(this), shared_struct_field(this),

src/ic/ic.cc

@@ -1779,7 +1779,8 @@ Maybe<bool> DefineOwnDataProperty(LookupIterator* it,
         }
         case LookupIterator::NOT_FOUND:
           return Object::AddDataProperty(it, value, NONE,
-                                         Nothing<ShouldThrow>(), store_origin);
+                                         Nothing<ShouldThrow>(), store_origin,
+                                         EnforceDefineSemantics::kDefine);
       }
     }
     case LookupIterator::ACCESS_CHECK:
@@ -1796,7 +1797,7 @@ Maybe<bool> DefineOwnDataProperty(LookupIterator* it,
 
   return JSObject::DefineOwnPropertyIgnoreAttributes(
       it, value, NONE, should_throw, JSObject::DONT_FORCE_FIELD,
-      JSObject::EnforceDefineSemantics::kDefine);
+      EnforceDefineSemantics::kDefine, store_origin);
 }
 }  // namespace
 
@@ -1806,10 +1807,15 @@ MaybeHandle<Object> StoreIC::Store(Handle<Object> object, Handle<Name> name,
   // TODO(verwaest): Let SetProperty do the migration, since storing a property
   // might deprecate the current map again, if value does not fit.
   if (MigrateDeprecated(isolate(), object)) {
+    // KeyedStoreIC should handle DefineKeyedOwnIC with deprecated maps directly
+    // instead of reusing this method.
+    DCHECK(!IsDefineKeyedOwnIC());
+    DCHECK(!name->IsPrivateName());
+
     PropertyKey key(isolate(), name);
     LookupIterator it(
         isolate(), object, key,
-        IsAnyDefineOwn() ? LookupIterator::OWN : LookupIterator::DEFAULT);
+        IsDefineNamedOwnIC() ? LookupIterator::OWN : LookupIterator::DEFAULT);
     DCHECK_IMPLIES(IsDefineNamedOwnIC(), it.IsFound() && it.HolderIsReceiver());
     // TODO(v8:12548): refactor DefinedNamedOwnIC and SetNamedIC as subclasses
     // of StoreIC so their logic doesn't get mixed here.
@@ -1867,14 +1873,16 @@ MaybeHandle<Object> StoreIC::Store(Handle<Object> object, Handle<Name> name,
     }
   }
 
-  // For IsDefineNamedOwnIC(), we can't simply do CreateDataProperty below
+  // For IsAnyDefineOwn(), we can't simply do CreateDataProperty below
   // because we need to check the attributes before UpdateCaches updates
   // the state of the LookupIterator.
   LookupIterator::State original_state = it.state();
   // We'll defer the check for JSProxy and objects with named interceptors,
   // because the defineProperty traps need to be called first if they are
-  // present.
-  if (IsDefineNamedOwnIC() && !object->IsJSProxy() &&
+  // present. We can also skip this for private names since they are not
+  // bound by configurability or extensibility checks, and errors would've
+  // been thrown if the private field already exists in the object.
+  if (IsAnyDefineOwn() && !name->IsPrivateName() && !object->IsJSProxy() &&
       !Handle<JSObject>::cast(object)->HasNamedInterceptor()) {
     Maybe<bool> can_define = JSReceiver::CheckIfCanDefine(
         isolate(), &it, value, Nothing<ShouldThrow>());
@@ -1895,12 +1903,17 @@ MaybeHandle<Object> StoreIC::Store(Handle<Object> object, Handle<Name> name,
   // of StoreIC so their logic doesn't get mixed here.
   // ES #sec-definefield
   // ES #sec-runtime-semantics-propertydefinitionevaluation
-  if (IsDefineNamedOwnIC()) {
-    // Private property should be defined via DefineKeyedOwnIC or
-    // KeyedStoreIC with private symbols.
-    DCHECK(!name->IsPrivate());
+  // IsAnyDefineOwn() can be true when this method is reused by KeyedStoreIC.
+  if (IsAnyDefineOwn()) {
+    if (name->IsPrivateName()) {
+      // We should define private fields without triggering traps or checking
+      // extensibility.
+      MAYBE_RETURN_NULL(
+          JSReceiver::AddPrivateField(&it, value, Nothing<ShouldThrow>()));
+    } else {
       MAYBE_RETURN_NULL(DefineOwnDataProperty(
           &it, original_state, value, Nothing<ShouldThrow>(), store_origin));
+    }
   } else {
     MAYBE_RETURN_NULL(Object::SetProperty(&it, value, store_origin));
   }
@@ -1982,9 +1995,9 @@ MaybeObjectHandle StoreIC::ComputeHandler(LookupIterator* lookup) {
       // If the interceptor is on the receiver...
       if (lookup->HolderIsReceiverOrHiddenPrototype() && !info.non_masking()) {
         // ...return a store interceptor Smi handler if there is a setter
-        // interceptor and it's not DefineNamedOwnIC (which should call the
-        // definer)...
-        if (!info.setter().IsUndefined(isolate()) && !IsDefineNamedOwnIC()) {
+        // interceptor and it's not DefineNamedOwnIC or DefineKeyedOwnIC
+        // (which should call the definer)...
+        if (!info.setter().IsUndefined(isolate()) && !IsAnyDefineOwn()) {
           return MaybeObjectHandle(StoreHandler::StoreInterceptor(isolate()));
         }
         // ...otherwise return a slow-case Smi handler, which invokes the
@@ -3141,7 +3154,11 @@ RUNTIME_FUNCTION(Runtime_ElementsTransitionAndStoreIC_Miss) {
     DCHECK(IsKeyedStoreICKind(kind) || IsStoreICKind(kind) ||
            IsDefineKeyedOwnICKind(kind));
     RETURN_RESULT_OR_FAILURE(
-        isolate, Runtime::SetObjectProperty(isolate, object, key, value,
+        isolate,
+        IsDefineKeyedOwnICKind(kind)
+            ? Runtime::DefineObjectOwnProperty(isolate, object, key, value,
+                                               StoreOrigin::kMaybeKeyed)
+            : Runtime::SetObjectProperty(isolate, object, key, value,
                                          StoreOrigin::kMaybeKeyed));
   }
 }

src/ic/keyed-store-generic.cc

@@ -151,6 +151,9 @@ class KeyedStoreGenericAssembler : public AccessorAssembler {
   bool IsStoreInLiteral() const { return mode_ == StoreMode::kInLiteral; }
   bool IsDefineNamedOwn() const { return mode_ == StoreMode::kDefineNamedOwn; }
   bool IsDefineKeyedOwn() const { return mode_ == StoreMode::kDefineKeyedOwn; }
+  bool IsAnyDefineOwn() const {
+    return IsDefineNamedOwn() || IsDefineKeyedOwn();
+  }
 
   bool ShouldCheckPrototype() const { return IsKeyedStore(); }
   bool ShouldReconfigureExisting() const { return IsStoreInLiteral(); }
@@ -818,15 +821,15 @@ void KeyedStoreGenericAssembler::EmitGenericPropertyStore(
     TNode<DescriptorArray> descriptors = LoadMapDescriptors(receiver_map);
     Label descriptor_found(this), lookup_transition(this);
     TVARIABLE(IntPtrT, var_name_index);
-    DescriptorLookup(name, descriptors, bitfield3, &descriptor_found,
+    DescriptorLookup(name, descriptors, bitfield3,
+                     IsAnyDefineOwn() ? slow : &descriptor_found,
                      &var_name_index, &lookup_transition);
 
+    // When dealing with class fields defined with DefineKeyedOwnIC or
+    // DefineNamedOwnIC, use the slow path to check the existing property.
+    if (!IsAnyDefineOwn()) {
       BIND(&descriptor_found);
       {
-      if (IsDefineKeyedOwn()) {
-        // Take slow path to throw if a private name already exists.
-        GotoIf(IsPrivateSymbol(name), slow);
-      }
         TNode<IntPtrT> name_index = var_name_index.value();
         TNode<Uint32T> details = LoadDetailsByKeyIndex(descriptors, name_index);
         Label data_property(this);
@@ -835,7 +838,8 @@ void KeyedStoreGenericAssembler::EmitGenericPropertyStore(
 
         if (ShouldCallSetter()) {
           // Accessor case.
-        // TODO(jkummerow): Implement a trimmed-down LoadAccessorFromFastObject.
+          // TODO(jkummerow): Implement a trimmed-down
+          // LoadAccessorFromFastObject.
           LoadPropertyFromFastObject(receiver, receiver_map, descriptors,
                                      name_index, details, &var_accessor_pair);
           var_accessor_holder = receiver;
@@ -852,8 +856,8 @@ void KeyedStoreGenericAssembler::EmitGenericPropertyStore(
 
           CheckForAssociatedProtector(name, slow);
           OverwriteExistingFastDataProperty(receiver, receiver_map, descriptors,
-                                          name_index, details, p->value(), slow,
-                                          false);
+                                            name_index, details, p->value(),
+                                            slow, false);
           exit_point->Return(p->value());
 
           BIND(&shared);
@@ -865,6 +869,8 @@ void KeyedStoreGenericAssembler::EmitGenericPropertyStore(
           }
         }
       }
+    }
+
     BIND(&lookup_transition);
     {
       Comment("lookup transition");
@@ -891,15 +897,17 @@ void KeyedStoreGenericAssembler::EmitGenericPropertyStore(
     TVARIABLE(IntPtrT, var_name_index);
     Label dictionary_found(this, &var_name_index), not_found(this);
     TNode<PropertyDictionary> properties = CAST(LoadSlowProperties(receiver));
+
+    // When dealing with class fields defined with DefineKeyedOwnIC or
+    // DefineNamedOwnIC, use the slow path to check the existing property.
     NameDictionaryLookup<PropertyDictionary>(
-        properties, name, &dictionary_found, &var_name_index, &not_found);
+        properties, name, IsAnyDefineOwn() ? slow : &dictionary_found,
+        &var_name_index, &not_found);
+
+    if (!IsAnyDefineOwn()) {
       BIND(&dictionary_found);
       {
         Label check_const(this), overwrite(this), done(this);
-      if (IsDefineKeyedOwn()) {
-        // Take slow path to throw if a private name already exists.
-        GotoIf(IsPrivateSymbol(name), slow);
-      }
         TNode<Uint32T> details =
             LoadDetailsByKeyIndex(properties, var_name_index.value());
         JumpIfDataProperty(details, &check_const,
@@ -942,6 +950,7 @@ void KeyedStoreGenericAssembler::EmitGenericPropertyStore(
         BIND(&done);
         exit_point->Return(p->value());
       }
+    }
 
     BIND(&not_found);
     {
@@ -1022,31 +1031,26 @@ void KeyedStoreGenericAssembler::EmitGenericPropertyStore(
     }
   }
 
-  if (!ShouldReconfigureExisting()) {
+  if (!ShouldReconfigureExisting() && !IsAnyDefineOwn()) {
     BIND(&readonly);
     {
-      if (IsDefineKeyedOwn() || IsDefineNamedOwn()) {
-        Goto(slow);
-      } else {
       LanguageMode language_mode;
       if (maybe_language_mode.To(&language_mode)) {
         if (language_mode == LanguageMode::kStrict) {
           TNode<String> type = Typeof(p->receiver());
-            ThrowTypeError(p->context(),
-                           MessageTemplate::kStrictReadOnlyProperty, name, type,
-                           p->receiver());
+          ThrowTypeError(p->context(), MessageTemplate::kStrictReadOnlyProperty,
+                         name, type, p->receiver());
         } else {
           exit_point->Return(p->value());
         }
       } else {
         CallRuntime(Runtime::kThrowTypeErrorIfStrict, p->context(),
-                      SmiConstant(MessageTemplate::kStrictReadOnlyProperty),
-                      name, Typeof(p->receiver()), p->receiver());
+                    SmiConstant(MessageTemplate::kStrictReadOnlyProperty), name,
+                    Typeof(p->receiver()), p->receiver());
         exit_point->Return(p->value());
       }
     }
   }
-  }
 }
 
 // Helper that is used by the public KeyedStoreGeneric and by SetProperty.

src/objects/js-objects.cc

@@ -1655,6 +1655,49 @@ Maybe<bool> JSReceiver::CreateDataProperty(LookupIterator* it,
                                        &new_desc, should_throw);
 }
 
+// static
+Maybe<bool> JSReceiver::AddPrivateField(LookupIterator* it,
+                                        Handle<Object> value,
+                                        Maybe<ShouldThrow> should_throw) {
+  Handle<JSReceiver> receiver = Handle<JSReceiver>::cast(it->GetReceiver());
+  Isolate* isolate = receiver->GetIsolate();
+  DCHECK(it->GetName()->IsPrivateName());
+  Handle<Symbol> symbol = Handle<Symbol>::cast(it->GetName());
+
+  switch (it->state()) {
+    case LookupIterator::JSPROXY: {
+      PropertyDescriptor new_desc;
+      new_desc.set_value(value);
+      new_desc.set_writable(true);
+      new_desc.set_enumerable(true);
+      new_desc.set_configurable(true);
+      return JSProxy::SetPrivateSymbol(isolate, Handle<JSProxy>::cast(receiver),
+                                       symbol, &new_desc, should_throw);
+    }
+    case LookupIterator::DATA:
+    case LookupIterator::INTERCEPTOR:
+    case LookupIterator::ACCESSOR:
+    case LookupIterator::INTEGER_INDEXED_EXOTIC:
+      UNREACHABLE();
+
+    case LookupIterator::ACCESS_CHECK: {
+      if (!it->HasAccess()) {
+        it->isolate()->ReportFailedAccessCheck(it->GetHolder<JSObject>());
+        RETURN_VALUE_IF_SCHEDULED_EXCEPTION(it->isolate(), Nothing<bool>());
+        return Just(true);
+      }
+      break;
+    }
+
+    case LookupIterator::TRANSITION:
+    case LookupIterator::NOT_FOUND:
+      break;
+  }
+
+  return Object::TransitionAndWriteDataProperty(it, value, NONE, should_throw,
+                                                StoreOrigin::kMaybeKeyed);
+}
+
 // static
 Maybe<bool> JSReceiver::GetOwnPropertyDescriptor(Isolate* isolate,
                                                  Handle<JSReceiver> object,
@@ -3417,7 +3460,7 @@ MaybeHandle<Object> JSObject::DefineOwnPropertyIgnoreAttributes(
 Maybe<bool> JSObject::DefineOwnPropertyIgnoreAttributes(
     LookupIterator* it, Handle<Object> value, PropertyAttributes attributes,
     Maybe<ShouldThrow> should_throw, AccessorInfoHandling handling,
-    EnforceDefineSemantics semantics) {
+    EnforceDefineSemantics semantics, StoreOrigin store_origin) {
   it->UpdateProtector();
   Handle<JSObject> object = Handle<JSObject>::cast(it->GetReceiver());
 
@@ -3523,7 +3566,7 @@ Maybe<bool> JSObject::DefineOwnPropertyIgnoreAttributes(
   }
 
   return Object::AddDataProperty(it, value, attributes, should_throw,
-                                 StoreOrigin::kNamed);
+                                 store_origin, semantics);
 }
 
 MaybeHandle<Object> JSObject::SetOwnPropertyIgnoreAttributes(

src/objects/js-objects.h

@@ -175,6 +175,13 @@ class JSReceiver : public TorqueGeneratedJSReceiver<JSReceiver, HeapObject> {
       LookupIterator* it, Handle<Object> value,
       Maybe<ShouldThrow> should_throw);
 
+  // Add private fields to the receiver, ignoring extensibility and the
+  // traps. The caller should check that the private field does not already
+  // exist on the receiver before calling this method.
+  V8_WARN_UNUSED_RESULT static Maybe<bool> AddPrivateField(
+      LookupIterator* it, Handle<Object> value,
+      Maybe<ShouldThrow> should_throw);
+
   // ES6 9.1.6.1
   V8_WARN_UNUSED_RESULT static Maybe<bool> OrdinaryDefineOwnProperty(
       Isolate* isolate, Handle<JSObject> object, Handle<Object> key,
@@ -409,16 +416,6 @@ class JSObject : public TorqueGeneratedJSObject<JSObject, JSReceiver> {
   // to the default behavior that calls the setter.
   enum AccessorInfoHandling { FORCE_FIELD, DONT_FORCE_FIELD };
 
-  // Currently DefineOwnPropertyIgnoreAttributes invokes the setter
-  // interceptor and user-defined setters during define operations,
-  // even in places where it makes more sense to invoke the definer
-  // interceptor and not invoke the setter: e.g. both the definer and
-  // the setter interceptors are called in Object.defineProperty().
-  // kDefine allows us to implement the define semantics correctly
-  // in selected locations.
-  // TODO(joyee): see if we can deprecate the old behavior.
-  enum class EnforceDefineSemantics { kSet, kDefine };
-
   V8_WARN_UNUSED_RESULT static MaybeHandle<Object>
   DefineOwnPropertyIgnoreAttributes(
       LookupIterator* it, Handle<Object> value, PropertyAttributes attributes,
@@ -429,7 +426,8 @@ class JSObject : public TorqueGeneratedJSObject<JSObject, JSReceiver> {
       LookupIterator* it, Handle<Object> value, PropertyAttributes attributes,
       Maybe<ShouldThrow> should_throw,
       AccessorInfoHandling handling = DONT_FORCE_FIELD,
-      EnforceDefineSemantics semantics = EnforceDefineSemantics::kSet);
+      EnforceDefineSemantics semantics = EnforceDefineSemantics::kSet,
+      StoreOrigin store_origin = StoreOrigin::kNamed);
 
   V8_WARN_UNUSED_RESULT static MaybeHandle<Object> V8_EXPORT_PRIVATE
   SetOwnPropertyIgnoreAttributes(Handle<JSObject> object, Handle<Name> name,

src/objects/lookup.cc

@@ -559,13 +559,15 @@ void LookupIterator::ReconfigureDataProperty(Handle<Object> value,
 #endif
 }
 
-// Can only be called when the receiver is a JSObject. JSProxy has to be handled
-// via a trap. Adding properties to primitive values is not observable.
+// Can only be called when the receiver is a JSObject, or when the name is a
+// private field, otherwise JSProxy has to be handled via a trap.
+// Adding properties to primitive values is not observable.
 void LookupIterator::PrepareTransitionToDataProperty(
     Handle<JSReceiver> receiver, Handle<Object> value,
     PropertyAttributes attributes, StoreOrigin store_origin) {
   DCHECK_IMPLIES(receiver->IsJSProxy(isolate_), name()->IsPrivate(isolate_));
-  DCHECK(receiver.is_identical_to(GetStoreTarget<JSReceiver>()));
+  DCHECK_IMPLIES(!receiver.is_identical_to(GetStoreTarget<JSReceiver>()),
+                 name()->IsPrivateName());
   if (state_ == TRANSITION) return;
 
   if (!IsElement() && name()->IsPrivate(isolate_)) {
@@ -624,7 +626,8 @@ void LookupIterator::ApplyTransitionToDataProperty(
     Handle<JSReceiver> receiver) {
   DCHECK_EQ(TRANSITION, state_);
 
-  DCHECK(receiver.is_identical_to(GetStoreTarget<JSReceiver>()));
+  DCHECK_IMPLIES(!receiver.is_identical_to(GetStoreTarget<JSReceiver>()),
+                 name()->IsPrivateName());
   holder_ = receiver;
   if (receiver->IsJSGlobalObject(isolate_)) {
     JSObject::InvalidatePrototypeChains(receiver->map(isolate_));

src/objects/objects.cc

@@ -2853,7 +2853,8 @@ Maybe<bool> Object::SetDataProperty(LookupIterator* it, Handle<Object> value) {
 Maybe<bool> Object::AddDataProperty(LookupIterator* it, Handle<Object> value,
                                     PropertyAttributes attributes,
                                     Maybe<ShouldThrow> should_throw,
-                                    StoreOrigin store_origin) {
+                                    StoreOrigin store_origin,
+                                    EnforceDefineSemantics semantics) {
   if (!it->GetReceiver()->IsJSReceiver()) {
     return CannotCreateProperty(it->isolate(), it->GetReceiver(), it->GetName(),
                                 value, should_throw);
@@ -2881,9 +2882,11 @@ Maybe<bool> Object::AddDataProperty(LookupIterator* it, Handle<Object> value,
   Isolate* isolate = it->isolate();
 
   if (it->ExtendingNonExtensible(receiver)) {
-    RETURN_FAILURE(
-        isolate, GetShouldThrow(it->isolate(), should_throw),
-        NewTypeError(MessageTemplate::kObjectNotExtensible, it->GetName()));
+    RETURN_FAILURE(isolate, GetShouldThrow(it->isolate(), should_throw),
+                   NewTypeError(semantics == EnforceDefineSemantics::kDefine
+                                    ? MessageTemplate::kDefineDisallowed
+                                    : MessageTemplate::kObjectNotExtensible,
+                                it->GetName()));
   }
 
   if (it->IsElement(*receiver)) {
@@ -2903,7 +2906,17 @@ Maybe<bool> Object::AddDataProperty(LookupIterator* it, Handle<Object> value,
                  Nothing<bool>());
     JSObject::ValidateElements(*receiver_obj);
     return Just(true);
-  } else {
+  }
+
+  return Object::TransitionAndWriteDataProperty(it, value, attributes,
+                                                should_throw, store_origin);
+}
+
+// static
+Maybe<bool> Object::TransitionAndWriteDataProperty(
+    LookupIterator* it, Handle<Object> value, PropertyAttributes attributes,
+    Maybe<ShouldThrow> should_throw, StoreOrigin store_origin) {
+  Handle<JSReceiver> receiver = it->GetStoreTarget<JSReceiver>();
   it->UpdateProtector();
   // Migrate to the most up-to-date map that will be able to store |value|
   // under it->name() with |attributes|.
@@ -2917,14 +2930,12 @@ Maybe<bool> Object::AddDataProperty(LookupIterator* it, Handle<Object> value,
 
 #if VERIFY_HEAP
     if (FLAG_verify_heap) {
-      receiver->HeapObjectVerify(isolate);
+      receiver->HeapObjectVerify(it->isolate());
     }
 #endif
-  }
 
     return Just(true);
 }
-
 // static
 MaybeHandle<Object> Object::ShareSlow(Isolate* isolate,
                                       Handle<HeapObject> value,
@@ -3586,7 +3597,6 @@ Maybe<bool> JSProxy::SetPrivateSymbol(Isolate* isolate, Handle<JSProxy> proxy,
                                       Handle<Symbol> private_name,
                                       PropertyDescriptor* desc,
                                       Maybe<ShouldThrow> should_throw) {
-  DCHECK(!private_name->IsPrivateName());
   // Despite the generic name, this can only add private data properties.
   if (!PropertyDescriptor::IsDataDescriptor(desc) ||
       desc->ToAttributes() != DONT_ENUM) {

src/objects/objects.h

@@ -277,6 +277,16 @@ enum class OnNonExistent { kThrowReferenceError, kReturnUndefined };
 // The element types selection for CreateListFromArrayLike.
 enum class ElementTypes { kAll, kStringAndSymbol };
 
+// Currently DefineOwnPropertyIgnoreAttributes invokes the setter
+// interceptor and user-defined setters during define operations,
+// even in places where it makes more sense to invoke the definer
+// interceptor and not invoke the setter: e.g. both the definer and
+// the setter interceptors are called in Object.defineProperty().
+// kDefine allows us to implement the define semantics correctly
+// in selected locations.
+// TODO(joyee): see if we can deprecate the old behavior.
+enum class EnforceDefineSemantics { kSet, kDefine };
+
 // TODO(mythria): Move this to a better place.
 ShouldThrow GetShouldThrow(Isolate* isolate, Maybe<ShouldThrow> should_throw);
 
@@ -539,8 +549,14 @@ class Object : public TaggedImpl<HeapObjectReferenceType::STRONG, Address> {
   V8_WARN_UNUSED_RESULT static Maybe<bool> SetDataProperty(
       LookupIterator* it, Handle<Object> value);
   V8_WARN_UNUSED_RESULT static Maybe<bool> AddDataProperty(
+      LookupIterator* it, Handle<Object> value, PropertyAttributes attributes,
+      Maybe<ShouldThrow> should_throw, StoreOrigin store_origin,
+      EnforceDefineSemantics semantics = EnforceDefineSemantics::kSet);
+
+  V8_WARN_UNUSED_RESULT static Maybe<bool> TransitionAndWriteDataProperty(
       LookupIterator* it, Handle<Object> value, PropertyAttributes attributes,
       Maybe<ShouldThrow> should_throw, StoreOrigin store_origin);
+
   V8_WARN_UNUSED_RESULT static inline MaybeHandle<Object> GetPropertyOrElement(
       Isolate* isolate, Handle<Object> object, Handle<Name> name);
   V8_WARN_UNUSED_RESULT static inline MaybeHandle<Object> GetPropertyOrElement(

src/runtime/runtime-object.cc

@@ -606,8 +606,9 @@ MaybeHandle<Object> Runtime::DefineObjectOwnProperty(
   if (!success) return MaybeHandle<Object>();
   LookupIterator it(isolate, object, lookup_key, LookupIterator::OWN);
 
-  if (it.IsFound() && key->IsSymbol() && Symbol::cast(*key).is_private_name()) {
+  if (key->IsSymbol() && Symbol::cast(*key).is_private_name()) {
     Handle<Symbol> private_symbol = Handle<Symbol>::cast(key);
+    if (it.IsFound()) {
       Handle<Object> name_string(private_symbol->description(), isolate);
       DCHECK(name_string->IsString());
       MessageTemplate message =
@@ -615,10 +616,12 @@ MaybeHandle<Object> Runtime::DefineObjectOwnProperty(
               ? MessageTemplate::kInvalidPrivateBrandReinitialization
               : MessageTemplate::kInvalidPrivateFieldReinitialization;
       THROW_NEW_ERROR(isolate, NewTypeError(message, name_string), Object);
+    } else {
+      MAYBE_RETURN_NULL(JSReceiver::AddPrivateField(&it, value, should_throw));
+    }
+  } else {
+    MAYBE_RETURN_NULL(JSReceiver::CreateDataProperty(&it, value, should_throw));
   }
-
-  MAYBE_RETURN_NULL(
-      Object::SetProperty(&it, value, store_origin, should_throw));
 
   return value;
 }

test/mjsunit/harmony/index-fields-nonextensible-global-proxy-no-lazy-feedback.js

+// Copyright 2022 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: --no-lazy-feedback-allocation
+
+d8.file.execute('test/mjsunit/harmony/index-fields-nonextensible-global-proxy.js');

test/mjsunit/harmony/index-fields-nonextensible-global-proxy.js

+// Copyright 2022 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.
+
+class Base {
+  constructor(arg) {
+    return arg;
+  }
+}
+
+class ClassNonExtensibleWithIndexField extends Base {
+  [0] = (() => {
+    Object.preventExtensions(this);
+    return 'defined';
+  })();
+  ['nonExtensible'] = 4;
+  constructor(arg) {
+    super(arg);
+  }
+}
+
+assertThrows(() => {
+  new ClassNonExtensibleWithIndexField(globalThis);
+}, TypeError, /Cannot define property 0, object is not extensible/);
+assertEquals("undefined", typeof nonExtensible);

test/mjsunit/harmony/private-fields-nonextensible-global-proxy-no-lazy-feedback.js

+// Copyright 2022 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: --no-lazy-feedback-allocation
+
+d8.file.execute('test/mjsunit/harmony/private-fields-nonextensible-global-proxy.js');

test/mjsunit/harmony/private-fields-nonextensible-global-proxy.js

+// Copyright 2022 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.
+
+class Base {
+  constructor(arg) {
+    return arg;
+  }
+}
+
+class ClassNonExtensibleWithPrivateField extends Base {
+  #privateField = (() => {
+    Object.preventExtensions(this);
+    return "defined";
+  })();
+  // In case the object has a null prototype, we'll use a static
+  // method to access the field.
+  static getPrivateField(obj) { return obj.#privateField; }
+  constructor(arg) {
+    super(arg);
+  }
+}
+
+new ClassNonExtensibleWithPrivateField(globalThis);
+assertEquals("defined", ClassNonExtensibleWithPrivateField.getPrivateField(globalThis));

test/mjsunit/regress/regress-crbug-1302527-no-lazy-feedback.js

+// Copyright 2021 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: --no-lazy-feedback-allocation
+
+d8.file.execute('test/mjsunit/regress/regress-crbug-1302527.js');

test/mjsunit/regress/regress-v8-12421.js

@@ -62,6 +62,21 @@
   }, TypeError, /Cannot redefine property: field2/);
 }
 
+{
+  class X {
+    field = Object.defineProperty(
+      this,
+      "field2",
+      { writable: true, configurable: false, value: 1}
+    );
+    field2 = true;
+  }
+
+  assertThrows(() => {
+    new X();
+  }, TypeError, /Cannot redefine property: field2/);
+}
+
 {
   let setterCalled = false;
   class X {
@@ -80,6 +95,34 @@
 
   let x = new X();
   assertFalse(setterCalled);
+  assertEquals({
+    "value": 2,
+    "writable": true,
+    "enumerable": true,
+    "configurable": true
+  }, Object.getOwnPropertyDescriptor(x, 'field2'));
+}
+
+{
+  let setterCalled = false;
+  class X {
+    field = Object.defineProperty(
+      this,
+      "field2",
+      {
+        configurable: false,
+        set(val) {
+          setterCalled = true;
+        }
+      }
+    );
+    field2 = 2;
+  }
+
+  assertThrows(() => {
+    new X();
+  }, TypeError, /Cannot redefine property: field2/);
+  assertFalse(setterCalled);
 }
 
 {
@@ -153,7 +196,7 @@
   class ClassNonExtensible extends Base {
     field = (() => {
       Object.preventExtensions(this);
-      return 1;
+      return 'defined';
     })();
     nonExtensible = 4;
     constructor(arg) {
@@ -162,35 +205,61 @@
   }
 
   // Test dictionary objects.
-  {
-    let dict = Object.create(null);
+  function testObject(getObject) {
+    let obj = getObject();
 
-    new ClassWithNormalField(dict);
-    assertEquals(1, dict.field);
-    assertEquals("written", dict.normalField);
+    new ClassWithNormalField(obj);
+    assertEquals(1, obj.field);
+    assertEquals("written", obj.normalField);
 
-    new ClassWithSetterField(dict);
+    obj = getObject();
+    new ClassWithSetterField(obj);
+    assertEquals(1, obj.field);
     assertFalse(setterCalled);
 
-    new ClassWithReadOnlyField(dict);
-    assertEquals("written", dict.readOnlyField);
+    obj = getObject();
+    new ClassWithReadOnlyField(obj);
+    assertEquals(1, obj.field);
+    assertEquals("written", obj.readOnlyField);
 
+    obj = getObject();
     assertThrows(() => {
-      new ClassWithNonConfigurableField(dict);
+      new ClassWithNonConfigurableField(obj);
     }, TypeError, /Cannot redefine property: nonConfigurableField/);
-    assertEquals("initial", dict.nonConfigurableField);
+    assertEquals("initial", obj.nonConfigurableField);
+    assertEquals(1, obj.field);
 
+    obj = getObject();
+    if (Object.hasOwn(obj, 'field')) {
       assertThrows(() => {
-      new ClassNonExtensible(dict);
+        new ClassNonExtensible(obj);
       }, TypeError, /Cannot define property nonExtensible, object is not extensible/);
-    assertEquals(undefined, dict.nonExtensible);
+      assertEquals({
+        "value": 'defined',
+        "writable": true,
+        "enumerable": true,
+        "configurable": true
+      }, Object.getOwnPropertyDescriptor(obj, 'field'));
+    } else {
+      assertThrows(() => {
+        new ClassNonExtensible(obj);
+      }, TypeError, /Cannot define property field, object is not extensible/);
+      assertFalse(Object.hasOwn(obj, 'field'));
     }
+    assertFalse(Object.hasOwn(obj, 'nonExtensible'));
+
+    return obj;
+  }
+  testObject(() => Object.create(null));
+  testObject( () => { return {field: 1000 } });
 
   // Test proxies.
   {
     let trapCalls = [];
+    function getProxy() {
+      trapCalls = [];
       let target = {};
-    let proxy = new Proxy(target, {
+      return new Proxy(target, {
         get(oTarget, sKey) {
           return oTarget[sKey];
         },
@@ -200,36 +269,39 @@
           return oTarget;
         }
       });
+    }
 
+    let proxy = getProxy();
     new ClassWithNormalField(proxy);
     assertEquals(1, proxy.field);
     assertEquals("written", proxy.normalField);
     assertEquals(["normalField", "field", "normalField"], trapCalls);
 
-    trapCalls = [];
+    proxy = getProxy();
     new ClassWithSetterField(proxy);
     assertFalse(setterCalled);
     assertEquals("written", proxy.setterField);
     assertEquals(["setterField", "field", "setterField"], trapCalls);
 
-    trapCalls = [];
+    proxy = getProxy();
     new ClassWithReadOnlyField(proxy);
     assertEquals("written", proxy.readOnlyField);
     assertEquals(["readOnlyField", "field", "readOnlyField"], trapCalls);
 
-    trapCalls = [];
+    proxy = getProxy();
     assertThrows(() => {
       new ClassWithNonConfigurableField(proxy);
     }, TypeError, /Cannot redefine property: nonConfigurableField/);
     assertEquals("initial", proxy.nonConfigurableField);
     assertEquals(["nonConfigurableField", "field", "nonConfigurableField"], trapCalls);
 
-    trapCalls = [];
+    proxy = getProxy();
     assertThrows(() => {
       new ClassNonExtensible(proxy);
-    }, TypeError, /Cannot define property nonExtensible, object is not extensible/);
-    assertEquals(undefined, proxy.nonExtensible);
-    assertEquals(["field", "nonExtensible"], trapCalls);
+    }, TypeError, /Cannot define property field, object is not extensible/);
+    assertFalse(Object.hasOwn(proxy, 'field'));
+    assertFalse(Object.hasOwn(proxy, 'nonExtensible'));
+    assertEquals(["field"], trapCalls);
   }
 
   // Test globalThis.
@@ -252,7 +324,7 @@
 
     assertThrows(() => {
       new ClassNonExtensible(globalThis);
-    }, TypeError, /Cannot add property nonExtensible, object is not extensible/);
+    }, TypeError, /Cannot define property nonExtensible, object is not extensible/);
     assertEquals("undefined", typeof nonExtensible);
   }
 }