[map] Ignore migration target bit when normalizing

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

src/objects/map.cc

@@ -1472,8 +1472,12 @@ Handle<Map> Map::Normalize(Isolate* isolate, Handle<Map> fast_map,
       // The IsInRetainedMapListBit might be different if the {new_map}
       // that we got from the {cache} was already embedded into optimized
       // code somewhere.
-      DCHECK_EQ(fresh->bit_field3() & ~IsInRetainedMapListBit::kMask,
-                new_map->bit_field3() & ~IsInRetainedMapListBit::kMask);
+      // The IsMigrationTargetBit might be different if the {new_map} from
+      // {cache} has already been marked as a migration target.
+      constexpr int ignored_bit_field3_bits =
+          IsInRetainedMapListBit::kMask | IsMigrationTargetBit::kMask;
+      DCHECK_EQ(fresh->bit_field3() & ~ignored_bit_field3_bits,
+                new_map->bit_field3() & ~ignored_bit_field3_bits);
       int offset = Map::kBitField3Offset + kInt32Size;
       DCHECK_EQ(0, memcmp(reinterpret_cast<void*>(fresh->address() + offset),
                           reinterpret_cast<void*>(new_map->address() + offset),
@@ -1499,10 +1503,10 @@ Handle<Map> Map::Normalize(Isolate* isolate, Handle<Map> fast_map,
       cache->Set(fast_map, new_map);
       isolate->counters()->maps_normalized()->Increment();
     }
+  }
   if (FLAG_trace_maps) {
     LOG(isolate, MapEvent("Normalize", *fast_map, *new_map, reason));
   }
-  }
   fast_map->NotifyLeafMapLayoutChange(isolate);
   return new_map;
 }

test/cctest/test-field-type-tracking.cc

@@ -2943,6 +2943,31 @@ TEST(StoreToConstantField_StoreIC) {
   TestStoreToConstantField_NaN(store_func_source, 2);
 }
 
+TEST(NormalizeToMigrationTarget) {
+  CcTest::InitializeVM();
+  v8::HandleScope scope(CcTest::isolate());
+  Isolate* isolate = CcTest::i_isolate();
+
+  CHECK(
+      isolate->native_context()->normalized_map_cache().IsNormalizedMapCache());
+
+  Handle<Map> base_map = Map::Create(isolate, 4);
+
+  Handle<Map> existing_normalized_map = Map::Normalize(
+      isolate, base_map, PropertyNormalizationMode::CLEAR_INOBJECT_PROPERTIES,
+      "Test_NormalizeToMigrationTarget_ExistingMap");
+  existing_normalized_map->set_is_migration_target(true);
+
+  // Normalizing a second map should hit the normalized map cache, including it
+  // being OK for the new map to be a migration target.
+  CHECK(!base_map->is_migration_target());
+  Handle<Map> new_normalized_map = Map::Normalize(
+      isolate, base_map, PropertyNormalizationMode::CLEAR_INOBJECT_PROPERTIES,
+      "Test_NormalizeToMigrationTarget_NewMap");
+  CHECK_EQ(*existing_normalized_map, *new_normalized_map);
+  CHECK(new_normalized_map->is_migration_target());
+}
+
 }  // namespace test_field_type_tracking
 }  // namespace compiler
 }  // namespace internal

test/mjsunit/regress/regress-crbug-977089.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
+
+// This function was carefully constructed by Clusterfuzz to execute a certain
+// sequence of transitions. Thus, it may no longer test anything useful if
+// the transition logic changes.
+//
+// The more stable unit test testing the same bug is:
+//   test-field-type-tracking/NormalizeToMigrationTarget
+var foo = function() {
+
+  function f1(arg) {
+    var ret = { x: arg };
+    ret.__defineGetter__("y", function() { });
+    return ret;
+  }
+  // Create v1 with a map with properties: {x:Smi, y:AccessorPair}
+  let v1 = f1(10);
+  // Create a map with properties: {x:Double, y:AccessorPair}, deprecating the
+  // previous map.
+  let v2 = f1(10.5);
+
+  // Access x on v1 to a function that reads x, which triggers it to update its
+  // map. This update transitions v1 to slow mode as there is already a "y"
+  // transition with a different accessor.
+  //
+  // Note that the parent function `foo` can't be an IIFE, as then this callsite
+  // would use the NoFeedback version of the LdaNamedProperty bytecode, and this
+  // doesn't trigger the map update.
+  v1.x;
+
+  // Create v3 which overwrites a non-accessor with an accessor, triggering it
+  // to normalize, and picking up the same cached normalized map as v1. However,
+  // v3's map is not a migration target and v1's is (as it was migrated to when
+  // updating v1), so the migration target bit doesn't match. This should be
+  // fine and shouldn't trigger any DCHECKs.
+  let v3 = { z:1 };
+  v3.__defineGetter__("z", function() {});
+};
+
+%EnsureFeedbackVectorForFunction(foo);
+foo();