[elements] Fix Object.entries/values with changing elements

Drive-by-cleanup:
- Add InternalElementsAccessor to expose protected instance methods
  to ElementsAccessor subclasses.
- Make some more ElementsAccessor methods protected that take the
  raw entry as parameter.

Bug: chromium:798644
Change-Id: Iffd00f1953461e8dd22c123e62298410fb6e049c
Reviewed-on: https://chromium-review.googlesource.com/856816
Commit-Queue: Camillo Bruni <cbruni@chromium.org>
Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Cr-Commit-Position: refs/heads/master@{#50480}

src/elements.cc

@@ -519,6 +519,21 @@ static Maybe<int64_t> IndexOfValueSlowPath(Isolate* isolate,
   return Just<int64_t>(-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
@@ -537,10 +552,10 @@ static Maybe<int64_t> IndexOfValueSlowPath(Isolate* isolate,
 // CRTP to guarantee aggressive compile time optimizations (i.e.  inlining and
 // specialization of SomeElementsAccessor methods).
 template <typename Subclass, typename ElementsTraitsParam>
-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;
@@ -1052,35 +1067,65 @@ class ElementsAccessorBase : public ElementsAccessor {
       Isolate* isolate, Handle<JSObject> object,
       Handle<FixedArray> 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<FixedArray> keys = accumulator.GetKeys();
 
-    for (int i = 0; i < keys->length(); ++i) {
+    int count = 0;
+    int i = 0;
+    Handle<Map> original_map(object->map(), isolate);
+
+    for (; i < keys->length(); ++i) {
       Handle<Object> key(keys->get(i), isolate);
-      Handle<Object> 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<Object> 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<bool>());
       }
-      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<Object> key(keys->get(i), isolate);
+      uint32_t index;
+      if (!key->ToUint32(&index)) continue;
+
+      if (filter & ONLY_ENUMERABLE) {
+        InternalElementsAccessor* accessor =
+            reinterpret_cast<InternalElementsAccessor*>(
+                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<Object> value;
+      LookupIterator it(isolate, object, index, LookupIterator::OWN);
+      ASSIGN_RETURN_ON_EXCEPTION_VALUE(isolate, value, Object::GetProperty(&it),
+                                       Nothing<bool>());
+
+      if (get_entries) value = MakeEntryPair(isolate, index, value);
       values_or_entries->set(count++, *value);
     }
 
@@ -1710,12 +1755,13 @@ class DictionaryElementsAccessor
         return result;
       }
     }
-
+    Handle<Map> original_map(receiver->map(), isolate);
     Handle<NumberDictionary> dictionary(
         NumberDictionary::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 == NumberDictionary::kNotFound) {
         if (search_for_hole) return Just(true);
@@ -1780,15 +1826,15 @@ class DictionaryElementsAccessor
                                          uint32_t start_from, uint32_t length) {
     DCHECK(JSObject::PrototypeHasNoElements(isolate, *receiver));
 
+    Handle<Map> original_map(receiver->map(), isolate);
     Handle<NumberDictionary> dictionary(
         NumberDictionary::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 == NumberDictionary::kNotFound) {
-        continue;
-      }
+      if (entry == NumberDictionary::kNotFound) continue;
 
       PropertyDetails details = GetDetailsImpl(*dictionary, entry);
       switch (details.kind()) {
@@ -3698,12 +3744,13 @@ class SloppyArgumentsElementsAccessor
                                        Handle<Object> value,
                                        uint32_t start_from, uint32_t length) {
     DCHECK(JSObject::PrototypeHasNoElements(isolate, *object));
-    Handle<Map> original_map = handle(object->map(), isolate);
+    Handle<Map> original_map(object->map(), isolate);
     Handle<SloppyArgumentsElements> elements(
         SloppyArgumentsElements::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, *elements, k, ALL_PROPERTIES);
       if (entry == kMaxUInt32) {
@@ -3739,11 +3786,12 @@ class SloppyArgumentsElementsAccessor
                                          Handle<Object> value,
                                          uint32_t start_from, uint32_t length) {
     DCHECK(JSObject::PrototypeHasNoElements(isolate, *object));
-    Handle<Map> original_map = handle(object->map(), isolate);
+    Handle<Map> original_map(object->map(), isolate);
     Handle<SloppyArgumentsElements> elements(
         SloppyArgumentsElements::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, *elements, k, ALL_PROPERTIES);
       if (entry == kMaxUInt32) {

src/elements.h

@@ -54,9 +54,10 @@ class ElementsAccessor {
   // typed array elements.
   virtual bool HasEntry(JSObject* holder, uint32_t entry) = 0;
 
+  // TODO(cbruni): HasEntry and Get should not be exposed publicly with the
+  // entry parameter.
   virtual Handle<Object> Get(Handle<JSObject> 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;
 
@@ -67,8 +68,6 @@ class ElementsAccessor {
   // element that is non-deletable.
   virtual void SetLength(Handle<JSArray> holder, uint32_t new_length) = 0;
 
-  // Deletes an element in an object.
-  virtual void Delete(Handle<JSObject> 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.
@@ -126,11 +125,6 @@ class ElementsAccessor {
 
   virtual void Set(Handle<JSObject> holder, uint32_t entry, Object* value) = 0;
 
-  virtual void Reconfigure(Handle<JSObject> object,
-                           Handle<FixedArrayBase> backing_store, uint32_t entry,
-                           Handle<Object> value,
-                           PropertyAttributes attributes) = 0;
-
   virtual void Add(Handle<JSObject> object, uint32_t index,
                    Handle<Object> value, PropertyAttributes attributes,
                    uint32_t new_capacity) = 0;
@@ -214,6 +208,15 @@ class ElementsAccessor {
                                     FixedArrayBase* backing_store,
                                     uint32_t index) = 0;
 
+  virtual PropertyDetails GetDetails(JSObject* holder, uint32_t entry) = 0;
+  virtual void Reconfigure(Handle<JSObject> object,
+                           Handle<FixedArrayBase> backing_store, uint32_t entry,
+                           Handle<Object> value,
+                           PropertyAttributes attributes) = 0;
+
+  // Deletes an element in an object.
+  virtual void Delete(Handle<JSObject> 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

test/mjsunit/es8/object-entries.js

@@ -284,8 +284,8 @@ TestMutateDuringEnumeration();
     HOLEY_DOUBLE_ELEMENTS: [ [, , NaN], [ ["2", NaN] ] ],
 
     DICTIONARY_ELEMENTS: [ Object.defineProperties({ 10000: "world" }, {
-        100: { enumerable: true, value: "hello" },
-        99: { enumerable: false, value: "nope" }
+       100: { enumerable: true, value: "hello", configurable: true},
+        99: { enumerable: false, value: "nope", configurable: true}
       }), [ ["100", "hello"], ["10000", "world" ] ] ],
     FAST_SLOPPY_ARGUMENTS_ELEMENTS: [
         fastSloppyArguments("a", "b", "c"),
@@ -298,17 +298,42 @@ TestMutateDuringEnumeration();
         [ ["0", "s"], ["1", "t"], ["2", "r"]] ],
     SLOW_STRING_WRAPPER_ELEMENTS: [
         Object.defineProperties(new String("str"), {
-          10000: { enumerable: false, value: "X" },
-          9999: { enumerable: true, value: "Y" }
+          10000: { enumerable: false, value: "X", configurable: true},
+          9999: { enumerable: true, value: "Y", configurable: true}
         }), [["0", "s"], ["1", "t"], ["2", "r"], ["9999", "Y"]] ],
   };
 
   for (let [kind, [object, expected]] of Object.entries(element_kinds)) {
     let result1 = Object.entries(object);
+    %HeapObjectVerify(object);
+    %HeapObjectVerify(result1);
     assertEquals(expected, result1, `fast Object.entries() with ${kind}`);
 
     let proxy = new Proxy(object, {});
     let result2 = Object.entries(proxy);
+    %HeapObjectVerify(result2);
     assertEquals(result1, result2, `slow Object.entries() with ${kind}`);
   }
+
+  function makeFastElements(array) {
+    // Remove all possible getters.
+    for (let k of Object.getOwnPropertyNames(this)) {
+      if (k == "length") continue;
+      delete this[k];
+    }
+    // Make the array large enough to trigger re-checking for compaction.
+    this[1000] = 1;
+    // Make the elements fast again.
+    Array.prototype.unshift.call(this, 1.1);
+  }
+
+  // Test that changing the elements kind is supported.
+  for (let [kind, [object, expected]] of Object.entries(element_kinds)) {
+    if (kind == "FAST_STRING_WRAPPER_ELEMENTS") break;
+    object.__defineGetter__(1, makeFastElements);
+    let result1 = Object.entries(object).toString();
+    %HeapObjectVerify(object);
+    %HeapObjectVerify(result1);
+  }
+
 })();

test/mjsunit/regress/regress-crbug-798644.js

+// Copyright 2018 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.
+
+// Flags: --allow-natives-syntax
+
+let arr = [];
+// Make the array large enough to trigger re-checking for compaction.
+arr[1000] = 0x1234;
+
+arr.__defineGetter__(256, function () {
+    // Remove the getter so we can compact the array.
+    delete arr[256];
+    // Trigger compaction.
+    arr.unshift(1.1);
+});
+
+let results = Object.entries(arr);
+%HeapObjectVerify(results);
+%HeapObjectVerify(arr);
+let str = results.toString();