[vm] Fix race between Page::Deallocate during isolate group shutdown and Page::Cleanup during Dart_Cleanup.

Dart_Cleanup waited only for the isolate group to be unregistered, which happens before the group's heap is deleted.

TEST=tsan
Change-Id: I20046516635adbcbf63eae460d7b09e7e26169d8
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/416283
Reviewed-by: Alexander Aprelev <[email protected]>
Commit-Queue: Ryan Macnak <[email protected]>

Author: rmacnak-google

runtime/vm/dart.cc

@@ -589,7 +589,8 @@ void Dart::WaitForIsolateShutdown() {
   ASSERT(!Isolate::creation_enabled_);
   MonitorLocker ml(Isolate::isolate_creation_monitor_);
   intptr_t num_attempts = 0;
-  while (!IsolateGroup::HasOnlyVMIsolateGroup()) {
+  while (!IsolateGroup::HasOnlyVMIsolateGroup() ||
+         (Isolate::pending_shutdowns_ != 0)) {
     Monitor::WaitResult retval = ml.Wait(1000);
     if (retval == Monitor::kTimedOut) {
       num_attempts += 1;

runtime/vm/dart_api_impl_test.cc

@@ -141,6 +141,76 @@ UNIT_TEST_CASE(DartAPI_DartInitializeHeapSizes) {
   EXPECT(Dart_Cleanup() == nullptr);
 }
 
+static RelaxedAtomic<intptr_t> pending_isolate_groups = 0;
+static Dart_Isolate CreateCallback(const char* script_uri,
+                                   const char* main,
+                                   const char* package_root,
+                                   const char* package_config,
+                                   Dart_IsolateFlags* flags,
+                                   void* isolate_data,
+                                   char** error) {
+  pending_isolate_groups++;
+  return TesterState::create_callback(script_uri, main, package_root,
+                                      package_config, flags, isolate_data,
+                                      error);
+}
+static void CleanupCallback(void* isolate_group_data) {
+  TesterState::group_cleanup_callback(isolate_group_data);
+  OS::Sleep(3000);
+  pending_isolate_groups--;
+}
+
+UNIT_TEST_CASE(DartAPI_DartCleanupWaitsForGroupCleanupCallbacks) {
+  EXPECT(Dart_SetVMFlags(TesterState::argc, TesterState::argv) == nullptr);
+
+  Dart_InitializeParams params;
+  memset(&params, 0, sizeof(Dart_InitializeParams));
+  params.version = DART_INITIALIZE_PARAMS_CURRENT_VERSION;
+  params.vm_snapshot_data = TesterState::vm_snapshot_data;
+  params.create_group = CreateCallback;
+  params.shutdown_isolate = TesterState::shutdown_callback;
+  params.cleanup_group = CleanupCallback;
+  params.start_kernel_isolate = true;
+  EXPECT(Dart_Initialize(&params) == nullptr);
+
+  Monitor monitor;
+  bool shutting_down = false;
+  std::thread thread(
+      [](Monitor* monitor, bool* shutting_down) {
+        TestIsolateScope scope;
+        const char* kScriptChars =
+            "@pragma('vm:entry-point', 'call')\n"
+            "int testMain() {\n"
+            "  return 42;\n"
+            "}\n";
+        Dart_Handle lib = TestCase::LoadTestScript(kScriptChars, nullptr);
+        EXPECT_VALID(lib);
+        Dart_Handle result =
+            Dart_Invoke(lib, NewString("testMain"), 0, nullptr);
+        EXPECT_VALID(result);
+        int64_t value = 0;
+        EXPECT_VALID(Dart_IntegerToInt64(result, &value));
+        EXPECT_EQ(42, value);
+
+        MonitorLocker ml(monitor);
+        *shutting_down = true;
+        ml.Notify();
+      },
+      &monitor, &shutting_down);
+
+  {
+    MonitorLocker ml(&monitor);
+    while (!shutting_down) {
+      ml.Wait();
+    }
+  }
+
+  EXPECT(Dart_Cleanup() == nullptr);
+  EXPECT_EQ(0, pending_isolate_groups);
+
+  thread.join();
+}
+
 TEST_CASE(Dart_KillIsolate) {
   const char* kScriptChars =
       "@pragma('vm:entry-point', 'call')\n"

runtime/vm/isolate.cc

@@ -524,6 +524,11 @@ void IsolateGroup::Shutdown() {
     thread_pool_.reset();
   }
 
+  {
+    MonitorLocker ml(Isolate::isolate_creation_monitor_);
+    Isolate::pending_shutdowns_++;
+  }
+
   // Needs to happen before starting to destroy the heap so helper tasks like
   // the SampleBlockProcessor don't try to enter the group during this
   // tear-down.
@@ -565,7 +570,8 @@ void IsolateGroup::Shutdown() {
                    Dart::UptimeMillis(), name);
     }
     MonitorLocker ml(Isolate::isolate_creation_monitor_);
-    if (!Isolate::creation_enabled_) {
+    Isolate::pending_shutdowns_--;
+    if (Isolate::pending_shutdowns_ == 0) {
       ml.Notify();
     }
     if (trace_shutdown) {
@@ -2743,6 +2749,7 @@ Dart_IsolateGroupCleanupCallback Isolate::cleanup_group_callback_ = nullptr;
 Random* IsolateGroup::isolate_group_random_ = nullptr;
 Monitor* Isolate::isolate_creation_monitor_ = nullptr;
 bool Isolate::creation_enabled_ = false;
+intptr_t Isolate::pending_shutdowns_ = 0;
 
 RwLock* IsolateGroup::isolate_groups_rwlock_ = nullptr;
 IntrusiveDList<IsolateGroup>* IsolateGroup::isolate_groups_ = nullptr;

runtime/vm/isolate.h

@@ -1738,9 +1738,10 @@ class Isolate : public IntrusiveDListEntry<Isolate> {
     return accepts_messages_;
   }
 
-  // This monitor protects [creation_enabled_].
+  // This monitor protects [creation_enabled_] and [pending_shutdowns_].
   static Monitor* isolate_creation_monitor_;
   static bool creation_enabled_;
+  static intptr_t pending_shutdowns_;
 
   ArrayPtr loaded_prefixes_set_storage_;