[vm, reload] Rehash constants again after become.

The become operation might have "merged" some constants, creating duplicating entries in a canonical table that will cause trouble for later reloads.

TEST=vm/cc/IsolateReload_EnumDeleteMultiple
Bug: #56583
Change-Id: I62171f2dd36e1d9294203ae998dc05a6941f77a4
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/386321
Reviewed-by: Siva Annamalai <[email protected]>
Commit-Queue: Ryan Macnak <[email protected]>

runtime/vm/isolate.cc

@@ -1004,8 +1004,12 @@ void IsolateGroup::RehashConstants(Become* become) {
         ASSERT(!old_value.IsNull());
 
         if (become == nullptr) {
-          ASSERT(old_value.IsCanonical());
-          cls.InsertCanonicalConstant(zone, old_value);
+          if (old_value.IsCanonical()) {
+            cls.InsertCanonicalConstant(zone, old_value);
+          } else {
+            // The deleted enum value sentinel is not marked canonical.
+            ASSERT(cls.is_enum_class());
+          }
         } else {
           new_value = old_value.Canonicalize(thread);
           if (old_value.ptr() != new_value.ptr()) {

runtime/vm/isolate_reload.cc

@@ -1790,16 +1790,24 @@ void ProgramReloadContext::CommitBeforeInstanceMorphing() {
 }
 
 void ProgramReloadContext::CommitAfterInstanceMorphing() {
-  // Rehash constants map for all classes. Constants are hashed by content, and
-  // content may have changed from fields being added or removed.
   {
+    // Rehash constants map for all classes. Constants are hashed by content,
+    // and content may have changed from fields being added or removed.
     TIMELINE_SCOPE(RehashConstants);
     IG->RehashConstants(&become_);
   }
   {
+    // Forward old enum values to new enum values. Note this is a nop if the
+    // become operation is empty.
     TIMELINE_SCOPE(ForwardEnums);
     become_.Forward();
   }
+  {
+    // Rehash again, since the become operation may have merged some constants
+    // and various things are unhappy with duplicates in the canonical tables.
+    TIMELINE_SCOPE(RehashConstants);
+    IG->RehashConstants(nullptr);
+  }
 
   if (FLAG_identity_reload) {
     const auto& saved_libs = GrowableObjectArray::Handle(saved_libraries_);

runtime/vm/isolate_reload_test.cc

@@ -2259,6 +2259,49 @@ TEST_CASE(IsolateReload_EnumIdentityReload) {
                SimpleInvokeStr(lib, "main"));
 }
 
+TEST_CASE(IsolateReload_EnumDeleteMultiple) {
+  // See https://github.com/dart-lang/sdk/issues/56583.
+  // Accessing Fruit.values will cause a const array with all the enum values
+  // to stick around in the canonical table for _List.
+  const char* kScript =
+      "enum Fruit { Apple, Banana, Cherry }\n"
+      "var retained;\n"
+      "main() {\n"
+      "  retained = Fruit.values[0];\n"
+      "  return retained.toString();\n"
+      "}\n";
+
+  Dart_Handle lib = TestCase::LoadTestScript(kScript, nullptr);
+  EXPECT_VALID(lib);
+  EXPECT_STREQ("Fruit.Apple", SimpleInvokeStr(lib, "main"));
+
+  // Both Banana and Cherry forwarded to the deleted-enum sentinel, and
+  // two copies of the sentinel remain in Fruit's canonical table.
+  const char* kReloadScript0 =
+      "enum Fruit { Apple }\n"
+      "var retained;\n"
+      "main() {\n"
+      "  return retained.toString();\n"
+      "}\n";
+
+  lib = TestCase::ReloadTestScript(kReloadScript0);
+  EXPECT_VALID(lib);
+  EXPECT_STREQ("Fruit.Apple", SimpleInvokeStr(lib, "main"));
+
+  // When visiting Fruit's canonical table, we try to forward both entries of
+  // the old sentinel to the new sentinel, creating a become conflict.
+  const char* kReloadScript1 =
+      "enum Fruit { Apple }\n"
+      "var retained;\n"
+      "main() {\n"
+      "  return retained.toString();\n"
+      "}\n";
+
+  lib = TestCase::ReloadTestScript(kReloadScript1);
+  EXPECT_VALID(lib);
+  EXPECT_STREQ("Fruit.Apple", SimpleInvokeStr(lib, "main"));
+}
+
 TEST_CASE(IsolateReload_EnumShapeChange) {
   const char* kScript =
       "enum Fruit { Apple, Banana }\n"