[runtime] Do not handle shadowing keys in CollectKeysTo

Make it explicit that AddShadowingKeys might allocate.

Bug: chromium:1049013
Change-Id: I938531a0324fa581422b74813518f3e85c9b3fbb
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2046888
Commit-Queue: Camillo Bruni <cbruni@chromium.org>
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#66288}

src/objects/elements.cc

@@ -1549,11 +1549,13 @@ class DictionaryElementsAccessor
     PropertyFilter filter = keys->filter();
     ReadOnlyRoots roots(isolate);
     for (InternalIndex i : dictionary->IterateEntries()) {
+      AllowHeapAllocation allow_gc;
       Object raw_key = dictionary->KeyAt(i);
       if (!dictionary->IsKey(roots, raw_key)) continue;
       uint32_t key = FilterKey(dictionary, i, raw_key, filter);
       if (key == kMaxUInt32) {
-        keys->AddShadowingKey(raw_key);
+        // This might allocate, but {raw_key} is not used afterwards.
+        keys->AddShadowingKey(raw_key, &allow_gc);
         continue;
       }
       elements->set(insertion_index, raw_key);

src/objects/keys.cc

@@ -281,7 +281,8 @@ bool KeyAccumulator::IsShadowed(Handle<Object> key) {
   return shadowing_keys_->Has(isolate_, key);
 }
 
-void KeyAccumulator::AddShadowingKey(Object key) {
+void KeyAccumulator::AddShadowingKey(Object key,
+                                     AllowHeapAllocation* allow_gc) {
   if (mode_ == KeyCollectionMode::kOwnOnly) return;
   AddShadowingKey(handle(key, isolate_));
 }
@@ -737,6 +738,7 @@ template <bool skip_symbols>
 base::Optional<int> CollectOwnPropertyNamesInternal(
     Handle<JSObject> object, KeyAccumulator* keys,
     Handle<DescriptorArray> descs, int start_index, int limit) {
+  AllowHeapAllocation allow_gc;
   int first_skipped = -1;
   PropertyFilter filter = keys->filter();
   KeyCollectionMode mode = keys->mode();
@@ -767,7 +769,9 @@ base::Optional<int> CollectOwnPropertyNamesInternal(
     if (key.FilterKey(keys->filter())) continue;
 
     if (is_shadowing_key) {
-      keys->AddShadowingKey(key);
+      // This might allocate, but {key} is not used afterwards.
+      keys->AddShadowingKey(key, &allow_gc);
+      continue;
     } else {
       if (keys->AddKey(key, DO_NOT_CONVERT) != ExceptionStatus::kSuccess) {
         return base::Optional<int>();
@@ -806,13 +810,13 @@ Maybe<bool> KeyAccumulator::CollectOwnPropertyNames(Handle<JSReceiver> receiver,
       int nof_descriptors = map.NumberOfOwnDescriptors();
       if (enum_keys->length() != nof_descriptors) {
         if (map.prototype(isolate_) != ReadOnlyRoots(isolate_).null_value()) {
+          AllowHeapAllocation allow_gc;
           Handle<DescriptorArray> descs =
               Handle<DescriptorArray>(map.instance_descriptors(), isolate_);
           for (InternalIndex i : InternalIndex::Range(nof_descriptors)) {
             PropertyDetails details = descs->GetDetails(i);
             if (!details.IsDontEnum()) continue;
-            Object key = descs->GetKey(i);
-            this->AddShadowingKey(key);
+            this->AddShadowingKey(descs->GetKey(i), &allow_gc);
           }
         }
       }
@@ -866,6 +870,7 @@ Maybe<bool> KeyAccumulator::CollectOwnPropertyNames(Handle<JSReceiver> receiver,
 
 ExceptionStatus KeyAccumulator::CollectPrivateNames(Handle<JSReceiver> receiver,
                                                     Handle<JSObject> object) {
+  DCHECK_EQ(mode_, KeyCollectionMode::kOwnOnly);
   if (object->HasFastProperties()) {
     int limit = object->map().NumberOfOwnDescriptors();
     Handle<DescriptorArray> descs(object->map().instance_descriptors(),

src/objects/keys.h

@@ -103,7 +103,7 @@ class KeyAccumulator final {
   void set_may_have_elements(bool value) { may_have_elements_ = value; }
   // Shadowing keys are used to filter keys. This happens when non-enumerable
   // keys appear again on the prototype chain.
-  void AddShadowingKey(Object key);
+  void AddShadowingKey(Object key, AllowHeapAllocation* allow_gc);
   void AddShadowingKey(Handle<Object> key);
 
  private:

src/objects/objects.cc

@@ -7443,41 +7443,47 @@ void BaseNameDictionary<Derived, Shape>::CopyEnumKeysTo(
   int length = storage->length();
   int properties = 0;
   ReadOnlyRoots roots(isolate);
-  for (InternalIndex i : dictionary->IterateEntries()) {
-    Object key;
-    if (!dictionary->ToKey(roots, i, &key)) continue;
-    bool is_shadowing_key = false;
-    if (key.IsSymbol()) continue;
-    PropertyDetails details = dictionary->DetailsAt(i);
-    if (details.IsDontEnum()) {
-      if (mode == KeyCollectionMode::kIncludePrototypes) {
-        is_shadowing_key = true;
-      } else {
+  {
+    AllowHeapAllocation allow_gc;
+    for (InternalIndex i : dictionary->IterateEntries()) {
+      Object key;
+      if (!dictionary->ToKey(roots, i, &key)) continue;
+      bool is_shadowing_key = false;
+      if (key.IsSymbol()) continue;
+      PropertyDetails details = dictionary->DetailsAt(i);
+      if (details.IsDontEnum()) {
+        if (mode == KeyCollectionMode::kIncludePrototypes) {
+          is_shadowing_key = true;
+        } else {
+          continue;
+        }
+      }
+      if (is_shadowing_key) {
+        // This might allocate, but {key} is not used afterwards.
+        accumulator->AddShadowingKey(key, &allow_gc);
         continue;
+      } else {
+        storage->set(properties, Smi::FromInt(i.as_int()));
       }
+      properties++;
+      if (mode == KeyCollectionMode::kOwnOnly && properties == length) break;
     }
-    if (is_shadowing_key) {
-      accumulator->AddShadowingKey(key);
-      continue;
-    } else {
-      storage->set(properties, Smi::FromInt(i.as_int()));
-    }
-    properties++;
-    if (mode == KeyCollectionMode::kOwnOnly && properties == length) break;
   }
 
   CHECK_EQ(length, properties);
-  DisallowHeapAllocation no_gc;
-  Derived raw_dictionary = *dictionary;
-  FixedArray raw_storage = *storage;
-  EnumIndexComparator<Derived> cmp(raw_dictionary);
-  // Use AtomicSlot wrapper to ensure that std::sort uses atomic load and
-  // store operations that are safe for concurrent marking.
-  AtomicSlot start(storage->GetFirstElementAddress());
-  std::sort(start, start + length, cmp);
-  for (int i = 0; i < length; i++) {
-    InternalIndex index(Smi::ToInt(raw_storage.get(i)));
-    raw_storage.set(i, raw_dictionary.NameAt(index));
+  {
+    DisallowHeapAllocation no_gc;
+    Derived raw_dictionary = *dictionary;
+    FixedArray raw_storage = *storage;
+    EnumIndexComparator<Derived> cmp(raw_dictionary);
+    // Use AtomicSlot wrapper to ensure that std::sort uses atomic load and
+    // store operations that are safe for concurrent marking.
+    AtomicSlot start(storage->GetFirstElementAddress());
+    std::sort(start, start + length, cmp);
+    for (int i = 0; i < length; i++) {
+      InternalIndex index(Smi::ToInt(raw_storage.get(i)));
+      raw_storage.set(i, raw_dictionary.NameAt(index));
+    }
   }
 }
 
@@ -7518,16 +7524,20 @@ ExceptionStatus BaseNameDictionary<Derived, Shape>::CollectKeysTo(
       isolate->factory()->NewFixedArray(dictionary->NumberOfElements());
   int array_size = 0;
   PropertyFilter filter = keys->filter();
+  // Handle enumerable strings in CopyEnumKeysTo.
+  DCHECK_NE(keys->filter(), ENUMERABLE_STRINGS);
   {
     DisallowHeapAllocation no_gc;
-    Derived raw_dictionary = *dictionary;
     for (InternalIndex i : dictionary->IterateEntries()) {
-      Object k;
-      if (!raw_dictionary.ToKey(roots, i, &k)) continue;
-      if (k.FilterKey(filter)) continue;
+      Object key;
+      Derived raw_dictionary = *dictionary;
+      if (!raw_dictionary.ToKey(roots, i, &key)) continue;
+      if (key.FilterKey(filter)) continue;
       PropertyDetails details = raw_dictionary.DetailsAt(i);
       if ((details.attributes() & filter) != 0) {
-        keys->AddShadowingKey(k);
+        AllowHeapAllocation gc;
+        // This might allocate, but {key} is not used afterwards.
+        keys->AddShadowingKey(key, &gc);
         continue;
       }
       if (filter & ONLY_ALL_CAN_READ) {
@@ -7539,7 +7549,7 @@ ExceptionStatus BaseNameDictionary<Derived, Shape>::CollectKeysTo(
       array->set(array_size++, Smi::FromInt(i.as_int()));
     }
 
-    EnumIndexComparator<Derived> cmp(raw_dictionary);
+    EnumIndexComparator<Derived> cmp(*dictionary);
     // Use AtomicSlot wrapper to ensure that std::sort uses atomic load and
     // store operations that are safe for concurrent marking.
     AtomicSlot start(array->GetFirstElementAddress());