From 6e0573c6fff1c3041bab106d1197ab1b64aa9a6a Mon Sep 17 00:00:00 2001 From: cbruni Date: Thu, 21 Jan 2016 09:47:53 -0800 Subject: [PATCH] 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 | 7 +- src/key-accumulator.cc | 37 +--- src/key-accumulator.h | 13 -- src/messages.cc | 3 +- src/objects.cc | 164 ++++++++---------- src/objects.h | 15 +- .../regress-705-shadowed_properties.js | 57 ------ 7 files changed, 84 insertions(+), 212 deletions(-) delete mode 100644 test/mjsunit/regress/regress-705-shadowed_properties.js diff --git a/src/elements.cc b/src/elements.cc index 839d833c94..16e8b067cf 100644 --- a/src/elements.cc +++ b/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); } diff --git a/src/key-accumulator.cc b/src/key-accumulator.cc index c259edffe9..e7a9c3cceb 100644 --- a/src/key-accumulator.cc +++ b/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 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 key, bool KeyAccumulator::AddSymbolKey(Handle 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(key, isolate_)); -} - - -void KeyAccumulator::HideKey(Handle 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 key) { - if (hidden_keys_.is_null()) return false; - return OrderedHashSet::HasKey(hidden_keys_, key); -} - - } // namespace internal } // namespace v8 diff --git a/src/key-accumulator.h b/src/key-accumulator.h index fa39c1996d..8a4d886f51 100644 --- a/src/key-accumulator.h +++ b/src/key-accumulator.h @@ -5,8 +5,6 @@ #ifndef V8_KEY_ACCUMULATOR_H_ #define V8_KEY_ACCUMULATOR_H_ -#include - #include "src/isolate.h" #include "src/objects.h" @@ -47,10 +45,6 @@ class KeyAccumulator final BASE_EMBEDDED { void AddKeysFromProxy(Handle array); Maybe AddKeysFromProxy(Handle proxy, Handle keys); void AddElementKeysFromInterceptor(Handle array); - void HideKey(uint32_t key); - void HideKey(Object* key); - void HideKey(Handle 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 key, AddKeyConversion convert); bool AddSymbolKey(Handle array); void SortCurrentElementsListRemoveDuplicates(); - bool IsKeyHidden(Handle 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 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 hidden_keys_; - std::set 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 diff --git a/src/messages.cc b/src/messages.cc index ab1a3e793c..0365ec8802 100644 --- a/src/messages.cc +++ b/src/messages.cc @@ -250,8 +250,7 @@ Handle CallSite::GetMethodName() { if (!current->IsJSObject()) break; Handle current_obj = Handle::cast(current); if (current_obj->IsAccessCheckNeeded()) break; - Handle keys = - JSObject::GetOwnEnumPropertyKeys(current_obj, false); + Handle keys = JSObject::GetEnumPropertyKeys(current_obj, false); for (int i = 0; i < keys->length(); i++) { HandleScope inner_scope(isolate_); if (!keys->get(i)->IsName()) continue; diff --git a/src/objects.cc b/src/objects.cc index 4885ed1ee2..6c2ff15bed 100644 --- a/src/objects.cc +++ b/src/objects.cc @@ -8108,8 +8108,8 @@ MaybeHandle JSObjectWalkVisitor::StructureWalk( PropertyFilter filter = static_cast( ONLY_WRITABLE | ONLY_ENUMERABLE | ONLY_CONFIGURABLE); KeyAccumulator accumulator(isolate, filter); - accumulator.Prepare(); - copy->CollectOwnPropertyKeys(&accumulator, filter); + accumulator.NextPrototype(); + copy->CollectOwnPropertyNames(&accumulator, filter); Handle names = accumulator.GetKeys(); for (int i = 0; i < names->length(); i++) { DCHECK(names->get(i)->IsName()); @@ -8448,9 +8448,10 @@ static Handle ReduceFixedArrayTo( namespace { -Handle GetFastEnumPropertyKeys(Isolate* isolate, JSObject* object, +Handle GetFastEnumPropertyKeys(Isolate* isolate, + Handle object, bool cache_enum_length) { - Handle map(object->map(), isolate); + Handle map(object->map()); Handle descs = Handle(map->instance_descriptors(), isolate); int own_property_count = map->EnumLength(); @@ -8524,13 +8525,30 @@ Handle GetFastEnumPropertyKeys(Isolate* isolate, JSObject* object, } // namespace -Handle JSObject::GetOwnEnumPropertyKeys(Handle 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 JSObject::GetEnumPropertyKeys(Handle object, + bool cache_enum_length) { + Isolate* isolate = object->GetIsolate(); + if (object->HasFastProperties()) { + return GetFastEnumPropertyKeys(isolate, object, cache_enum_length); + } else if (object->IsJSGlobalObject()) { + Handle dictionary(object->global_dictionary()); + int length = dictionary->NumberOfEnumElements(); + if (length == 0) { + return Handle(isolate->heap()->empty_fixed_array()); + } + Handle storage = isolate->factory()->NewFixedArray(length); + dictionary->CopyEnumKeysTo(*storage); + return storage; + } else { + Handle dictionary(object->property_dictionary()); + int length = dictionary->NumberOfEnumElements(); + if (length == 0) { + return Handle(isolate->heap()->empty_fixed_array()); + } + Handle storage = isolate->factory()->NewFixedArray(length); + dictionary->CopyEnumKeysTo(*storage); + return storage; + } } @@ -8614,7 +8632,30 @@ static Maybe GetKeysFromJSObject(Isolate* isolate, isolate, receiver, object, *filter, accumulator); MAYBE_RETURN(success, Nothing()); - 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 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 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 = GetKeysFromInterceptorisolate(); - Handle 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 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 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 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 void Dictionary::CollectKeysTo( Handle > dictionary, KeyAccumulator* keys, PropertyFilter filter) { - if (dictionary->NumberOfElements() == 0) return; int capacity = dictionary->Capacity(); Handle array = keys->isolate()->factory()->NewFixedArray(dictionary->NumberOfElements()); int array_size = 0; - std::vector hidden_key_indices; { DisallowHeapAllocation no_gc; Dictionary* 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::CollectKeysTo( Smi** start = reinterpret_cast(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)); diff --git a/src/objects.h b/src/objects.h index 8f411c1a73..795b429a92 100644 --- a/src/objects.h +++ b/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 object, KeyAccumulator* keys, PropertyFilter filter); - static Handle GetOwnEnumPropertyKeys(Handle object, - bool cache_result); + + static Handle GetEnumPropertyKeys(Handle 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 object, Handle value, bool from_javascript, ShouldThrow should_throw); - void CollectFastPropertyKeysTo(KeyAccumulator* keys, PropertyFilter filter, - JSReceiver::KeyCollectionType type); - DISALLOW_IMPLICIT_CONSTRUCTORS(JSObject); }; diff --git a/test/mjsunit/regress/regress-705-shadowed_properties.js b/test/mjsunit/regress/regress-705-shadowed_properties.js deleted file mode 100644 index f5b9e8262e..0000000000 --- a/test/mjsunit/regress/regress-705-shadowed_properties.js +++ /dev/null @@ -1,57 +0,0 @@ -// 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));