Avoid manually shutting down engine managed isolates. (#8621)

These are now shutdown by the VM and cleanup waits for their shutdown.

BUILD.gn

@@ -47,6 +47,7 @@ group("flutter") {
     public_deps += [
       "$flutter_root/flow:flow_unittests",
       "$flutter_root/fml:fml_unittests",
+      "$flutter_root/runtime:runtime_lifecycle_unittests",
       "$flutter_root/runtime:runtime_unittests",
       "$flutter_root/shell/common:shell_unittests",
       "$flutter_root/shell/platform/common/cpp/client_wrapper:client_wrapper_unittests",

runtime/dart_isolate.cc

@@ -721,8 +721,6 @@ DartIsolate::CreateDartVMAndEmbedderObjectPair(
     }
   }
 
-  DartVMRef::GetRunningVM()->RegisterActiveIsolate(*embedder_isolate);
-
   // The ownership of the embedder object is controlled by the Dart VM. So the
   // only reference returned to the caller is weak.
   embedder_isolate.release();
@@ -760,8 +758,6 @@ void DartIsolate::AddIsolateShutdownCallback(fml::closure closure) {
 
 void DartIsolate::OnShutdownCallback() {
   shutdown_callbacks_.clear();
-  DartVMRef::GetRunningVM()->UnregisterActiveIsolate(
-      std::static_pointer_cast<DartIsolate>(shared_from_this()));
 }
 
 DartIsolate::AutoFireClosure::AutoFireClosure(fml::closure closure)

runtime/dart_lifecycle_unittests.cc

@@ -4,6 +4,7 @@
 
 #include "flutter/common/task_runners.h"
 #include "flutter/fml/paths.h"
+#include "flutter/fml/synchronization/count_down_latch.h"
 #include "flutter/fml/synchronization/waitable_event.h"
 #include "flutter/runtime/dart_vm.h"
 #include "flutter/runtime/dart_vm_lifecycle.h"
@@ -41,8 +42,7 @@ TEST_F(DartLifecycleTest, CanStartAndShutdownVMOverAndOver) {
   ASSERT_FALSE(DartVMRef::IsInstanceRunning());
 }
 
-static void CreateAndRunRootIsolate(
-    std::shared_ptr<DartIsolate>& isolate_result,
+static std::shared_ptr<DartIsolate> CreateAndRunRootIsolate(
     const Settings& settings,
     const DartVMData& vm,
     fml::RefPtr<fml::TaskRunner> task_runner,
@@ -67,73 +67,74 @@ static void CreateAndRunRootIsolate(
 
   if (!isolate) {
     FML_LOG(ERROR) << "Could not create valid isolate.";
-    return;
+    return nullptr;
   }
 
   if (DartVM::IsRunningPrecompiledCode()) {
     if (!isolate->PrepareForRunningFromPrecompiledCode()) {
       FML_LOG(ERROR)
           << "Could not prepare to run the isolate from precompiled code.";
-      return;
+      return nullptr;
     }
 
   } else {
     if (!isolate->PrepareForRunningFromKernels(
             settings.application_kernels())) {
       FML_LOG(ERROR) << "Could not prepare isolate from application kernels.";
-      return;
+      return nullptr;
     }
   }
 
   if (isolate->GetPhase() != DartIsolate::Phase::Ready) {
     FML_LOG(ERROR) << "Isolate was not ready.";
-    return;
+    return nullptr;
   }
 
   if (!isolate->Run(entrypoint, settings.root_isolate_create_callback)) {
     FML_LOG(ERROR) << "Could not run entrypoint: " << entrypoint << ".";
-    return;
+    return nullptr;
   }
 
   if (isolate->GetPhase() != DartIsolate::Phase::Running) {
     FML_LOG(ERROR) << "Isolate was not Running.";
-    return;
+    return nullptr;
   }
 
-  isolate_result = isolate;
-}
-
-static std::shared_ptr<DartIsolate> CreateAndRunRootIsolate(
-    const Settings& settings,
-    const DartVMData& vm,
-    fml::RefPtr<fml::TaskRunner> task_runner,
-    std::string entrypoint) {
-  fml::AutoResetWaitableEvent latch;
-  std::shared_ptr<DartIsolate> isolate;
-  fml::TaskRunner::RunNowOrPostTask(task_runner, [&]() {
-    CreateAndRunRootIsolate(isolate, settings, vm, task_runner, entrypoint);
-    latch.Signal();
-  });
-  latch.Wait();
   return isolate;
 }
 
-TEST_F(DartLifecycleTest, ShuttingDownTheVMShutsDownTheIsolate) {
+TEST_F(DartLifecycleTest, ShuttingDownTheVMShutsDownAllIsolates) {
   auto settings = CreateSettingsForFixture();
   settings.leak_vm = false;
-  settings.enable_observatory = false;
-  ASSERT_FALSE(DartVMRef::IsInstanceRunning());
-  {
+  // Make sure the service protocol launches
+  settings.enable_observatory = true;
+
+  for (size_t i = 0; i < 3; i++) {
+    ASSERT_FALSE(DartVMRef::IsInstanceRunning());
+
+    const auto last_launch_count = DartVM::GetVMLaunchCount();
+
     auto vm_ref = DartVMRef::Create(settings);
+
     ASSERT_TRUE(DartVMRef::IsInstanceRunning());
-    ASSERT_EQ(vm_ref->GetIsolateCount(), 0u);
-    auto isolate =
-        CreateAndRunRootIsolate(settings, *vm_ref.GetVMData(),
-                                GetThreadTaskRunner(), "testIsolateShutdown");
-    ASSERT_TRUE(isolate);
-    ASSERT_EQ(vm_ref->GetIsolateCount(), 1u);
-    vm_ref->ShutdownAllIsolates();
-    ASSERT_EQ(vm_ref->GetIsolateCount(), 0u);
+    ASSERT_EQ(last_launch_count + 1, DartVM::GetVMLaunchCount());
+
+    const size_t isolate_count = 100;
+
+    fml::CountDownLatch latch(isolate_count);
+    auto vm_data = vm_ref.GetVMData();
+    auto thread_task_runner = GetThreadTaskRunner();
+    for (size_t i = 0; i < isolate_count; ++i) {
+      thread_task_runner->PostTask(
+          [vm_data, &settings, &latch, thread_task_runner]() {
+            ASSERT_TRUE(CreateAndRunRootIsolate(settings, *vm_data.get(),
+                                                thread_task_runner,
+                                                "testIsolateShutdown"));
+            latch.CountDown();
+          });
+    }
+
+    latch.Wait();
   }
   ASSERT_FALSE(DartVMRef::IsInstanceRunning());
 }

runtime/dart_vm.cc

@@ -460,54 +460,4 @@ std::shared_ptr<IsolateNameServer> DartVM::GetIsolateNameServer() const {
   return isolate_name_server_;
 }
 
-size_t DartVM::GetIsolateCount() const {
-  std::lock_guard<std::mutex> lock(active_isolates_mutex_);
-  return active_isolates_.size();
-}
-
-void DartVM::ShutdownAllIsolates() {
-  std::set<std::shared_ptr<DartIsolate>> isolates_to_shutdown;
-  // We may be shutting down isolates on the current thread. Shutting down the
-  // isolate calls the shutdown callback which removes the entry from the
-  // active isolate. The lock must be obtained to mutate that entry. To avoid a
-  // deadlock, collect the isolate is a seprate collection.
-  {
-    std::lock_guard<std::mutex> lock(active_isolates_mutex_);
-    for (const auto& active_isolate : active_isolates_) {
-      if (auto task_runner = active_isolate->GetMessageHandlingTaskRunner()) {
-        isolates_to_shutdown.insert(active_isolate);
-      }
-    }
-  }
-
-  fml::CountDownLatch latch(isolates_to_shutdown.size());
-
-  for (const auto& isolate : isolates_to_shutdown) {
-    fml::TaskRunner::RunNowOrPostTask(
-        isolate->GetMessageHandlingTaskRunner(), [&latch, isolate]() {
-          if (!isolate || !isolate->Shutdown()) {
-            FML_LOG(ERROR) << "Could not shutdown isolate.";
-          }
-          latch.CountDown();
-        });
-  }
-  latch.Wait();
-}
-
-void DartVM::RegisterActiveIsolate(std::shared_ptr<DartIsolate> isolate) {
-  if (!isolate) {
-    return;
-  }
-  std::lock_guard<std::mutex> lock(active_isolates_mutex_);
-  active_isolates_.insert(isolate);
-}
-
-void DartVM::UnregisterActiveIsolate(std::shared_ptr<DartIsolate> isolate) {
-  if (!isolate) {
-    return;
-  }
-  std::lock_guard<std::mutex> lock(active_isolates_mutex_);
-  active_isolates_.erase(isolate);
-}
-
 }  // namespace flutter

runtime/dart_vm.h

@@ -36,24 +36,15 @@ class DartVM {
 
   std::shared_ptr<const DartVMData> GetVMData() const;
 
-  // This accessor is racy and only meant to the used in tests where there is a
-  // consistent threading mode.
-  size_t GetIsolateCount() const;
-
   std::shared_ptr<ServiceProtocol> GetServiceProtocol() const;
 
   std::shared_ptr<IsolateNameServer> GetIsolateNameServer() const;
 
-  void ShutdownAllIsolates();
-
  private:
   const Settings settings_;
   std::shared_ptr<const DartVMData> vm_data_;
   const std::shared_ptr<IsolateNameServer> isolate_name_server_;
   const std::shared_ptr<ServiceProtocol> service_protocol_;
-  mutable std::mutex active_isolates_mutex_;
-  std::set<std::shared_ptr<DartIsolate>> active_isolates_
-      FML_GUARDED_BY(active_isolates_mutex_);
 
   friend class DartVMRef;
   friend class DartIsolate;
@@ -68,10 +59,6 @@ class DartVM {
   DartVM(std::shared_ptr<const DartVMData> data,
          std::shared_ptr<IsolateNameServer> isolate_name_server);
 
-  void RegisterActiveIsolate(std::shared_ptr<DartIsolate> isolate);
-
-  void UnregisterActiveIsolate(std::shared_ptr<DartIsolate> isolate);
-
   FML_DISALLOW_COPY_AND_ASSIGN(DartVM);
 };
 

testing/run_tests.sh

@@ -35,6 +35,9 @@ echo "Running fml_unittests..."
 echo "Running runtime_unittests..."
 "$HOST_DIR/runtime_unittests"
 
+echo "Running runtime_lifecycle_unittests..."
+"$HOST_DIR/runtime_lifecycle_unittests"
+
 echo "Running shell_unittests..."
 "$HOST_DIR/shell_unittests"