Reland children isolates sharing isolate group change. (#13758)

* Revert "Revert "Provide dart vm initalize isolate callback so that children isolates belong to parent's isolate group. (#9888)" (#12327)"

* Ensure that when isolate shuts down it calls isolate_data, rather than isolage_group_data callback.

runtime/dart_isolate.cc

@@ -68,7 +68,9 @@ std::weak_ptr<DartIsolate> DartIsolate::CreateRootIsolate(
           advisory_script_entrypoint,    // advisory entrypoint
           nullptr,                       // child isolate preparer
           isolate_create_callback,       // isolate create callback
-          isolate_shutdown_callback      // isolate shutdown callback
+          isolate_shutdown_callback,     // isolate shutdown callback
+          true,                          // is_root_isolate
+          true                           // is_group_root_isolate
           )));
 
   std::tie(vm_isolate, embedder_isolate) = CreateDartVMAndEmbedderObjectPair(
@@ -113,7 +115,9 @@ DartIsolate::DartIsolate(const Settings& settings,
                          std::string advisory_script_entrypoint,
                          ChildIsolatePreparer child_isolate_preparer,
                          fml::closure isolate_create_callback,
-                         fml::closure isolate_shutdown_callback)
+                         fml::closure isolate_shutdown_callback,
+                         bool is_root_isolate,
+                         bool is_group_root_isolate)
     : UIDartState(std::move(task_runners),
                   settings.task_observer_add,
                   settings.task_observer_remove,
@@ -130,7 +134,9 @@ DartIsolate::DartIsolate(const Settings& settings,
       isolate_snapshot_(std::move(isolate_snapshot)),
       child_isolate_preparer_(std::move(child_isolate_preparer)),
       isolate_create_callback_(isolate_create_callback),
-      isolate_shutdown_callback_(isolate_shutdown_callback) {
+      isolate_shutdown_callback_(isolate_shutdown_callback),
+      is_root_isolate_(is_root_isolate),
+      is_group_root_isolate_(is_group_root_isolate) {
   FML_DCHECK(isolate_snapshot_) << "Must contain a valid isolate snapshot.";
   phase_ = Phase::Uninitialized;
 }
@@ -152,7 +158,7 @@ std::string DartIsolate::GetServiceId() {
   return service_id;
 }
 
-bool DartIsolate::Initialize(Dart_Isolate dart_isolate, bool is_root_isolate) {
+bool DartIsolate::Initialize(Dart_Isolate dart_isolate) {
   TRACE_EVENT0("flutter", "DartIsolate::Initialize");
   if (phase_ != Phase::Uninitialized) {
     return false;
@@ -166,12 +172,6 @@ bool DartIsolate::Initialize(Dart_Isolate dart_isolate, bool is_root_isolate) {
     return false;
   }
 
-  auto* isolate_data = static_cast<std::shared_ptr<DartIsolate>*>(
-      Dart_IsolateGroupData(dart_isolate));
-  if (isolate_data->get() != this) {
-    return false;
-  }
-
   // After this point, isolate scopes can be safely used.
   SetIsolate(dart_isolate);
 
@@ -183,8 +183,7 @@ bool DartIsolate::Initialize(Dart_Isolate dart_isolate, bool is_root_isolate) {
 
   tonic::DartIsolateScope scope(isolate());
 
-  SetMessageHandlingTaskRunner(GetTaskRunners().GetUITaskRunner(),
-                               is_root_isolate);
+  SetMessageHandlingTaskRunner(GetTaskRunners().GetUITaskRunner());
 
   if (tonic::LogIfError(
           Dart_SetLibraryTagHandler(tonic::DartState::HandleLibraryTag))) {
@@ -204,9 +203,8 @@ fml::RefPtr<fml::TaskRunner> DartIsolate::GetMessageHandlingTaskRunner() const {
 }
 
 void DartIsolate::SetMessageHandlingTaskRunner(
-    fml::RefPtr<fml::TaskRunner> runner,
-    bool is_root_isolate) {
-  if (!is_root_isolate || !runner) {
+    fml::RefPtr<fml::TaskRunner> runner) {
+  if (!IsRootIsolate() || !runner) {
     return;
   }
 
@@ -255,7 +253,7 @@ bool DartIsolate::UpdateThreadPoolNames() const {
   return true;
 }
 
-bool DartIsolate::LoadLibraries(bool is_root_isolate) {
+bool DartIsolate::LoadLibraries() {
   TRACE_EVENT0("flutter", "DartIsolate::LoadLibraries");
   if (phase_ != Phase::Initialized) {
     return false;
@@ -265,11 +263,11 @@ bool DartIsolate::LoadLibraries(bool is_root_isolate) {
 
   DartIO::InitForIsolate();
 
-  DartUI::InitForIsolate(is_root_isolate);
+  DartUI::InitForIsolate(IsRootIsolate());
 
   const bool is_service_isolate = Dart_IsServiceIsolate(isolate());
 
-  DartRuntimeHooks::Install(is_root_isolate && !is_service_isolate,
+  DartRuntimeHooks::Install(IsRootIsolate() && !is_service_isolate,
                             GetAdvisoryScriptURI());
 
   if (!is_service_isolate) {
@@ -650,6 +648,7 @@ Dart_Isolate DartIsolate::DartIsolateGroupCreateCallback(
     Dart_IsolateFlags* flags,
     std::shared_ptr<DartIsolate>* parent_embedder_isolate,
     char** error) {
+  TRACE_EVENT0("flutter", "DartIsolate::DartIsolateGroupCreateCallback");
   if (parent_embedder_isolate == nullptr &&
       strcmp(advisory_script_uri, DART_VM_SERVICE_ISOLATE_NAME) == 0) {
     // The VM attempts to start the VM service for us on |Dart_Initialize|. In
@@ -677,6 +676,59 @@ Dart_Isolate DartIsolate::DartIsolateGroupCreateCallback(
       .first;
 }
 
+// |Dart_IsolateInitializeCallback|
+bool DartIsolate::DartIsolateInitializeCallback(void** child_callback_data,
+                                                char** error) {
+  TRACE_EVENT0("flutter", "DartIsolate::DartIsolateInitializeCallback");
+  Dart_Isolate isolate = Dart_CurrentIsolate();
+  if (isolate == nullptr) {
+    *error = strdup("Isolate should be available in initialize callback.");
+    FML_DLOG(ERROR) << *error;
+    return false;
+  }
+
+  auto* root_embedder_isolate = static_cast<std::shared_ptr<DartIsolate>*>(
+      Dart_CurrentIsolateGroupData());
+
+  TaskRunners null_task_runners(
+      (*root_embedder_isolate)->GetAdvisoryScriptURI(),
+      /* platform= */ nullptr, /* gpu= */ nullptr,
+      /* ui= */ nullptr,
+      /* io= */ nullptr);
+
+  auto embedder_isolate = std::make_unique<std::shared_ptr<DartIsolate>>(
+      std::shared_ptr<DartIsolate>(new DartIsolate(
+          (*root_embedder_isolate)->GetSettings(),         // settings
+          (*root_embedder_isolate)->GetIsolateSnapshot(),  // isolate_snapshot
+          null_task_runners,                               // task_runners
+          fml::WeakPtr<SnapshotDelegate>{},                // snapshot_delegate
+          fml::WeakPtr<IOManager>{},                       // io_manager
+          fml::RefPtr<SkiaUnrefQueue>{},                   // unref_queue
+          fml::WeakPtr<ImageDecoder>{},                    // image_decoder
+          (*root_embedder_isolate)
+              ->GetAdvisoryScriptURI(),  // advisory_script_uri
+          (*root_embedder_isolate)
+              ->GetAdvisoryScriptEntrypoint(),  // advisory_script_entrypoint
+          (*root_embedder_isolate)->child_isolate_preparer_,     // preparer
+          (*root_embedder_isolate)->isolate_create_callback_,    // on create
+          (*root_embedder_isolate)->isolate_shutdown_callback_,  // on shutdown
+          false,     // is_root_isolate
+          false)));  // is_group_root_isolate
+
+  // root isolate should have been created via CreateRootIsolate and
+  // CreateDartVMAndEmbedderObjectPair
+  if (!InitializeIsolate(*embedder_isolate, isolate, error)) {
+    return false;
+  }
+
+  // The ownership of the embedder object is controlled by the Dart VM. So the
+  // only reference returned to the caller is weak.
+  *child_callback_data = embedder_isolate.release();
+
+  Dart_EnterIsolate(isolate);
+  return true;
+}
+
 std::pair<Dart_Isolate, std::weak_ptr<DartIsolate>>
 DartIsolate::CreateDartVMAndEmbedderObjectPair(
     const char* advisory_script_uri,
@@ -717,11 +769,11 @@ DartIsolate::CreateDartVMAndEmbedderObjectPair(
             fml::WeakPtr<ImageDecoder>{},                   // image_decoder
             advisory_script_uri,         // advisory_script_uri
             advisory_script_entrypoint,  // advisory_script_entrypoint
-            (*raw_embedder_isolate)->child_isolate_preparer_,    // preparer
-            (*raw_embedder_isolate)->isolate_create_callback_,   // on create
-            (*raw_embedder_isolate)->isolate_shutdown_callback_  // on shutdown
-            ))
-
+            (*raw_embedder_isolate)->child_isolate_preparer_,     // preparer
+            (*raw_embedder_isolate)->isolate_create_callback_,    // on create
+            (*raw_embedder_isolate)->isolate_shutdown_callback_,  // on shutdown
+            is_root_isolate,
+            true))  // is_root_group_isolate
     );
   }
 
@@ -741,38 +793,53 @@ DartIsolate::CreateDartVMAndEmbedderObjectPair(
     return {nullptr, {}};
   }
 
-  if (!(*embedder_isolate)->Initialize(isolate, is_root_isolate)) {
+  if (!InitializeIsolate(*embedder_isolate, isolate, error)) {
+    return {nullptr, {}};
+  }
+
+  auto* isolate_data = static_cast<std::shared_ptr<DartIsolate>*>(
+      Dart_IsolateGroupData(isolate));
+  FML_DCHECK(isolate_data->get() == embedder_isolate->get());
+
+  auto weak_embedder_isolate = (*embedder_isolate)->GetWeakIsolatePtr();
+
+  // 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();
+  return {isolate, weak_embedder_isolate};
+}
+
+bool DartIsolate::InitializeIsolate(
+    std::shared_ptr<DartIsolate> embedder_isolate,
+    Dart_Isolate isolate,
+    char** error) {
+  TRACE_EVENT0("flutter", "DartIsolate::InitializeIsolate");
+  if (!embedder_isolate->Initialize(isolate)) {
     *error = strdup("Embedder could not initialize the Dart isolate.");
     FML_DLOG(ERROR) << *error;
-    return {nullptr, {}};
+    return false;
   }
 
-  if (!(*embedder_isolate)->LoadLibraries(is_root_isolate)) {
+  if (!embedder_isolate->LoadLibraries()) {
     *error =
         strdup("Embedder could not load libraries in the new Dart isolate.");
     FML_DLOG(ERROR) << *error;
-    return {nullptr, {}};
+    return false;
   }
 
-  auto weak_embedder_isolate = (*embedder_isolate)->GetWeakIsolatePtr();
-
   // Root isolates will be setup by the engine and the service isolate (which is
   // also a root isolate) by the utility routines in the VM. However, secondary
   // isolates will be run by the VM if they are marked as runnable.
-  if (!is_root_isolate) {
-    FML_DCHECK((*embedder_isolate)->child_isolate_preparer_);
-    if (!(*embedder_isolate)
-             ->child_isolate_preparer_((*embedder_isolate).get())) {
+  if (!embedder_isolate->IsRootIsolate()) {
+    FML_DCHECK(embedder_isolate->child_isolate_preparer_);
+    if (!embedder_isolate->child_isolate_preparer_(embedder_isolate.get())) {
       *error = strdup("Could not prepare the child isolate to run.");
       FML_DLOG(ERROR) << *error;
-      return {nullptr, {}};
+      return false;
     }
   }
 
-  // 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();
-  return {isolate, weak_embedder_isolate};
+  return true;
 }
 
 // |Dart_IsolateShutdownCallback|
@@ -783,12 +850,46 @@ void DartIsolate::DartIsolateShutdownCallback(
   FML_DLOG(INFO) << "DartIsolateShutdownCallback"
                  << " isolate_group_data " << isolate_group_data
                  << " isolate_data " << isolate_data;
-  isolate_group_data->get()->OnShutdownCallback();
+  isolate_data->get()->OnShutdownCallback();
 }
 
 // |Dart_IsolateGroupCleanupCallback|
 void DartIsolate::DartIsolateGroupCleanupCallback(
     std::shared_ptr<DartIsolate>* isolate_data) {
+  TRACE_EVENT0("flutter", "DartIsolate::DartIsolateGroupCleanupCallback");
+  FML_DLOG(INFO) << "DartIsolateGroupCleanupCallback isolate_data "
+                 << isolate_data;
+
+  delete isolate_data;
+}
+
+// |Dart_IsolateCleanupCallback|
+void DartIsolate::DartIsolateCleanupCallback(
+    std::shared_ptr<DartIsolate>* isolate_group_data,
+    std::shared_ptr<DartIsolate>* isolate_data) {
+  TRACE_EVENT0("flutter", "DartIsolate::DartIsolateCleanupCallback");
+
+  if ((*isolate_data)->IsRootIsolate()) {
+    // isolate_data will be cleaned up as part of IsolateGroup cleanup
+    FML_DLOG(INFO)
+        << "DartIsolateCleanupCallback no-op for root isolate isolate_data "
+        << isolate_data;
+    return;
+  }
+  if ((*isolate_data)->IsGroupRootIsolate()) {
+    // Even if isolate was not a root isolate(i.e. was spawned),
+    // it might have IsolateGroup created for it (when
+    // --no-enable-isolate-groups dart vm flag is used).
+    // Then its isolate_data will be cleaned up as part of IsolateGroup
+    // cleanup as well.
+    FML_DLOG(INFO) << "DartIsolateCleanupCallback no-op for group root isolate "
+                      "isolate_data "
+                   << isolate_data;
+    return;
+  }
+
+  FML_DLOG(INFO) << "DartIsolateCleanupCallback cleaned up isolate_data "
+                 << isolate_data;
   delete isolate_data;
 }
 

runtime/dart_isolate.h

@@ -401,6 +401,13 @@ class DartIsolate : public UIDartState {
   ///
   fml::RefPtr<fml::TaskRunner> GetMessageHandlingTaskRunner() const;
 
+  // Root isolate of the VM application
+  bool IsRootIsolate() const { return is_root_isolate_; }
+  // Isolate that owns IsolateGroup it lives in.
+  // When --no-enable-isolate-groups dart vm flag is set,
+  // all child isolates will have their own IsolateGroups.
+  bool IsGroupRootIsolate() const { return is_group_root_isolate_; }
+
  private:
   using ChildIsolatePreparer = std::function<bool(DartIsolate*)>;
 
@@ -425,6 +432,8 @@ class DartIsolate : public UIDartState {
   fml::RefPtr<fml::TaskRunner> message_handling_task_runner_;
   const fml::closure isolate_create_callback_;
   const fml::closure isolate_shutdown_callback_;
+  const bool is_root_isolate_;
+  const bool is_group_root_isolate_;
 
   DartIsolate(const Settings& settings,
               fml::RefPtr<const DartSnapshot> isolate_snapshot,
@@ -437,18 +446,17 @@ class DartIsolate : public UIDartState {
               std::string advisory_script_entrypoint,
               ChildIsolatePreparer child_isolate_preparer,
               fml::closure isolate_create_callback,
-              fml::closure isolate_shutdown_callback);
-
-  FML_WARN_UNUSED_RESULT bool Initialize(Dart_Isolate isolate,
-                                         bool is_root_isolate);
+              fml::closure isolate_shutdown_callback,
+              bool is_root_isolate,
+              bool is_group_root_isolate);
+  FML_WARN_UNUSED_RESULT bool Initialize(Dart_Isolate isolate);
 
-  void SetMessageHandlingTaskRunner(fml::RefPtr<fml::TaskRunner> runner,
-                                    bool is_root_isolate);
+  void SetMessageHandlingTaskRunner(fml::RefPtr<fml::TaskRunner> runner);
 
   bool LoadKernel(std::shared_ptr<const fml::Mapping> mapping, bool last_piece);
 
   FML_WARN_UNUSED_RESULT
-  bool LoadLibraries(bool is_root_isolate);
+  bool LoadLibraries();
 
   bool UpdateThreadPoolNames() const;
 
@@ -467,6 +475,10 @@ class DartIsolate : public UIDartState {
       std::shared_ptr<DartIsolate>* embedder_isolate,
       char** error);
 
+  // |Dart_IsolateInitializeCallback|
+  static bool DartIsolateInitializeCallback(void** child_callback_data,
+                                            char** error);
+
   static Dart_Isolate DartCreateAndStartServiceIsolate(
       const char* package_root,
       const char* package_config,
@@ -485,11 +497,20 @@ class DartIsolate : public UIDartState {
       bool is_root_isolate,
       char** error);
 
+  static bool InitializeIsolate(std::shared_ptr<DartIsolate> embedder_isolate,
+                                Dart_Isolate isolate,
+                                char** error);
+
   // |Dart_IsolateShutdownCallback|
   static void DartIsolateShutdownCallback(
       std::shared_ptr<DartIsolate>* isolate_group_data,
       std::shared_ptr<DartIsolate>* isolate_data);
 
+  // |Dart_IsolateCleanupCallback|
+  static void DartIsolateCleanupCallback(
+      std::shared_ptr<DartIsolate>* isolate_group_data,
+      std::shared_ptr<DartIsolate>* isolate_data);
+
   // |Dart_IsolateGroupCleanupCallback|
   static void DartIsolateGroupCleanupCallback(
       std::shared_ptr<DartIsolate>* isolate_group_data);

runtime/dart_isolate_unittests.cc

@@ -364,6 +364,7 @@ TEST_F(DartIsolateTest, CanSaveCompilationTrace) {
 TEST_F(DartIsolateTest, CanLaunchSecondaryIsolates) {
   fml::CountDownLatch latch(3);
   fml::AutoResetWaitableEvent child_shutdown_latch;
+  fml::AutoResetWaitableEvent root_isolate_shutdown_latch;
   AddNativeCallback("NotifyNative",
                     CREATE_NATIVE_ENTRY(([&latch](Dart_NativeArguments args) {
                       latch.CountDown();
@@ -376,6 +377,9 @@ TEST_F(DartIsolateTest, CanLaunchSecondaryIsolates) {
         latch.CountDown();
       })));
   auto settings = CreateSettingsForFixture();
+  settings.root_isolate_shutdown_callback = [&root_isolate_shutdown_latch]() {
+    root_isolate_shutdown_latch.Signal();
+  };
   settings.isolate_shutdown_callback = [&child_shutdown_latch]() {
     child_shutdown_latch.Signal();
   };
@@ -385,6 +389,7 @@ TEST_F(DartIsolateTest, CanLaunchSecondaryIsolates) {
   ASSERT_TRUE(isolate);
   ASSERT_EQ(isolate->get()->GetPhase(), DartIsolate::Phase::Running);
   child_shutdown_latch.Wait();  // wait for child isolate to shutdown first
+  ASSERT_FALSE(root_isolate_shutdown_latch.IsSignaledForTest());
   latch.Wait();  // wait for last NotifyNative called by main isolate
   // root isolate will be auto-shutdown
 }

runtime/dart_vm.cc

@@ -391,9 +391,14 @@ DartVM::DartVM(std::shared_ptr<const DartVMData> vm_data,
         vm_data_->GetVMSnapshot().GetInstructionsMapping();
     params.create_group = reinterpret_cast<decltype(params.create_group)>(
         DartIsolate::DartIsolateGroupCreateCallback);
+    params.initialize_isolate =
+        reinterpret_cast<decltype(params.initialize_isolate)>(
+            DartIsolate::DartIsolateInitializeCallback);
     params.shutdown_isolate =
         reinterpret_cast<decltype(params.shutdown_isolate)>(
             DartIsolate::DartIsolateShutdownCallback);
+    params.cleanup_isolate = reinterpret_cast<decltype(params.cleanup_isolate)>(
+        DartIsolate::DartIsolateCleanupCallback);
     params.cleanup_group = reinterpret_cast<decltype(params.cleanup_group)>(
         DartIsolate::DartIsolateGroupCleanupCallback);
     params.thread_exit = ThreadExitCallback;