[runtime] Fix CloneObject for all in-place repr changes

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

src/objects/objects.cc

@@ -65,6 +65,7 @@
 #include "src/objects/lookup-inl.h"
 #include "src/objects/map-updater.h"
 #include "src/objects/objects-body-descriptors-inl.h"
+#include "src/objects/property-details.h"
 #include "src/utils/identity-map.h"
 #ifdef V8_INTL_SUPPORT
 #include "src/objects/js-break-iterator.h"
@@ -3764,18 +3765,14 @@ Handle<DescriptorArray> DescriptorArray::CopyForFastObjectClone(
   for (InternalIndex i : InternalIndex::Range(size)) {
     Name key = src->GetKey(i);
     PropertyDetails details = src->GetDetails(i);
+    Representation new_representation = details.representation();
 
     DCHECK(!key.IsPrivateName());
     DCHECK(details.IsEnumerable());
     DCHECK_EQ(details.kind(), kData);
-
-    // Ensure the ObjectClone property details are NONE, and that all source
-    // details did not contain DONT_ENUM.
-    PropertyDetails new_details(kData, NONE, details.location(),
-                                details.constness(), details.representation(),
-                                details.field_index());
-    // Do not propagate the field type of normal object fields from the
-    // original descriptors since FieldType changes don't create new maps.
+    // If the new representation is an in-place changeable field, make it
+    // generic as possible (under in-place changes) to avoid type confusion if
+    // the source representation changes after this feedback has been collected.
     MaybeObject type = src->GetValue(i);
     if (details.location() == PropertyLocation::kField) {
       type = MaybeObject::FromObject(FieldType::Any());
@@ -3784,13 +3781,15 @@ Handle<DescriptorArray> DescriptorArray::CopyForFastObjectClone(
       // need to generalize the descriptors here. That will also enable
       // us to skip the defensive copying of the target map whenever a
       // CloneObjectIC misses.
-      if (FLAG_modify_field_representation_inplace &&
-          (new_details.representation().IsSmi() ||
-           new_details.representation().IsHeapObject())) {
-        new_details =
-            new_details.CopyWithRepresentation(Representation::Tagged());
-      }
+      new_representation = new_representation.MostGenericInPlaceChange();
     }
+
+    // Ensure the ObjectClone property details are NONE, and that all source
+    // details did not contain DONT_ENUM.
+    PropertyDetails new_details(kData, NONE, details.location(),
+                                details.constness(), new_representation,
+                                details.field_index());
+
     descriptors->Set(i, key, type, new_details);
   }
 

src/objects/property-details.h

@@ -117,6 +117,16 @@ class Representation {
            other.IsTagged();
   }
 
+  // Return the most generic representation that this representation can be
+  // changed to in-place. If in-place representation changes are disabled, then
+  // this will return the current representation.
+  Representation MostGenericInPlaceChange() const {
+    if (!FLAG_modify_field_representation_inplace) return *this;
+    // Everything but unboxed doubles can be in-place changed to Tagged.
+    if (FLAG_unbox_double_fields && IsDouble()) return Representation::Double();
+    return Representation::Tagged();
+  }
+
   bool is_more_general_than(const Representation& other) const {
     if (IsHeapObject()) return other.IsNone();
     return kind_ > other.kind_;

test/mjsunit/regress/regress-crbug-1012301.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.
+//
+// Flags: --allow-natives-syntax
+
+function f(o) {
+  // The spread after the CloneObject IC shouldn't crash when trying to write a
+  // double value to a field created by CloneObject.
+  return {...o, ...{a:1.4}};
+}
+
+%EnsureFeedbackVectorForFunction(f);
+
+var o = {};
+// Train the CloneObject IC with a Double field.
+o.a = 1.5;
+f(o);
+f(o);
+f(o);
+// Change the source map to have a Tagged field.
+o.a = undefined;
+f(o);