Revert of [keys] Simplify KeyAccumulator

src/keys.cc

Index: src/keys.cc
diff --git a/src/keys.cc b/src/keys.cc
index bacb201b20d44c79513ea7cf5831e4b71ff847af..17270eb1bede8711201731f3010c02b16a2cb2ed 100644
--- a/src/keys.cc
+++ b/src/keys.cc
@@ -17,6 +17,9 @@
 namespace internal {
 
 KeyAccumulator::~KeyAccumulator() {
+  for (size_t i = 0; i < elements_.size(); i++) {
+    delete elements_[i];
+  }
 }
 
 namespace {
@@ -34,12 +37,11 @@
 
 MaybeHandle<FixedArray> KeyAccumulator::GetKeys(
     Handle<JSReceiver> object, KeyCollectionType type, PropertyFilter filter,
-    GetKeysConversion keys_conversion, bool filter_proxy_keys, bool is_for_in) {
+    GetKeysConversion keys_conversion, bool filter_proxy_keys) {
   USE(ContainsOnlyValidKeys);
   Isolate* isolate = object->GetIsolate();
   KeyAccumulator accumulator(isolate, type, filter);
   accumulator.set_filter_proxy_keys(filter_proxy_keys);
-  accumulator.set_is_for_in(is_for_in);
   MAYBE_RETURN(accumulator.CollectKeys(object, object),
                MaybeHandle<FixedArray>());
   Handle<FixedArray> keys = accumulator.GetKeys(keys_conversion);
@@ -48,41 +50,181 @@
 }
 
 Handle<FixedArray> KeyAccumulator::GetKeys(GetKeysConversion convert) {
-  if (keys_.is_null()) {
+  if (length_ == 0) {
     return isolate_->factory()->empty_fixed_array();
   }
-  if (type_ == OWN_ONLY &&
-      keys_->map() == isolate_->heap()->fixed_array_map()) {
-    return Handle<FixedArray>::cast(keys_);
-  }
-  return OrderedHashSet::ConvertToKeysArray(keys(), convert);
-}
-
-void KeyAccumulator::AddKey(Object* key, AddKeyConversion convert) {
-  AddKey(handle(key, isolate_), convert);
-}
-
-void KeyAccumulator::AddKey(Handle<Object> key, AddKeyConversion convert) {
+  // Make sure we have all the lengths collected.
+  NextPrototype();
+
+  if (type_ == OWN_ONLY && !ownProxyKeys_.is_null()) {
+    return ownProxyKeys_;
+  }
+  // Assemble the result array by first adding the element keys and then the
+  // property keys. We use the total number of String + Symbol keys per level in
+  // |level_lengths_| and the available element keys in the corresponding bucket
+  // in |elements_| to deduce the number of keys to take from the
+  // |string_properties_| and |symbol_properties_| set.
+  Handle<FixedArray> result = isolate_->factory()->NewFixedArray(length_);
+  int insertion_index = 0;
+  int string_properties_index = 0;
+  int symbol_properties_index = 0;
+  // String and Symbol lengths always come in pairs:
+  size_t max_level = level_lengths_.size() / 2;
+  for (size_t level = 0; level < max_level; level++) {
+    int num_string_properties = level_lengths_[level * 2];
+    int num_symbol_properties = level_lengths_[level * 2 + 1];
+    int num_elements = 0;
+    if (num_string_properties < 0) {
+      // If the |num_string_properties| is negative, the current level contains
+      // properties from a proxy, hence we skip the integer keys in |elements_|
+      // since proxies define the complete ordering.
+      num_string_properties = -num_string_properties;
+    } else if (level < elements_.size()) {
+      // Add the element indices for this prototype level.
+      std::vector<uint32_t>* elements = elements_[level];
+      num_elements = static_cast<int>(elements->size());
+      for (int i = 0; i < num_elements; i++) {
+        Handle<Object> key;
+        if (convert == KEEP_NUMBERS) {
+          key = isolate_->factory()->NewNumberFromUint(elements->at(i));
+        } else {
+          key = isolate_->factory()->Uint32ToString(elements->at(i));
+        }
+        result->set(insertion_index, *key);
+        insertion_index++;
+      }
+    }
+    // Add the string property keys for this prototype level.
+    for (int i = 0; i < num_string_properties; i++) {
+      Object* key = string_properties_->KeyAt(string_properties_index);
+      result->set(insertion_index, key);
+      insertion_index++;
+      string_properties_index++;
+    }
+    // Add the symbol property keys for this prototype level.
+    for (int i = 0; i < num_symbol_properties; i++) {
+      Object* key = symbol_properties_->KeyAt(symbol_properties_index);
+      result->set(insertion_index, key);
+      insertion_index++;
+      symbol_properties_index++;
+    }
+    if (FLAG_trace_for_in_enumerate) {
+      PrintF("| strings=%d symbols=%d elements=%i ", num_string_properties,
+             num_symbol_properties, num_elements);
+    }
+  }
+  if (FLAG_trace_for_in_enumerate) {
+    PrintF("|| prototypes=%zu ||\n", max_level);
+  }
+
+  DCHECK_EQ(insertion_index, length_);
+  return result;
+}
+
+namespace {
+
+bool AccumulatorHasKey(std::vector<uint32_t>* sub_elements, uint32_t key) {
+  return std::binary_search(sub_elements->begin(), sub_elements->end(), key);
+}
+
+}  // namespace
+
+bool KeyAccumulator::AddKey(Object* key, AddKeyConversion convert) {
+  return AddKey(handle(key, isolate_), convert);
+}
+
+bool KeyAccumulator::AddKey(Handle<Object> key, AddKeyConversion convert) {
   if (key->IsSymbol()) {
-    if (filter_ & SKIP_SYMBOLS) return;
-    if (Handle<Symbol>::cast(key)->is_private()) return;
-  } else if (filter_ & SKIP_STRINGS) {
-    return;
-  }
-  if (keys_.is_null()) {
-    keys_ = OrderedHashSet::Allocate(isolate_, 16);
-  }
-  uint32_t index;
-  if (convert == CONVERT_TO_ARRAY_INDEX && key->IsString() &&
-      Handle<String>::cast(key)->AsArrayIndex(&index)) {
-    key = isolate_->factory()->NewNumberFromUint(index);
-  }
-  keys_ = OrderedHashSet::Add(keys(), key);
+    if (filter_ & SKIP_SYMBOLS) return false;
+    if (Handle<Symbol>::cast(key)->is_private()) return false;
+    return AddSymbolKey(key);
+  }
+  if (filter_ & SKIP_STRINGS) return false;
+  // Make sure we do not add keys to a proxy-level (see AddKeysFromJSProxy).
+  DCHECK_LE(0, level_string_length_);
+  // In some cases (e.g. proxies) we might get in String-converted ints which
+  // should be added to the elements list instead of the properties. For
+  // proxies we have to convert as well but also respect the original order.
+  // Therefore we add a converted key to both sides
+  if (convert == CONVERT_TO_ARRAY_INDEX || convert == PROXY_MAGIC) {
+    uint32_t index = 0;
+    int prev_length = length_;
+    int prev_proto = level_string_length_;
+    if ((key->IsString() && Handle<String>::cast(key)->AsArrayIndex(&index)) ||
+        key->ToArrayIndex(&index)) {
+      bool key_was_added = AddIntegerKey(index);
+      if (convert == CONVERT_TO_ARRAY_INDEX) return key_was_added;
+      if (convert == PROXY_MAGIC) {
+        // If we had an array index (number) and it wasn't added, the key
+        // already existed before, hence we cannot add it to the properties
+        // keys as it would lead to duplicate entries.
+        if (!key_was_added) {
+          return false;
+        }
+        length_ = prev_length;
+        level_string_length_ = prev_proto;
+      }
+    }
+  }
+  return AddStringKey(key, convert);
+}
+
+bool KeyAccumulator::AddKey(uint32_t key) { return AddIntegerKey(key); }
+
+bool KeyAccumulator::AddIntegerKey(uint32_t key) {
+  // Make sure we do not add keys to a proxy-level (see AddKeysFromJSProxy).
+  // We mark proxy-levels with a negative length
+  DCHECK_LE(0, level_string_length_);
+  // Binary search over all but the last level. The last one might not be
+  // sorted yet.
+  for (size_t i = 1; i < elements_.size(); i++) {
+    if (AccumulatorHasKey(elements_[i - 1], key)) return false;
+  }
+  elements_.back()->push_back(key);
+  length_++;
+  return true;
+}
+
+bool KeyAccumulator::AddStringKey(Handle<Object> key,
+                                  AddKeyConversion convert) {
+  if (string_properties_.is_null()) {
+    string_properties_ = OrderedHashSet::Allocate(isolate_, 16);
+  }
+  // TODO(cbruni): remove this conversion once we throw the correct TypeError
+  // for non-string/symbol elements returned by proxies
+  if (convert == PROXY_MAGIC && key->IsNumber()) {
+    key = isolate_->factory()->NumberToString(key);
+  }
+  int prev_size = string_properties_->NumberOfElements();
+  string_properties_ = OrderedHashSet::Add(string_properties_, key);
+  if (prev_size < string_properties_->NumberOfElements()) {
+    length_++;
+    level_string_length_++;
+    return true;
+  } else {
+    return false;
+  }
+}
+
+bool KeyAccumulator::AddSymbolKey(Handle<Object> key) {
+  if (symbol_properties_.is_null()) {
+    symbol_properties_ = OrderedHashSet::Allocate(isolate_, 16);
+  }
+  int prev_size = symbol_properties_->NumberOfElements();
+  symbol_properties_ = OrderedHashSet::Add(symbol_properties_, key);
+  if (prev_size < symbol_properties_->NumberOfElements()) {
+    length_++;
+    level_symbol_length_++;
+    return true;
+  } else {
+    return false;
+  }
 }
 
 void KeyAccumulator::AddKeys(Handle<FixedArray> array,
                              AddKeyConversion convert) {
   int add_length = array->length();
+  if (add_length == 0) return;
   for (int i = 0; i < add_length; i++) {
     Handle<Object> current(array->get(i), isolate_);
     AddKey(current, convert);
@@ -129,18 +271,63 @@
 Maybe<bool> KeyAccumulator::AddKeysFromJSProxy(Handle<JSProxy> proxy,
                                                Handle<FixedArray> keys) {
   if (filter_proxy_keys_) {
-    DCHECK(!is_for_in_);
     ASSIGN_RETURN_ON_EXCEPTION_VALUE(
         isolate_, keys, FilterProxyKeys(isolate_, proxy, keys, filter_),
         Nothing<bool>());
   }
-  if (type_ == OWN_ONLY && !is_for_in_) {
-    // If we collect only the keys from a JSProxy do not sort or deduplicate it.
-    keys_ = keys;
-    return Just(true);
-  }
-  AddKeys(keys, is_for_in_ ? CONVERT_TO_ARRAY_INDEX : DO_NOT_CONVERT);
+  // Proxies define a complete list of keys with no distinction of
+  // elements and properties, which breaks the normal assumption for the
+  // KeyAccumulator.
+  if (type_ == OWN_ONLY) {
+    ownProxyKeys_ = keys;
+    level_string_length_ = keys->length();
+    length_ = level_string_length_;
+  } else {
+    AddKeys(keys, PROXY_MAGIC);
+  }
+  // Invert the current length to indicate a present proxy, so we can ignore
+  // element keys for this level. Otherwise we would not fully respect the order
+  // given by the proxy.
+  level_string_length_ = -level_string_length_;
   return Just(true);
+}
+
+void KeyAccumulator::AddElementKeysFromInterceptor(
+    Handle<JSObject> array_like) {
+  AddKeys(array_like, CONVERT_TO_ARRAY_INDEX);
+  // The interceptor might introduce duplicates for the current level, since
+  // these keys get added after the objects's normal element keys.
+  SortCurrentElementsListRemoveDuplicates();
+}
+
+void KeyAccumulator::SortCurrentElementsListRemoveDuplicates() {
+  // Sort and remove duplicates from the current elements level and adjust.
+  // the lengths accordingly.
+  auto last_level = elements_.back();
+  size_t nof_removed_keys = last_level->size();
+  std::sort(last_level->begin(), last_level->end());
+  last_level->erase(std::unique(last_level->begin(), last_level->end()),
+                    last_level->end());
+  // Adjust total length by the number of removed duplicates.
+  nof_removed_keys -= last_level->size();
+  length_ -= static_cast<int>(nof_removed_keys);
+}
+
+void KeyAccumulator::SortCurrentElementsList() {
+  if (elements_.empty()) return;
+  auto element_keys = elements_.back();
+  std::sort(element_keys->begin(), element_keys->end());
+}
+
+void KeyAccumulator::NextPrototype() {
+  // Store the protoLength on the first call of this method.
+  if (!elements_.empty()) {
+    level_lengths_.push_back(level_string_length_);
+    level_lengths_.push_back(level_symbol_length_);
+  }
+  elements_.push_back(new std::vector<uint32_t>());
+  level_string_length_ = 0;
+  level_symbol_length_ = 0;
 }
 
 Maybe<bool> KeyAccumulator::CollectKeys(Handle<JSReceiver> receiver,
@@ -408,19 +595,17 @@
 
 MaybeHandle<FixedArray> FastKeyAccumulator::GetKeysSlow(
     GetKeysConversion convert) {
-  return KeyAccumulator::GetKeys(receiver_, type_, filter_, KEEP_NUMBERS,
-                                 filter_proxy_keys_, is_for_in_);
-}
-
-namespace {
+  return JSReceiver::GetKeys(receiver_, type_, filter_, KEEP_NUMBERS,
+                             filter_proxy_keys_);
+}
 
 enum IndexedOrNamed { kIndexed, kNamed };
 
 // Returns |true| on success, |nothing| on exception.
 template <class Callback, IndexedOrNamed type>
-Maybe<bool> GetKeysFromInterceptor(Handle<JSReceiver> receiver,
-                                   Handle<JSObject> object,
-                                   KeyAccumulator* accumulator) {
+static Maybe<bool> GetKeysFromInterceptor(Handle<JSReceiver> receiver,
+                                          Handle<JSObject> object,
+                                          KeyAccumulator* accumulator) {
   Isolate* isolate = accumulator->isolate();
   if (type == kIndexed) {
     if (!object->HasIndexedInterceptor()) return Just(true);
@@ -447,90 +632,54 @@
   }
   RETURN_VALUE_IF_SCHEDULED_EXCEPTION(isolate, Nothing<bool>());
   if (result.is_null()) return Just(true);
-  accumulator->AddKeys(
-      result, type == kIndexed ? CONVERT_TO_ARRAY_INDEX : DO_NOT_CONVERT);
+  DCHECK(result->IsJSArray() || result->HasSloppyArgumentsElements());
+  // The accumulator takes care of string/symbol filtering.
+  if (type == kIndexed) {
+    accumulator->AddElementKeysFromInterceptor(result);
+  } else {
+    accumulator->AddKeys(result, DO_NOT_CONVERT);
+  }
   return Just(true);
 }
 
-}  // namespace
-
-Maybe<bool> KeyAccumulator::CollectOwnElementIndices(
-    Handle<JSReceiver> receiver, Handle<JSObject> object) {
-  if (filter_ & SKIP_STRINGS || skip_indices_) return Just(true);
-
+void KeyAccumulator::CollectOwnElementIndices(Handle<JSObject> object) {
+  if (filter_ & SKIP_STRINGS) return;
   ElementsAccessor* accessor = object->GetElementsAccessor();
   accessor->CollectElementIndices(object, this);
-
-  return GetKeysFromInterceptor<v8::IndexedPropertyEnumeratorCallback,
-                                kIndexed>(receiver, object, this);
-}
-
-namespace {
-
-template <bool skip_symbols>
-int CollectOwnPropertyNamesInternal(Handle<JSObject> object,
-                                    KeyAccumulator* keys,
-                                    Handle<DescriptorArray> descs,
-                                    int start_index, int limit) {
-  int first_skipped = -1;
-  for (int i = start_index; i < limit; i++) {
-    PropertyDetails details = descs->GetDetails(i);
-    if ((details.attributes() & keys->filter()) != 0) continue;
-    if (keys->filter() & ONLY_ALL_CAN_READ) {
-      if (details.kind() != kAccessor) continue;
-      Object* accessors = descs->GetValue(i);
-      if (!accessors->IsAccessorInfo()) continue;
-      if (!AccessorInfo::cast(accessors)->all_can_read()) continue;
-    }
-    Name* key = descs->GetKey(i);
-    if (skip_symbols == key->IsSymbol()) {
-      if (first_skipped == -1) first_skipped = i;
-      continue;
-    }
-    if (key->FilterKey(keys->filter())) continue;
-    keys->AddKey(key, DO_NOT_CONVERT);
-  }
-  return first_skipped;
-}
-
-}  // namespace
-
-Maybe<bool> KeyAccumulator::CollectOwnPropertyNames(Handle<JSReceiver> receiver,
-                                                    Handle<JSObject> object) {
-  if (filter_ == ENUMERABLE_STRINGS) {
-    Handle<FixedArray> enum_keys =
-        KeyAccumulator::GetEnumPropertyKeys(isolate_, object);
-    AddKeys(enum_keys, DO_NOT_CONVERT);
+}
+
+void KeyAccumulator::CollectOwnPropertyNames(Handle<JSObject> object) {
+  if (object->HasFastProperties()) {
+    int real_size = object->map()->NumberOfOwnDescriptors();
+    Handle<DescriptorArray> descs(object->map()->instance_descriptors(),
+                                  isolate_);
+    for (int i = 0; i < real_size; i++) {
+      PropertyDetails details = descs->GetDetails(i);
+      if ((details.attributes() & filter_) != 0) continue;
+      if (filter_ & ONLY_ALL_CAN_READ) {
+        if (details.kind() != kAccessor) continue;
+        Object* accessors = descs->GetValue(i);
+        if (!accessors->IsAccessorInfo()) continue;
+        if (!AccessorInfo::cast(accessors)->all_can_read()) continue;
+      }
+      Name* key = descs->GetKey(i);
+      if (key->FilterKey(filter_)) continue;
+      AddKey(key, DO_NOT_CONVERT);
+    }
+  } else if (object->IsJSGlobalObject()) {
+    GlobalDictionary::CollectKeysTo(
+        handle(object->global_dictionary(), isolate_), this, filter_);
   } else {
-    if (object->HasFastProperties()) {
-      int limit = object->map()->NumberOfOwnDescriptors();
-      Handle<DescriptorArray> descs(object->map()->instance_descriptors(),
-                                    isolate_);
-      // First collect the strings,
-      int first_symbol =
-          CollectOwnPropertyNamesInternal<true>(object, this, descs, 0, limit);
-      // then the symbols.
-      if (first_symbol != -1) {
-        CollectOwnPropertyNamesInternal<false>(object, this, descs,
-                                               first_symbol, limit);
-      }
-    } else if (object->IsJSGlobalObject()) {
-      GlobalDictionary::CollectKeysTo(
-          handle(object->global_dictionary(), isolate_), this, filter_);
-    } else {
-      NameDictionary::CollectKeysTo(
-          handle(object->property_dictionary(), isolate_), this, filter_);
-    }
-  }
-  // Add the property keys from the interceptor.
-  return GetKeysFromInterceptor<v8::GenericNamedPropertyEnumeratorCallback,
-                                kNamed>(receiver, object, this);
+    NameDictionary::CollectKeysTo(
+        handle(object->property_dictionary(), isolate_), this, filter_);
+  }
 }
 
 // Returns |true| on success, |false| if prototype walking should be stopped,
 // |nothing| if an exception was thrown.
 Maybe<bool> KeyAccumulator::CollectOwnKeys(Handle<JSReceiver> receiver,
                                            Handle<JSObject> object) {
+  NextPrototype();
   // Check access rights if required.
   if (object->IsAccessCheckNeeded() &&
       !isolate_->MayAccess(handle(isolate_->context()), object)) {
@@ -543,8 +692,27 @@
     DCHECK_EQ(OWN_ONLY, type_);
     filter_ = static_cast<PropertyFilter>(filter_ | ONLY_ALL_CAN_READ);
   }
-  MAYBE_RETURN(CollectOwnElementIndices(receiver, object), Nothing<bool>());
-  MAYBE_RETURN(CollectOwnPropertyNames(receiver, object), Nothing<bool>());
+
+  CollectOwnElementIndices(object);
+
+  // Add the element keys from the interceptor.
+  Maybe<bool> success =
+      GetKeysFromInterceptor<v8::IndexedPropertyEnumeratorCallback, kIndexed>(
+          receiver, object, this);
+  MAYBE_RETURN(success, Nothing<bool>());
+
+  if (filter_ == ENUMERABLE_STRINGS) {
+    Handle<FixedArray> enum_keys =
+        KeyAccumulator::GetEnumPropertyKeys(isolate_, object);
+    AddKeys(enum_keys, DO_NOT_CONVERT);
+  } else {
+    CollectOwnPropertyNames(object);
+  }
+
+  // Add the property keys from the interceptor.
+  success = GetKeysFromInterceptor<v8::GenericNamedPropertyEnumeratorCallback,
+                                   kNamed>(receiver, object, this);
+  MAYBE_RETURN(success, Nothing<bool>());
   return Just(true);
 }
 
@@ -654,6 +822,7 @@
       // (No-op, just keep it in |target_keys|.)
     }
   }
+  NextPrototype();  // Prepare for accumulating keys.
   // 15. If extensibleTarget is true and targetNonconfigurableKeys is empty,
   //     then:
   if (extensible_target && nonconfigurable_keys_length == 0) {
@@ -727,6 +896,7 @@
   Handle<FixedArray> keys;
   ASSIGN_RETURN_ON_EXCEPTION_VALUE(
       isolate_, keys, JSReceiver::OwnPropertyKeys(target), Nothing<bool>());
+  NextPrototype();  // Prepare for accumulating keys.
   bool prev_filter_proxy_keys_ = filter_proxy_keys_;
   filter_proxy_keys_ = false;
   Maybe<bool> result = AddKeysFromJSProxy(proxy, keys);