[runtime] Fix Object.assign for in-place repr changes

Fix uses of cached descriptors arrays used in loops that map-check
to ensure validity of the cache to also reload the descriptor in
case there are missed in-place representation updates.

As a drive-by, introduce inner HandleScopes for these loops.

Bug: chromium:1012301
Change-Id: I17273caf629a181b846d3c09777b5c08fd8cbb0e
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1859621Reviewed-by: Igor Sheludko <ishell@chromium.org>
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/master@{#64287}

src/codegen/code-stub-assembler.cc

@@ -8507,7 +8507,7 @@ void CodeStubAssembler::ForEachEnumerableOwnProperty(
   TNode<Uint16T> type = LoadMapInstanceType(map);
   TNode<Uint32T> bit_field3 = EnsureOnlyHasSimpleProperties(map, type, bailout);
 
-  TNode<DescriptorArray> descriptors = LoadMapDescriptors(map);
+  TVARIABLE(DescriptorArray, var_descriptors, LoadMapDescriptors(map));
   TNode<Uint32T> nof_descriptors =
       DecodeWord32<Map::NumberOfOwnDescriptorsBits>(bit_field3);
 
@@ -8522,13 +8522,14 @@ void CodeStubAssembler::ForEachEnumerableOwnProperty(
   // Note: var_end_key_index is exclusive for the loop
   TVARIABLE(IntPtrT, var_end_key_index,
             ToKeyIndex<DescriptorArray>(nof_descriptors));
-  VariableList list(
-      {&var_stable, &var_has_symbol, &var_is_symbol_processing_loop,
-       &var_start_key_index, &var_end_key_index},
-      zone());
+  VariableList list({&var_descriptors, &var_stable, &var_has_symbol,
+                     &var_is_symbol_processing_loop, &var_start_key_index,
+                     &var_end_key_index},
+                    zone());
   Label descriptor_array_loop(
-      this, {&var_stable, &var_has_symbol, &var_is_symbol_processing_loop,
-             &var_start_key_index, &var_end_key_index});
+      this, {&var_descriptors, &var_stable, &var_has_symbol,
+             &var_is_symbol_processing_loop, &var_start_key_index,
+             &var_end_key_index});
 
   Goto(&descriptor_array_loop);
   BIND(&descriptor_array_loop);
@@ -8537,7 +8538,7 @@ void CodeStubAssembler::ForEachEnumerableOwnProperty(
       list, var_start_key_index.value(), var_end_key_index.value(),
       [&](TNode<IntPtrT> descriptor_key_index) {
         TNode<Name> next_key =
-            LoadKeyByKeyIndex(descriptors, descriptor_key_index);
+            LoadKeyByKeyIndex(var_descriptors.value(), descriptor_key_index);
 
         TVARIABLE(Object, var_value, SmiConstant(0));
         Label callback(this), next_iteration(this);
@@ -8592,7 +8593,7 @@ void CodeStubAssembler::ForEachEnumerableOwnProperty(
             // Directly decode from the descriptor array if |object| did not
             // change shape.
             var_map = map;
-            var_meta_storage = descriptors;
+            var_meta_storage = var_descriptors.value();
             var_entry = Signed(descriptor_key_index);
             Goto(&if_found_fast);
           }
@@ -8658,12 +8659,14 @@ void CodeStubAssembler::ForEachEnumerableOwnProperty(
             BIND(&callback);
             body(next_key, var_value.value());
 
-            // Check if |object| is still stable, i.e. we can proceed using
-            // property details from preloaded |descriptors|.
-            var_stable = Select<BoolT>(
-                var_stable.value(),
-                [=] { return TaggedEqual(LoadMap(object), map); },
-                [=] { return Int32FalseConstant(); });
+            // Check if |object| is still stable, i.e. the descriptors in the
+            // preloaded |descriptors| are still the same modulo in-place
+            // representation changes.
+            GotoIfNot(var_stable.value(), &next_iteration);
+            var_stable = TaggedEqual(LoadMap(object), map);
+            // Reload the descriptors just in case the actual array changed, and
+            // any of the field representations changed in-place.
+            var_descriptors = LoadMapDescriptors(map);
 
             Goto(&next_iteration);
           }

src/objects/js-objects.cc

@@ -220,10 +220,15 @@ V8_WARN_UNUSED_RESULT Maybe<bool> FastAssign(
   bool stable = true;
 
   for (InternalIndex i : map->IterateOwnDescriptors()) {
+    HandleScope inner_scope(isolate);
+
     Handle<Name> next_key(descriptors->GetKey(i), isolate);
     Handle<Object> prop_value;
     // Directly decode from the descriptor array if |from| did not change shape.
     if (stable) {
+      DCHECK_EQ(from->map(), *map);
+      DCHECK_EQ(*descriptors, map->instance_descriptors());
+
       PropertyDetails details = descriptors->GetDetails(i);
       if (!details.IsEnumerable()) continue;
       if (details.kind() == kData) {
@@ -231,7 +236,8 @@ V8_WARN_UNUSED_RESULT Maybe<bool> FastAssign(
           prop_value = handle(descriptors->GetStrongValue(i), isolate);
         } else {
           Representation representation = details.representation();
-          FieldIndex index = FieldIndex::ForDescriptor(*map, i);
+          FieldIndex index = FieldIndex::ForPropertyIndex(
+              *map, details.field_index(), representation);
           prop_value = JSObject::FastPropertyAt(from, representation, index);
         }
       } else {
@@ -239,6 +245,7 @@ V8_WARN_UNUSED_RESULT Maybe<bool> FastAssign(
             isolate, prop_value,
             JSReceiver::GetProperty(isolate, from, next_key), Nothing<bool>());
         stable = from->map() == *map;
+        *descriptors.location() = map->instance_descriptors().ptr();
       }
     } else {
       // If the map did change, do a slower lookup. We are still guaranteed that
@@ -259,7 +266,10 @@ V8_WARN_UNUSED_RESULT Maybe<bool> FastAssign(
           Object::SetProperty(&it, prop_value, StoreOrigin::kNamed,
                               Just(ShouldThrow::kThrowOnError));
       if (result.IsNothing()) return result;
-      if (stable) stable = from->map() == *map;
+      if (stable) {
+        stable = from->map() == *map;
+        *descriptors.location() = map->instance_descriptors().ptr();
+      }
     } else {
       if (excluded_properties != nullptr &&
           HasExcludedProperty(excluded_properties, next_key)) {
@@ -1832,8 +1842,8 @@ V8_WARN_UNUSED_RESULT Maybe<bool> FastGetOwnValuesOrEntries(
   if (!map->OnlyHasSimpleProperties()) return Just(false);
 
   Handle<JSObject> object(JSObject::cast(*receiver), isolate);
-
   Handle<DescriptorArray> descriptors(map->instance_descriptors(), isolate);
+
   int number_of_own_descriptors = map->NumberOfOwnDescriptors();
   int number_of_own_elements =
       object->GetElementsAccessor()->GetCapacity(*object, object->elements());
@@ -1855,15 +1865,25 @@ V8_WARN_UNUSED_RESULT Maybe<bool> FastGetOwnValuesOrEntries(
                  Nothing<bool>());
   }
 
-  bool stable = object->map() == *map;
+  // We may have already lost stability, if CollectValuesOrEntries had
+  // side-effects.
+  bool stable = *map == object->map();
+  if (stable) {
+    *descriptors.location() = map->instance_descriptors().ptr();
+  }
 
   for (InternalIndex index : InternalIndex::Range(number_of_own_descriptors)) {
+    HandleScope inner_scope(isolate);
+
     Handle<Name> next_key(descriptors->GetKey(index), isolate);
     if (!next_key->IsString()) continue;
     Handle<Object> prop_value;
 
     // Directly decode from the descriptor array if |from| did not change shape.
     if (stable) {
+      DCHECK_EQ(object->map(), *map);
+      DCHECK_EQ(*descriptors, map->instance_descriptors());
+
       PropertyDetails details = descriptors->GetDetails(index);
       if (!details.IsEnumerable()) continue;
       if (details.kind() == kData) {
@@ -1871,7 +1891,8 @@ V8_WARN_UNUSED_RESULT Maybe<bool> FastGetOwnValuesOrEntries(
           prop_value = handle(descriptors->GetStrongValue(index), isolate);
         } else {
           Representation representation = details.representation();
-          FieldIndex field_index = FieldIndex::ForDescriptor(*map, index);
+          FieldIndex field_index = FieldIndex::ForPropertyIndex(
+              *map, details.field_index(), representation);
           prop_value =
               JSObject::FastPropertyAt(object, representation, field_index);
         }
@@ -1881,6 +1902,7 @@ V8_WARN_UNUSED_RESULT Maybe<bool> FastGetOwnValuesOrEntries(
             JSReceiver::GetProperty(isolate, object, next_key),
             Nothing<bool>());
         stable = object->map() == *map;
+        *descriptors.location() = map->instance_descriptors().ptr();
       }
     } else {
       // If the map did change, do a slower lookup. We are still guaranteed that

test/mjsunit/regress/regress-crbug-1012301-1.js

+// Copyright 2019 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 get() {
+  // Update the descriptor array now shared between the Foo map and the
+  // (Foo + c) map.
+  o1.c = 10;
+  // Change the type of the field on the new descriptor array in-place to
+  // Tagged. If Object.assign has a cached descriptor array, then it will point
+  // to the old Foo map's descriptors, which still have .b as Double.
+  o2.b = "string";
+  return 1;
+}
+
+function Foo() {
+  Object.defineProperty(this, "a", {get, enumerable: true});
+  // Initialise Foo.b to have Double representation.
+  this.b = 1.5;
+}
+
+var o1 = new Foo();
+var o2 = new Foo();
+var target = {};
+Object.assign(target, o2);
+// Make sure that target has the right representation after assignment.
+assertEquals(target.b, "string");