Revert of [runtime] Do not use the enum-cache for non-prototype objects....

Revert of [runtime] Do not use the enum-cache for non-prototype objects. (patchset #10 id:180001 of https://codereview.chromium.org/1608523002/ )

Reason for revert:
tanks for-in significantly

Original issue's description:
> [runtime] Do not use the enum-cache for keys retrieval.
>
> Currently we fail to properly handle shadowed properties. If the
> receiver defines a non-enumerable property that reappears on the
> prototype as enumerable it incorrectly shows up in [[Enumerate]].
> By extending the KeyAccumulator to track non-enumerable properties
> we can now properly filter them out when seeing them further up in
> the prototype-chain.
>
> BUG=v8:705
> LOG=y
>
> Committed: https://crrev.com/ed24dfe80d1da0827b8571839ee52c03ad09c9c7
> Cr-Commit-Position: refs/heads/master@{#33405}

TBR=jkummerow@chromium.org,bmeurer@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=v8:705
LOG=n

Review URL: https://codereview.chromium.org/1619803003

Cr-Commit-Position: refs/heads/master@{#33443}

src/elements.cc

@@ -1153,12 +1153,7 @@ class DictionaryElementsAccessor
         if (!AccessorInfo::cast(accessors)->all_can_read()) continue;
       }
       PropertyAttributes attr = details.attributes();
-      if ((attr & filter) != 0) {
-        if (attr & PropertyAttributes::DONT_ENUM) {
-          keys->HideKey(index);
-        }
-        continue;
-      }
+      if ((attr & filter) != 0) continue;
       keys->AddKey(index);
     }
 

src/key-accumulator.cc

@@ -138,11 +138,10 @@ bool KeyAccumulator::AddKey(uint32_t key) { return AddIntegerKey(key); }
 
 
 bool KeyAccumulator::AddIntegerKey(uint32_t key) {
-  if (IsKeyHidden(key)) return false;
   // Make sure we do not add keys to a proxy-level (see AddKeysFromProxy).
   // We mark proxy-levels with a negative length
   DCHECK_LE(0, level_string_length_);
-  // Binary search over all but the last level. The last level might not be
+  // 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;
@@ -155,10 +154,11 @@ bool KeyAccumulator::AddIntegerKey(uint32_t key) {
 
 bool KeyAccumulator::AddStringKey(Handle<Object> key,
                                   AddKeyConversion convert) {
-  if (IsKeyHidden(key)) return false;
   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);
   }
@@ -175,7 +175,6 @@ bool KeyAccumulator::AddStringKey(Handle<Object> key,
 
 
 bool KeyAccumulator::AddSymbolKey(Handle<Object> key) {
-  if (IsKeyHidden(key)) return false;
   if (symbol_properties_.is_null()) {
     symbol_properties_ = OrderedHashSet::Allocate(isolate_, 16);
   }
@@ -300,9 +299,6 @@ void KeyAccumulator::SortCurrentElementsList() {
 }
 
 
-void KeyAccumulator::Prepare() { NextPrototype(); }
-
-
 void KeyAccumulator::NextPrototype() {
   // Store the protoLength on the first call of this method.
   if (!elements_.empty()) {
@@ -315,32 +311,5 @@ void KeyAccumulator::NextPrototype() {
 }
 
 
-void KeyAccumulator::HideKey(uint32_t key) { hidden_element_keys_.insert(key); }
-
-
-void KeyAccumulator::HideKey(Object* key) {
-  HideKey(Handle<Object>(key, isolate_));
-}
-
-
-void KeyAccumulator::HideKey(Handle<Object> key) {
-  if (hidden_keys_.is_null()) {
-    hidden_keys_ = OrderedHashSet::Allocate(isolate_, 8);
-  }
-  hidden_keys_ = OrderedHashSet::Add(hidden_keys_, key);
-}
-
-
-bool KeyAccumulator::IsKeyHidden(uint32_t key) {
-  return hidden_element_keys_.count(key);
-}
-
-
-bool KeyAccumulator::IsKeyHidden(Handle<Object> key) {
-  if (hidden_keys_.is_null()) return false;
-  return OrderedHashSet::HasKey(hidden_keys_, key);
-}
-
-
 }  // namespace internal
 }  // namespace v8

src/key-accumulator.h

@@ -5,8 +5,6 @@
 #ifndef V8_KEY_ACCUMULATOR_H_
 #define V8_KEY_ACCUMULATOR_H_
 
-#include <set>
-
 #include "src/isolate.h"
 #include "src/objects.h"
 
@@ -47,10 +45,6 @@ class KeyAccumulator final BASE_EMBEDDED {
   void AddKeysFromProxy(Handle<JSObject> array);
   Maybe<bool> AddKeysFromProxy(Handle<JSProxy> proxy, Handle<FixedArray> keys);
   void AddElementKeysFromInterceptor(Handle<JSObject> array);
-  void HideKey(uint32_t key);
-  void HideKey(Object* key);
-  void HideKey(Handle<Object> key);
-  void Prepare();
   // Jump to the next level, pushing the current |levelLength_| to
   // |levelLengths_| and adding a new list to |elements_|.
   void NextPrototype();
@@ -65,8 +59,6 @@ class KeyAccumulator final BASE_EMBEDDED {
   bool AddStringKey(Handle<Object> key, AddKeyConversion convert);
   bool AddSymbolKey(Handle<Object> array);
   void SortCurrentElementsListRemoveDuplicates();
-  bool IsKeyHidden(Handle<Object> key);
-  bool IsKeyHidden(uint32_t key);
 
   Isolate* isolate_;
   PropertyFilter filter_;
@@ -81,11 +73,6 @@ class KeyAccumulator final BASE_EMBEDDED {
   // |symbol_properties_| contains the unique Symbol property keys for all
   // levels in insertion order per level.
   Handle<OrderedHashSet> symbol_properties_;
-  // |hidden_keys_| keeps track of the non-enumerable property keys to prevent
-  // them from being added to the accumulator if they reappear higher up in the
-  // prototype-chain.
-  Handle<OrderedHashSet> hidden_keys_;
-  std::set<uint32_t> hidden_element_keys_;
   // |length_| keeps track of the total number of all element and property keys.
   int length_ = 0;
   // |levelLength_| keeps track of the number of String keys in the current

src/messages.cc

@@ -250,8 +250,7 @@ Handle<Object> CallSite::GetMethodName() {
     if (!current->IsJSObject()) break;
     Handle<JSObject> current_obj = Handle<JSObject>::cast(current);
     if (current_obj->IsAccessCheckNeeded()) break;
-    Handle<FixedArray> keys =
-        JSObject::GetOwnEnumPropertyKeys(current_obj, false);
+    Handle<FixedArray> keys = JSObject::GetEnumPropertyKeys(current_obj, false);
     for (int i = 0; i < keys->length(); i++) {
       HandleScope inner_scope(isolate_);
       if (!keys->get(i)->IsName()) continue;

src/objects.cc

@@ -8108,8 +8108,8 @@ MaybeHandle<JSObject> JSObjectWalkVisitor<ContextObject>::StructureWalk(
       PropertyFilter filter = static_cast<PropertyFilter>(
           ONLY_WRITABLE | ONLY_ENUMERABLE | ONLY_CONFIGURABLE);
       KeyAccumulator accumulator(isolate, filter);
-      accumulator.Prepare();
-      copy->CollectOwnPropertyKeys(&accumulator, filter);
+      accumulator.NextPrototype();
+      copy->CollectOwnPropertyNames(&accumulator, filter);
       Handle<FixedArray> names = accumulator.GetKeys();
       for (int i = 0; i < names->length(); i++) {
         DCHECK(names->get(i)->IsName());
@@ -8448,9 +8448,10 @@ static Handle<FixedArray> ReduceFixedArrayTo(
 
 namespace {
 
-Handle<FixedArray> GetFastEnumPropertyKeys(Isolate* isolate, JSObject* object,
+Handle<FixedArray> GetFastEnumPropertyKeys(Isolate* isolate,
+                                           Handle<JSObject> object,
                                            bool cache_enum_length) {
-  Handle<Map> map(object->map(), isolate);
+  Handle<Map> map(object->map());
   Handle<DescriptorArray> descs =
       Handle<DescriptorArray>(map->instance_descriptors(), isolate);
   int own_property_count = map->EnumLength();
@@ -8524,13 +8525,30 @@ Handle<FixedArray> GetFastEnumPropertyKeys(Isolate* isolate, JSObject* object,
 }  // namespace
 
 
-Handle<FixedArray> JSObject::GetOwnEnumPropertyKeys(Handle<JSObject> object,
-                                                    bool cache_enum_length) {
-  PropertyFilter filter = PropertyFilter::ENUMERABLE_STRINGS;
-  KeyAccumulator keys(object->GetIsolate(), filter);
-  keys.Prepare();
-  object->CollectOwnPropertyKeys(&keys, filter);
-  return keys.GetKeys();
+Handle<FixedArray> JSObject::GetEnumPropertyKeys(Handle<JSObject> object,
+                                                 bool cache_enum_length) {
+  Isolate* isolate = object->GetIsolate();
+  if (object->HasFastProperties()) {
+    return GetFastEnumPropertyKeys(isolate, object, cache_enum_length);
+  } else if (object->IsJSGlobalObject()) {
+    Handle<GlobalDictionary> dictionary(object->global_dictionary());
+    int length = dictionary->NumberOfEnumElements();
+    if (length == 0) {
+      return Handle<FixedArray>(isolate->heap()->empty_fixed_array());
+    }
+    Handle<FixedArray> storage = isolate->factory()->NewFixedArray(length);
+    dictionary->CopyEnumKeysTo(*storage);
+    return storage;
+  } else {
+    Handle<NameDictionary> dictionary(object->property_dictionary());
+    int length = dictionary->NumberOfEnumElements();
+    if (length == 0) {
+      return Handle<FixedArray>(isolate->heap()->empty_fixed_array());
+    }
+    Handle<FixedArray> storage = isolate->factory()->NewFixedArray(length);
+    dictionary->CopyEnumKeysTo(*storage);
+    return storage;
+  }
 }
 
 
@@ -8614,7 +8632,30 @@ static Maybe<bool> GetKeysFromJSObject(Isolate* isolate,
           isolate, receiver, object, *filter, accumulator);
   MAYBE_RETURN(success, Nothing<bool>());
 
-  object->CollectOwnPropertyKeys(accumulator, *filter, type);
+  if (*filter == ENUMERABLE_STRINGS) {
+    // We can cache the computed property keys if access checks are
+    // not needed and no interceptors are involved.
+    //
+    // We do not use the cache if the object has elements and
+    // therefore it does not make sense to cache the property names
+    // for arguments objects.  Arguments objects will always have
+    // elements.
+    // Wrapped strings have elements, but don't have an elements
+    // array or dictionary.  So the fast inline test for whether to
+    // use the cache says yes, so we should not create a cache.
+    Handle<JSFunction> arguments_function(
+        JSFunction::cast(isolate->sloppy_arguments_map()->GetConstructor()));
+    bool cache_enum_length =
+        ((object->map()->GetConstructor() != *arguments_function) &&
+         !object->IsJSValue() && !object->IsAccessCheckNeeded() &&
+         !object->HasNamedInterceptor() && !object->HasIndexedInterceptor());
+    // Compute the property keys and cache them if possible.
+    Handle<FixedArray> enum_keys =
+        JSObject::GetEnumPropertyKeys(object, cache_enum_length);
+    accumulator->AddKeys(enum_keys);
+  } else {
+    object->CollectOwnPropertyNames(accumulator, *filter);
+  }
 
   // Add the property keys from the interceptor.
   success = GetKeysFromInterceptor<v8::GenericNamedPropertyEnumeratorCallback,
@@ -16360,74 +16401,24 @@ void FixedArray::SortPairs(FixedArray* numbers, uint32_t len) {
 }
 
 
-namespace {
-
-void CollectEnumCacheTo(KeyAccumulator* keys, JSObject* object) {
-  // We can cache the computed property keys if access checks are
-  // not needed and no interceptors are involved.
-  //
-  // We do not use the cache if the object has elements and
-  // therefore it does not make sense to cache the property names
-  // for arguments objects.  Arguments objects will always have
-  // elements.
-  // Wrapped strings have elements, but don't have an elements
-  // array or dictionary.  So the fast inline test for whether to
-  // use the cache says yes, so we should not create a cache.
-  Isolate* isolate = keys->isolate();
-  Handle<JSFunction> arguments_function(
-      JSFunction::cast(isolate->sloppy_arguments_map()->GetConstructor()));
-  bool cache_enum_length =
-      ((object->map()->GetConstructor() != *arguments_function) &&
-       !object->IsJSValue() && !object->IsAccessCheckNeeded() &&
-       !object->HasNamedInterceptor() && !object->HasIndexedInterceptor());
-  // Compute the property keys and cache them if possible.
-  Handle<FixedArray> enum_keys =
-      GetFastEnumPropertyKeys(isolate, object, cache_enum_length);
-  keys->AddKeys(enum_keys);
-}
-
-}  // namespace
-
-
-void JSObject::CollectFastPropertyKeysTo(KeyAccumulator* keys,
-                                         PropertyFilter filter,
-                                         JSReceiver::KeyCollectionType type) {
-  // Avoid using the enum cache if we have to walk the prototype chain due
-  // to shadowing properties.
-  if (filter == ENUMERABLE_STRINGS && type == JSReceiver::OWN_ONLY) {
-    CollectEnumCacheTo(keys, this);
-    return;
-  }
-
-  int real_size = map()->NumberOfOwnDescriptors();
-  Handle<DescriptorArray> descs(map()->instance_descriptors());
-  for (int i = 0; i < real_size; i++) {
-    PropertyDetails details = descs->GetDetails(i);
-    if ((details.attributes() & filter) != 0) {
-      if (type == JSReceiver::OWN_ONLY) continue;
-      if (details.attributes() & DONT_ENUM) {
-        keys->HideKey(descs->GetKey(i));
+void JSObject::CollectOwnPropertyNames(KeyAccumulator* keys,
+                                       PropertyFilter filter) {
+  if (HasFastProperties()) {
+    int real_size = map()->NumberOfOwnDescriptors();
+    Handle<DescriptorArray> descs(map()->instance_descriptors());
+    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;
       }
-      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;
+      keys->AddKey(key);
     }
-    Name* key = descs->GetKey(i);
-    if (key->FilterKey(filter)) continue;
-    keys->AddKey(key);
-  }
-}
-
-
-void JSObject::CollectOwnPropertyKeys(KeyAccumulator* keys,
-                                      PropertyFilter filter,
-                                      JSReceiver::KeyCollectionType type) {
-  if (HasFastProperties()) {
-    CollectFastPropertyKeysTo(keys, filter, type);
   } else if (IsJSGlobalObject()) {
     GlobalDictionary::CollectKeysTo(handle(global_dictionary()), keys, filter);
   } else {
@@ -18461,27 +18452,20 @@ template <typename Derived, typename Shape, typename Key>
 void Dictionary<Derived, Shape, Key>::CollectKeysTo(
     Handle<Dictionary<Derived, Shape, Key> > dictionary, KeyAccumulator* keys,
     PropertyFilter filter) {
-  if (dictionary->NumberOfElements() == 0) return;
   int capacity = dictionary->Capacity();
   Handle<FixedArray> array =
       keys->isolate()->factory()->NewFixedArray(dictionary->NumberOfElements());
   int array_size = 0;
-  std::vector<int> hidden_key_indices;
 
   {
     DisallowHeapAllocation no_gc;
     Dictionary<Derived, Shape, Key>* raw_dict = *dictionary;
     for (int i = 0; i < capacity; i++) {
-      Object* key = raw_dict->KeyAt(i);
-      if (!raw_dict->IsKey(key) || key->FilterKey(filter)) continue;
+      Object* k = raw_dict->KeyAt(i);
+      if (!raw_dict->IsKey(k) || k->FilterKey(filter)) continue;
       if (raw_dict->IsDeleted(i)) continue;
       PropertyDetails details = raw_dict->DetailsAt(i);
-      if ((details.attributes() & filter) != 0) {
-        if (details.attributes() & DONT_ENUM) {
-          hidden_key_indices.push_back(i);
-        }
-        continue;
-      }
+      if ((details.attributes() & filter) != 0) continue;
       if (filter & ONLY_ALL_CAN_READ) {
         if (details.kind() != kAccessor) continue;
         Object* accessors = raw_dict->ValueAt(i);
@@ -18498,9 +18482,7 @@ void Dictionary<Derived, Shape, Key>::CollectKeysTo(
     Smi** start = reinterpret_cast<Smi**>(array->GetFirstElementAddress());
     std::sort(start, start + array_size, cmp);
   }
-  for (uint32_t i = 0; i < hidden_key_indices.size(); i++) {
-    keys->HideKey(dictionary->KeyAt(hidden_key_indices[i]));
-  }
+
   for (int i = 0; i < array_size; i++) {
     int index = Smi::cast(array->get(i))->value();
     keys->AddKey(dictionary->KeyAt(index));

src/objects.h

@@ -2284,6 +2284,9 @@ class JSObject: public JSReceiver {
   inline void SetInternalField(int index, Object* value);
   inline void SetInternalField(int index, Smi* value);
 
+  void CollectOwnPropertyNames(KeyAccumulator* keys,
+                               PropertyFilter filter = ALL_PROPERTIES);
+
   // Returns the number of properties on this object filtering out properties
   // with the specified attributes (ignoring interceptors).
   // TODO(jkummerow): Deprecated, only used by Object.observe.
@@ -2293,15 +2296,12 @@ class JSObject: public JSReceiver {
   // TODO(jkummerow): Deprecated, only used by Object.observe.
   int GetOwnElementKeys(FixedArray* storage, PropertyFilter filter);
 
-  void CollectOwnPropertyKeys(
-      KeyAccumulator* keys, PropertyFilter filter = ALL_PROPERTIES,
-      JSReceiver::KeyCollectionType type = JSReceiver::OWN_ONLY);
-
   static void CollectOwnElementKeys(Handle<JSObject> object,
                                     KeyAccumulator* keys,
                                     PropertyFilter filter);
-  static Handle<FixedArray> GetOwnEnumPropertyKeys(Handle<JSObject> object,
-                                                   bool cache_result);
+
+  static Handle<FixedArray> GetEnumPropertyKeys(Handle<JSObject> object,
+                                                bool cache_result);
 
   // Returns a new map with all transitions dropped from the object's current
   // map and the ElementsKind set.
@@ -2555,9 +2555,6 @@ class JSObject: public JSReceiver {
       Handle<JSObject> object, Handle<Object> value, bool from_javascript,
       ShouldThrow should_throw);
 
-  void CollectFastPropertyKeysTo(KeyAccumulator* keys, PropertyFilter filter,
-                                 JSReceiver::KeyCollectionType type);
-
   DISALLOW_IMPLICIT_CONSTRUCTORS(JSObject);
 };
 

test/mjsunit/regress/regress-705-shadowed_properties.js

-// Copyright 2016 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.
-
-function allKeys(object) {
-  var keys = [];
-  for (var key in object) {
-    keys.push(key);
-  }
-  return keys;
-}
-
-var a = { a1: true, a2: true};
-var b = { b1: true, b2: true};
-a.__proto__ = b;
-
-assertEquals(['a1', 'a2', 'b1', 'b2'], allKeys(a));
-assertEquals(['b1', 'b2'], allKeys(b));
-
-// Adding a non-enumerable property to either a or b shouldn't change
-// the result.
-var propertyDescriptor = {
-  enumerable: false,
-  configurable: true,
-  writable: true,
-  value: true
-};
-
-Object.defineProperty(a, 'a3', propertyDescriptor);
-assertEquals(['a1', 'a2', 'b1', 'b2'], allKeys(a));
-assertEquals(['b1', 'b2'], allKeys(b));
-
-Object.defineProperty(b, 'b3', propertyDescriptor);
-assertEquals(['a1', 'a2', 'b1', 'b2'], allKeys(a));
-assertEquals(['b1', 'b2'], allKeys(b));
-
-// A non-enumerable property shadows an enumerable version on the prototype
-// chain.
-b['a3'] = true;
-assertEquals(['a1', 'a2', 'b1', 'b2'], allKeys(a));
-assertEquals(['b1', 'b2', 'a3'], allKeys(b));
-
-// Try the same with indexed-properties.
-var aIndex  = {0:true, 1:true};
-var bIndex = {10:true, 11:true};
-aIndex.__proto__= bIndex;
-assertEquals(['0', '1', '10', '11'], allKeys(aIndex));
-assertEquals(['10', '11'], allKeys(bIndex));
-
-Object.defineProperty(aIndex, 2, propertyDescriptor);
-Object.defineProperty(bIndex, 12, propertyDescriptor);
-assertEquals(['0', '1', '10', '11'], allKeys(aIndex));
-assertEquals(['10', '11'], allKeys(bIndex));
-
-bIndex[2] = 2;
-assertEquals(['0', '1', '10', '11'], allKeys(aIndex));
-assertEquals(['2', '10', '11'], allKeys(bIndex));