From 5658d8cb21d7fad20fe4a15dc2ebe2fc832b4545 Mon Sep 17 00:00:00 2001 From: Pavel Grafov Date: Thu, 2 Aug 2018 20:18:07 +0100 Subject: Backport: Fix Object.entries/values with changing elements Bug: 111274046 Test: m -j proxy_resolver_v8_unittest && adb sync && adb shell \ /data/nativetest64/proxy_resolver_v8_unittest/proxy_resolver_v8_unittest Change-Id: I705fc512cc5837e9364ed187559cc75d079aa5cb (cherry picked from commit d8be9a10287afed07705ac8af027d6a46d4def99) --- src/elements.cc | 71 +++++++++++++++++++++++++++++++++++++++++++++++++-------- src/elements.h | 18 +++++++-------- 2 files changed, 70 insertions(+), 19 deletions(-) diff --git a/src/elements.cc b/src/elements.cc index d5acb667..501cff7c 100644 --- a/src/elements.cc +++ b/src/elements.cc @@ -511,6 +511,21 @@ static Maybe IndexOfValueSlowPath(Isolate* isolate, return Just(-1); } +// The InternalElementsAccessor is a helper class to expose otherwise protected +// methods to its subclasses. Namely, we don't want to publicly expose methods +// that take an entry (instead of an index) as an argument. +class InternalElementsAccessor : public ElementsAccessor { + public: + explicit InternalElementsAccessor(const char* name) + : ElementsAccessor(name) {} + + virtual uint32_t GetEntryForIndex(Isolate* isolate, JSObject* holder, + FixedArrayBase* backing_store, + uint32_t index) = 0; + + virtual PropertyDetails GetDetails(JSObject* holder, uint32_t entry) = 0; +}; + // Base class for element handler implementations. Contains the // the common logic for objects with different ElementsKinds. // Subclasses must specialize method for which the element @@ -529,10 +544,10 @@ static Maybe IndexOfValueSlowPath(Isolate* isolate, // CRTP to guarantee aggressive compile time optimizations (i.e. inlining and // specialization of SomeElementsAccessor methods). template -class ElementsAccessorBase : public ElementsAccessor { +class ElementsAccessorBase : public InternalElementsAccessor { public: explicit ElementsAccessorBase(const char* name) - : ElementsAccessor(name) { } + : InternalElementsAccessor(name) {} typedef ElementsTraitsParam ElementsTraits; typedef typename ElementsTraitsParam::BackingStore BackingStore; @@ -1014,35 +1029,66 @@ class ElementsAccessorBase : public ElementsAccessor { Isolate* isolate, Handle object, Handle values_or_entries, bool get_entries, int* nof_items, PropertyFilter filter) { - int count = 0; + DCHECK_EQ(*nof_items, 0); KeyAccumulator accumulator(isolate, KeyCollectionMode::kOwnOnly, ALL_PROPERTIES); Subclass::CollectElementIndicesImpl( object, handle(object->elements(), isolate), &accumulator); Handle keys = accumulator.GetKeys(); - for (int i = 0; i < keys->length(); ++i) { + int count = 0; + int i = 0; + Handle original_map(object->map(), isolate); + + for (; i < keys->length(); ++i) { Handle key(keys->get(i), isolate); - Handle value; uint32_t index; if (!key->ToUint32(&index)) continue; + DCHECK_EQ(object->map(), *original_map); uint32_t entry = Subclass::GetEntryForIndexImpl( isolate, *object, object->elements(), index, filter); if (entry == kMaxUInt32) continue; PropertyDetails details = Subclass::GetDetailsImpl(*object, entry); + Handle value; if (details.kind() == kData) { value = Subclass::GetImpl(isolate, object->elements(), entry); } else { + // This might modify the elements and/or change the elements kind. LookupIterator it(isolate, object, index, LookupIterator::OWN); ASSIGN_RETURN_ON_EXCEPTION_VALUE( isolate, value, Object::GetProperty(&it), Nothing()); } - if (get_entries) { - value = MakeEntryPair(isolate, index, value); + if (get_entries) value = MakeEntryPair(isolate, index, value); + values_or_entries->set(count++, *value); + if (object->map() != *original_map) break; + } + + // Slow path caused by changes in elements kind during iteration. + for (; i < keys->length(); i++) { + Handle key(keys->get(i), isolate); + uint32_t index; + if (!key->ToUint32(&index)) continue; + + if (filter & ONLY_ENUMERABLE) { + InternalElementsAccessor* accessor = + reinterpret_cast( + object->GetElementsAccessor()); + uint32_t entry = accessor->GetEntryForIndex(isolate, *object, + object->elements(), index); + if (entry == kMaxUInt32) continue; + PropertyDetails details = accessor->GetDetails(*object, entry); + if (!details.IsEnumerable()) continue; } + + Handle value; + LookupIterator it(isolate, object, index, LookupIterator::OWN); + ASSIGN_RETURN_ON_EXCEPTION_VALUE(isolate, value, Object::GetProperty(&it), + Nothing()); + + if (get_entries) value = MakeEntryPair(isolate, index, value); values_or_entries->set(count++, *value); } @@ -1623,12 +1669,13 @@ class DictionaryElementsAccessor return result; } } - + Handle original_map(receiver->map(), isolate); Handle dictionary( SeededNumberDictionary::cast(receiver->elements()), isolate); // Iterate through entire range, as accessing elements out of order is // observable for (uint32_t k = start_from; k < length; ++k) { + DCHECK_EQ(receiver->map(), *original_map); int entry = dictionary->FindEntry(isolate, k); if (entry == SeededNumberDictionary::kNotFound) { if (search_for_hole) return Just(true); @@ -1690,11 +1737,13 @@ class DictionaryElementsAccessor uint32_t start_from, uint32_t length) { DCHECK(JSObject::PrototypeHasNoElements(isolate, *receiver)); + Handle original_map(receiver->map(), isolate); Handle dictionary( SeededNumberDictionary::cast(receiver->elements()), isolate); // Iterate through entire range, as accessing elements out of order is // observable. for (uint32_t k = start_from; k < length; ++k) { + DCHECK_EQ(receiver->map(), *original_map); int entry = dictionary->FindEntry(isolate, k); if (entry == SeededNumberDictionary::kNotFound) { continue; @@ -3170,12 +3219,13 @@ class SloppyArgumentsElementsAccessor Handle value, uint32_t start_from, uint32_t length) { DCHECK(JSObject::PrototypeHasNoElements(isolate, *object)); - Handle original_map = handle(object->map(), isolate); + Handle original_map(object->map(), isolate); Handle parameter_map(FixedArray::cast(object->elements()), isolate); bool search_for_hole = value->IsUndefined(isolate); for (uint32_t k = start_from; k < length; ++k) { + DCHECK_EQ(object->map(), *original_map); uint32_t entry = GetEntryForIndexImpl(isolate, *object, *parameter_map, k, ALL_PROPERTIES); if (entry == kMaxUInt32) { @@ -3212,11 +3262,12 @@ class SloppyArgumentsElementsAccessor Handle value, uint32_t start_from, uint32_t length) { DCHECK(JSObject::PrototypeHasNoElements(isolate, *object)); - Handle original_map = handle(object->map(), isolate); + Handle original_map(object->map(), isolate); Handle parameter_map(FixedArray::cast(object->elements()), isolate); for (uint32_t k = start_from; k < length; ++k) { + DCHECK_EQ(object->map(), *original_map); uint32_t entry = GetEntryForIndexImpl(isolate, *object, *parameter_map, k, ALL_PROPERTIES); if (entry == kMaxUInt32) { diff --git a/src/elements.h b/src/elements.h index 28635d5b..376bcbe8 100644 --- a/src/elements.h +++ b/src/elements.h @@ -54,7 +54,6 @@ class ElementsAccessor { virtual Handle Get(Handle holder, uint32_t entry) = 0; - virtual PropertyDetails GetDetails(JSObject* holder, uint32_t entry) = 0; virtual bool HasAccessors(JSObject* holder) = 0; virtual uint32_t NumberOfElements(JSObject* holder) = 0; @@ -65,9 +64,6 @@ class ElementsAccessor { // element that is non-deletable. virtual void SetLength(Handle holder, uint32_t new_length) = 0; - // Deletes an element in an object. - virtual void Delete(Handle holder, uint32_t entry) = 0; - // If kCopyToEnd is specified as the copy_size to CopyElements, it copies all // of elements from source after source_start to the destination array. static const int kCopyToEnd = -1; @@ -124,11 +120,6 @@ class ElementsAccessor { virtual void Set(Handle holder, uint32_t entry, Object* value) = 0; - virtual void Reconfigure(Handle object, - Handle backing_store, uint32_t entry, - Handle value, - PropertyAttributes attributes) = 0; - virtual void Add(Handle object, uint32_t index, Handle value, PropertyAttributes attributes, uint32_t new_capacity) = 0; @@ -193,6 +184,15 @@ class ElementsAccessor { FixedArrayBase* backing_store, uint32_t index) = 0; + virtual PropertyDetails GetDetails(JSObject* holder, uint32_t entry) = 0; + virtual void Reconfigure(Handle object, + Handle backing_store, uint32_t entry, + Handle value, + PropertyAttributes attributes) = 0; + + // Deletes an element in an object. + virtual void Delete(Handle holder, uint32_t entry) = 0; + // NOTE: this method violates the handlified function signature convention: // raw pointer parameter |source_holder| in the function that allocates. // This is done intentionally to avoid ArrayConcat() builtin performance -- cgit v1.2.3