[csa] Make JSProxy's CheckGetSetTrapResult bailout for certain names

The TryGetOwnProperty code supports only unique names that are not
array indices. Unfortunately, this is neither obvious from its type,
nor from its comment, nor from its code.

ProxiesCodeStubAssembler::CheckHasTrapResult violated the assumption
and was already fixed a few days ago. This CL fixes
CheckGetSetTrapResult and improves our code documentation in the
form of comments and assertions. Concretely:

- Add CodeStubAssembler::IsUniqueName and IsUniqueNameNoIndex
- Use IsUniqueNameNoIndex in CheckGetSetTrapResult to guard
  TryGetOwnProperty (bailout to runtime if not satisfied).
- Similarly, use IsUniqueNameNoIndex to simplify the previous fix in
  CheckHasTrapResult.
- Add a IsUniqueNameNoIndex CSA_ASSERT to TryGetOwnProperty and a few
  other places to avoid such bugs in the future.
- Add a IsUniqueName CSA_ASSERT to a few places where we apparently
  expect unique names (I don't know if those allow indices or not).
- Add a DCHECK to Name::IsUniqueName to ensure and document that this
  shortcut version is equivalent to HeapObject::IsUniqueName.

Bug: chromium:937618
Change-Id: Id4a18ab2a0e9c7591b087dd0c9fe018aa9b9ef3a
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1514732
Auto-Submit: Georg Neis <neis@chromium.org>
Reviewed-by: Georg Neis <neis@chromium.org>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Reviewed-by: Maya Lekova <mslekova@chromium.org>
Commit-Queue: Georg Neis <neis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#60196}

src/builtins/builtins-proxy-gen.cc

@@ -636,6 +636,7 @@ void ProxiesCodeStubAssembler::CheckGetSetTrapResult(
 
   Label if_found_value(this), check_in_runtime(this, Label::kDeferred);
 
+  GotoIfNot(IsUniqueNameNoIndex(CAST(name)), &check_in_runtime);
   Node* instance_type = LoadInstanceType(target);
   TryGetOwnProperty(context, target, target, map, instance_type, name,
                     &if_found_value, &var_value, &var_details, &var_raw_value,
@@ -738,18 +739,13 @@ void ProxiesCodeStubAssembler::CheckHasTrapResult(Node* context, Node* target,
   VARIABLE(var_details, MachineRepresentation::kWord32);
   VARIABLE(var_raw_value, MachineRepresentation::kTagged);
 
-  Label if_unique_name(this), if_found_value(this, Label::kDeferred),
+  Label if_found_value(this, Label::kDeferred),
       throw_non_configurable(this, Label::kDeferred),
       throw_non_extensible(this, Label::kDeferred);
 
-  // If the name is a unique name, bailout to the runtime.
-  VARIABLE(var_index, MachineType::PointerRepresentation(), IntPtrConstant(0));
-  VARIABLE(var_name, MachineRepresentation::kTagged);
-  TryToName(name, if_bailout, &var_index, &if_unique_name, &var_name,
-            if_bailout);
-
   // 9.a. Let targetDesc be ? target.[[GetOwnProperty]](P).
-  BIND(&if_unique_name);
+  Print(name);
+  GotoIfNot(IsUniqueNameNoIndex(CAST(name)), if_bailout);
   Node* instance_type = LoadInstanceType(target);
   TryGetOwnProperty(context, target, target, target_map, instance_type, name,
                     &if_found_value, &var_value, &var_details, &var_raw_value,

src/code-stub-assembler.cc

@@ -6343,6 +6343,34 @@ TNode<BoolT> CodeStubAssembler::IsSymbol(SloppyTNode<HeapObject> object) {
   return IsSymbolMap(LoadMap(object));
 }
 
+TNode<BoolT> CodeStubAssembler::IsInternalizedStringInstanceType(
+    TNode<Int32T> instance_type) {
+  STATIC_ASSERT(kNotInternalizedTag != 0);
+  return Word32Equal(
+      Word32And(instance_type,
+                Int32Constant(kIsNotStringMask | kIsNotInternalizedMask)),
+      Int32Constant(kStringTag | kInternalizedTag));
+}
+
+TNode<BoolT> CodeStubAssembler::IsUniqueName(TNode<HeapObject> object) {
+  TNode<Int32T> instance_type = LoadInstanceType(object);
+  return Select<BoolT>(
+      IsInternalizedStringInstanceType(instance_type),
+      [=] { return Int32TrueConstant(); },
+      [=] { return IsSymbolInstanceType(instance_type); });
+}
+
+TNode<BoolT> CodeStubAssembler::IsUniqueNameNoIndex(TNode<HeapObject> object) {
+  TNode<Int32T> instance_type = LoadInstanceType(object);
+  return Select<BoolT>(
+      IsInternalizedStringInstanceType(instance_type),
+      [=] {
+        return IsSetWord32(LoadNameHashField(CAST(object)),
+                           Name::kIsNotArrayIndexMask);
+      },
+      [=] { return IsSymbolInstanceType(instance_type); });
+}
+
 TNode<BoolT> CodeStubAssembler::IsBigIntInstanceType(
     SloppyTNode<Int32T> instance_type) {
   return InstanceTypeEqual(instance_type, BIGINT_TYPE);
@@ -8350,6 +8378,7 @@ void CodeStubAssembler::NameDictionaryLookup(
   DCHECK_IMPLIES(mode == kFindInsertionIndex,
                  inlined_probes == 0 && if_found == nullptr);
   Comment("NameDictionaryLookup");
+  CSA_ASSERT(this, IsUniqueName(unique_name));
 
   TNode<IntPtrT> capacity = SmiUntag(GetCapacity<Dictionary>(dictionary));
   TNode<WordT> mask = IntPtrSub(capacity, IntPtrConstant(1));
@@ -8666,6 +8695,7 @@ void CodeStubAssembler::LookupLinear(TNode<Name> unique_name,
                     std::is_base_of<DescriptorArray, Array>::value,
                 "T must be a descendant of FixedArray or a WeakFixedArray");
   Comment("LookupLinear");
+  CSA_ASSERT(this, IsUniqueName(unique_name));
   TNode<IntPtrT> first_inclusive = IntPtrConstant(Array::ToKeyIndex(0));
   TNode<IntPtrT> factor = IntPtrConstant(Array::kEntrySize);
   TNode<IntPtrT> last_exclusive = IntPtrAdd(
@@ -9076,6 +9106,7 @@ void CodeStubAssembler::TryLookupPropertyInSimpleObject(
     TVariable<HeapObject>* var_meta_storage, TVariable<IntPtrT>* var_name_index,
     Label* if_not_found) {
   CSA_ASSERT(this, IsSimpleObjectMap(map));
+  CSA_ASSERT(this, IsUniqueNameNoIndex(unique_name));
 
   TNode<Uint32T> bit_field3 = LoadMapBitField3(map);
   Label if_isfastmap(this), if_isslowmap(this);
@@ -9138,6 +9169,7 @@ void CodeStubAssembler::TryHasOwnProperty(Node* object, Node* map,
                                           Label* if_not_found,
                                           Label* if_bailout) {
   Comment("TryHasOwnProperty");
+  CSA_ASSERT(this, IsUniqueNameNoIndex(CAST(unique_name)));
   TVARIABLE(HeapObject, var_meta_storage);
   TVARIABLE(IntPtrT, var_name_index);
 
@@ -9436,6 +9468,7 @@ void CodeStubAssembler::TryGetOwnProperty(
     Label* if_bailout, GetOwnPropertyMode mode) {
   DCHECK_EQ(MachineRepresentation::kTagged, var_value->rep());
   Comment("TryGetOwnProperty");
+  CSA_ASSERT(this, IsUniqueNameNoIndex(CAST(unique_name)));
 
   TVARIABLE(HeapObject, var_meta_storage);
   TVARIABLE(IntPtrT, var_entry);

src/code-stub-assembler.h

@@ -2172,6 +2172,9 @@ class V8_EXPORT_PRIVATE CodeStubAssembler
   TNode<BoolT> IsString(SloppyTNode<HeapObject> object);
   TNode<BoolT> IsSymbolInstanceType(SloppyTNode<Int32T> instance_type);
   TNode<BoolT> IsSymbol(SloppyTNode<HeapObject> object);
+  TNode<BoolT> IsInternalizedStringInstanceType(TNode<Int32T> instance_type);
+  TNode<BoolT> IsUniqueName(TNode<HeapObject> object);
+  TNode<BoolT> IsUniqueNameNoIndex(TNode<HeapObject> object);
   TNode<BoolT> IsUndetectableMap(SloppyTNode<Map> map);
   TNode<BoolT> IsNotWeakFixedArraySubclass(SloppyTNode<HeapObject> object);
   TNode<BoolT> IsZeroOrContext(SloppyTNode<Object> object);
@@ -2500,6 +2503,9 @@ class V8_EXPORT_PRIVATE CodeStubAssembler
   // Various building blocks for stubs doing property lookups.
 
   // |if_notinternalized| is optional; |if_bailout| will be used by default.
+  // Note: If |key| does not yet have a hash, |if_notinternalized| will be taken
+  // even if |key| is an array index. |if_keyisunique| will never
+  // be taken for array indices.
   void TryToName(Node* key, Label* if_keyisindex, Variable* var_index,
                  Label* if_keyisunique, Variable* var_unique, Label* if_bailout,
                  Label* if_notinternalized = nullptr);
@@ -2687,7 +2693,8 @@ class V8_EXPORT_PRIVATE CodeStubAssembler
   enum GetOwnPropertyMode { kCallJSGetter, kReturnAccessorPair };
   // Tries to get {object}'s own {unique_name} property value. If the property
   // is an accessor then it also calls a getter. If the property is a double
-  // field it re-wraps value in an immutable heap number.
+  // field it re-wraps value in an immutable heap number. {unique_name} must be
+  // a unique name (Symbol or InternalizedString) that is not an array index.
   void TryGetOwnProperty(Node* context, Node* receiver, Node* object, Node* map,
                          Node* instance_type, Node* unique_name,
                          Label* if_found, Variable* var_value,

src/objects/instance-type.h

@@ -20,8 +20,8 @@ namespace internal {
 const uint32_t kIsNotStringMask = 0xffc0;
 const uint32_t kStringTag = 0x0;
 
-// Bit 5 indicates that the object is an internalized string (if set) or not.
+// Bit 5 indicates that the object is an internalized string (if not set) or
-// Bit 7 has to be clear as well.
+// not (if set). Bit 7 has to be clear as well.
 const uint32_t kIsNotInternalizedMask = 0x20;
 const uint32_t kNotInternalizedTag = 0x20;
 const uint32_t kInternalizedTag = 0x0;

src/objects/name-inl.h

@@ -46,8 +46,10 @@ void Symbol::set_is_private_name() {
 
 bool Name::IsUniqueName() const {
   uint32_t type = map()->instance_type();
-  return (type & (kIsNotStringMask | kIsNotInternalizedMask)) !=
+  bool result = (type & (kIsNotStringMask | kIsNotInternalizedMask)) !=
-         (kStringTag | kNotInternalizedTag);
+                (kStringTag | kNotInternalizedTag);
+  SLOW_DCHECK(result == HeapObject::IsUniqueName());
+  return result;
 }
 
 uint32_t Name::hash_field() {