[const-tracking] Ensure map is updated before generalizing constness

isheludko · Commit Bot · commit db2acd7a046d · 2021-04-06T16:13:32.000Z
... when reconfiguring property attributes. Bug: chromium:1195331 Change-Id: I65a29f0ad303a603207376a283e943480c4b18d2 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2807608 Reviewed-by: Leszek Swirski <leszeks@chromium.org> Commit-Queue: Igor Sheludko <ishell@chromium.org> Cr-Commit-Position: refs/heads/master@{#73810}
diff --git a/src/objects/map-updater.cc b/src/objects/map-updater.cc
@@ -143,11 +143,20 @@ Handle<Map> MapUpdater::ReconfigureToDataField(InternalIndex descriptor,
   if (old_details.constness() == PropertyConstness::kConst &&
       old_details.location() == kField &&
       old_details.attributes() != new_attributes_) {
+    // Ensure we'll be updating constness of the up-to-date version of old_map_.
+    Handle<Map> old_map = UpdateMapNoLock(isolate_, old_map_);
+    PropertyDetails details =
+        old_map->instance_descriptors(isolate_).GetDetails(descriptor);
     Handle<FieldType> field_type(
-        old_descriptors_->GetFieldType(modified_descriptor_), isolate_);
-    Map::GeneralizeField(isolate_, old_map_, descriptor,
-                         PropertyConstness::kMutable,
-                         old_details.representation(), field_type);
+        old_map->instance_descriptors(isolate_).GetFieldType(descriptor),
+        isolate_);
+    Map::GeneralizeField(isolate_, old_map, descriptor,
+                         PropertyConstness::kMutable, details.representation(),
+                         field_type);
+    DCHECK_EQ(PropertyConstness::kMutable,
+              old_map->instance_descriptors(isolate_)
+                  .GetDetails(descriptor)
+                  .constness());
     // The old_map_'s property must become mutable.
     // Note, that the {old_map_} and {old_descriptors_} are not expected to be
     // updated by the generalization if the map is already deprecated.
@@ -221,12 +230,25 @@ Handle<Map> MapUpdater::ReconfigureElementsKind(ElementsKind elements_kind) {
   return result_map_;
 }
 
-Handle<Map> MapUpdater::Update() {
-  DCHECK_EQ(kInitialized, state_);
-  DCHECK(old_map_->is_deprecated());
+// static
+Handle<Map> MapUpdater::UpdateMapNoLock(Isolate* isolate, Handle<Map> map) {
+  if (!map->is_deprecated()) return map;
+  // TODO(ishell): support fast map updating if we enable it.
+  CHECK(!FLAG_fast_map_update);
+  MapUpdater mu(isolate, map);
+  // Update map without locking the Isolate::map_updater_access mutex.
+  return mu.UpdateImpl();
+}
 
+Handle<Map> MapUpdater::Update() {
   base::SharedMutexGuard<base::kExclusive> mutex_guard(
       isolate_->map_updater_access());
+  return UpdateImpl();
+}
+
+Handle<Map> MapUpdater::UpdateImpl() {
+  DCHECK_EQ(kInitialized, state_);
+  DCHECK(old_map_->is_deprecated());
 
   if (FindRootMap() == kEnd) return result_map_;
   if (FindTargetMap() == kEnd) return result_map_;
diff --git a/src/objects/map-updater.h b/src/objects/map-updater.h
@@ -76,6 +76,16 @@ class V8_EXPORT_PRIVATE MapUpdater {
     kEnd
   };
 
+  // Updates map to the most up-to-date non-deprecated version.
+  static inline Handle<Map> UpdateMapNoLock(Isolate* isolate,
+                                            Handle<Map> old_map);
+
+  // Prepares for updating deprecated map to most up-to-date non-deprecated
+  // version and performs the steps 1-6.
+  // Unlike the Update() entry point it doesn't lock the map_updater_access
+  // mutex.
+  Handle<Map> UpdateImpl();
+
   // Try to reconfigure property in-place without rebuilding transition tree
   // and creating new maps. See implementation for details.
   State TryReconfigureToDataFieldInplace();
diff --git a/test/mjsunit/regress/regress-crbug-1195331.js b/test/mjsunit/regress/regress-crbug-1195331.js
@@ -0,0 +1,36 @@
+// Copyright 2021 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 o1 = { a: 1, b: 0 };
+let o2 = { a: 2, b: 0 };
+assertTrue(%HaveSameMap(o1, o2));
+assertTrue(%HasOwnConstDataProperty(o1, "a"));
+assertTrue(%HasOwnConstDataProperty(o1, "b"));
+
+Object.defineProperty(o1, "b", {
+  value: 4.2, enumerable: true, configurable: true, writable: true,
+});
+assertFalse(%HaveSameMap(o1, o2));
+assertTrue(%HasOwnConstDataProperty(o1, "a"));
+assertFalse(%HasOwnConstDataProperty(o1, "b"));
+assertTrue(%HasOwnConstDataProperty(o2, "a"));
+assertTrue(%HasOwnConstDataProperty(o2, "b"));
+
+let o3 = { a: "foo", b: 0 };
+assertFalse(%HaveSameMap(o2, o3));
+assertTrue(%HasOwnConstDataProperty(o3, "a"));
+assertFalse(%HasOwnConstDataProperty(o3, "b"));
+
+Object.defineProperty(o2, "a", {
+  value:2, enumerable: false, configurable: true, writable: true,
+});
+assertFalse(%HasOwnConstDataProperty(o1, "a"));
+assertFalse(%HasOwnConstDataProperty(o1, "b"));
+assertFalse(%HasOwnConstDataProperty(o3, "a"));
+assertFalse(%HasOwnConstDataProperty(o3, "b"));
+
+assertFalse(%HasOwnConstDataProperty(o2, "a"));
+assertTrue(%HasOwnConstDataProperty(o2, "b"));
