Reland "Create root isolate asynchronously (flutter#20142)" (flutter#…

…21747)

This reverts commit 5585ed9.

Additionally, the following _flutter.runInView deadlock is fixed.

Previously, a deadlock would occur when service protocol
_flutter.runInView is used to restart the engine wihtout tearing down
the shell: the shared mutex of the service protocol will be locked
during the restart as it's in the middle of handling a service protocol
message; if ServiceProtocol::AddHandler is also called during the
restart, the deadlock happens as AddHandler also requires such lock.

test/integration.shard/background_isolate_test.dart would fail
without this fix.

runtime/runtime_controller.cc

@@ -18,7 +18,10 @@ namespace flutter {
 
 RuntimeController::RuntimeController(RuntimeDelegate& client,
                                      TaskRunners p_task_runners)
-    : client_(client), vm_(nullptr), task_runners_(p_task_runners) {}
+    : client_(client),
+      vm_(nullptr),
+      task_runners_(p_task_runners),
+      weak_factory_(this) {}
 
 RuntimeController::RuntimeController(
     RuntimeDelegate& p_client,
@@ -52,49 +55,81 @@ RuntimeController::RuntimeController(
       platform_data_(std::move(p_platform_data)),
       isolate_create_callback_(p_isolate_create_callback),
       isolate_shutdown_callback_(p_isolate_shutdown_callback),
-      persistent_isolate_data_(std::move(p_persistent_isolate_data)) {
-  // Create the root isolate as soon as the runtime controller is initialized.
+      persistent_isolate_data_(std::move(p_persistent_isolate_data)),
+      weak_factory_(this) {
+  // Create the root isolate as soon as the runtime controller is initialized,
+  // but not using a synchronous way to avoid blocking the platform thread a
+  // long time as it is waiting while creating `Shell` on that platform thread.
   // It will be run at a later point when the engine provides a run
   // configuration and then runs the isolate.
-  auto strong_root_isolate =
-      DartIsolate::CreateRootIsolate(
-          vm_->GetVMData()->GetSettings(),                //
-          isolate_snapshot_,                              //
-          task_runners_,                                  //
-          std::make_unique<PlatformConfiguration>(this),  //
-          snapshot_delegate_,                             //
-          hint_freed_delegate_,                           //
-          io_manager_,                                    //
-          unref_queue_,                                   //
-          image_decoder_,                                 //
-          p_advisory_script_uri,                          //
-          p_advisory_script_entrypoint,                   //
-          nullptr,                                        //
-          isolate_create_callback_,                       //
-          isolate_shutdown_callback_                      //
-          )
-          .lock();
-
-  FML_CHECK(strong_root_isolate) << "Could not create root isolate.";
-
-  // The root isolate ivar is weak.
-  root_isolate_ = strong_root_isolate;
-
-  strong_root_isolate->SetReturnCodeCallback([this](uint32_t code) {
-    root_isolate_return_code_ = {true, code};
-  });
-
-  if (auto* platform_configuration = GetPlatformConfigurationIfAvailable()) {
-    tonic::DartState::Scope scope(strong_root_isolate);
-    platform_configuration->DidCreateIsolate();
-    if (!FlushRuntimeStateToIsolate()) {
-      FML_DLOG(ERROR) << "Could not setup initial isolate state.";
-    }
-  } else {
-    FML_DCHECK(false) << "RuntimeController created without window binding.";
-  }
-
-  FML_DCHECK(Dart_CurrentIsolate() == nullptr);
+  create_and_config_root_isolate_ =
+      std::async(std::launch::deferred, [self = weak_factory_.GetWeakPtr()]() {
+        if (!self) {
+          return;
+        }
+
+        auto strong_root_isolate =
+            DartIsolate::CreateRootIsolate(
+                self->vm_->GetVMData()->GetSettings(),                //
+                self->isolate_snapshot_,                              //
+                self->task_runners_,                                  //
+                std::make_unique<PlatformConfiguration>(self.get()),  //
+                self->snapshot_delegate_,                             //
+                self->hint_freed_delegate_,                           //
+                self->io_manager_,                                    //
+                self->unref_queue_,                                   //
+                self->image_decoder_,                                 //
+                self->advisory_script_uri_,                           //
+                self->advisory_script_entrypoint_,                    //
+                nullptr,                                              //
+                self->isolate_create_callback_,                       //
+                self->isolate_shutdown_callback_                      //
+                )
+                .lock();
+
+        FML_CHECK(strong_root_isolate) << "Could not create root isolate.";
+
+        // The root isolate ivar is weak.
+        self->root_isolate_ = strong_root_isolate;
+
+        strong_root_isolate->SetReturnCodeCallback([self](uint32_t code) {
+          if (!self) {
+            return;
+          }
+
+          self->root_isolate_return_code_ = {true, code};
+        });
+
+        if (auto* platform_configuration =
+                self->GetPlatformConfigurationIfAvailable()) {
+          tonic::DartState::Scope scope(strong_root_isolate);
+          platform_configuration->DidCreateIsolate();
+          if (!self->FlushRuntimeStateToIsolate()) {
+            FML_DLOG(ERROR) << "Could not setup initial isolate state.";
+          }
+        } else {
+          FML_DCHECK(false)
+              << "RuntimeController created without window binding.";
+        }
+
+        FML_DCHECK(Dart_CurrentIsolate() == nullptr);
+
+        self->client_.OnRootIsolateCreated();
+        return;
+      });
+
+  // We're still trying to create the root isolate as soon as possible here on
+  // the UI thread although it's deferred a little bit by
+  // std::async(std::launch::deferred, ...). So the callers of `GetRootIsolate`
+  // should get a quick return after this UI thread task.
+  task_runners_.GetUITaskRunner()->PostTask(
+      [self = weak_factory_.GetWeakPtr()]() {
+        if (!self) {
+          return;
+        }
+
+        self->GetRootIsolate();
+      });
 }
 
 RuntimeController::~RuntimeController() {
@@ -110,8 +145,8 @@ RuntimeController::~RuntimeController() {
   }
 }
 
-bool RuntimeController::IsRootIsolateRunning() const {
-  std::shared_ptr<DartIsolate> root_isolate = root_isolate_.lock();
+bool RuntimeController::IsRootIsolateRunning() {
+  std::shared_ptr<DartIsolate> root_isolate = GetRootIsolate().lock();
   if (root_isolate) {
     return root_isolate->GetPhase() == DartIsolate::Phase::Running;
   }
@@ -238,7 +273,7 @@ bool RuntimeController::ReportTimings(std::vector<int64_t> timings) {
 }
 
 bool RuntimeController::NotifyIdle(int64_t deadline, size_t freed_hint) {
-  std::shared_ptr<DartIsolate> root_isolate = root_isolate_.lock();
+  std::shared_ptr<DartIsolate> root_isolate = GetRootIsolate().lock();
   if (!root_isolate) {
     return false;
   }
@@ -298,7 +333,7 @@ bool RuntimeController::DispatchSemanticsAction(int32_t id,
 
 PlatformConfiguration*
 RuntimeController::GetPlatformConfigurationIfAvailable() {
-  std::shared_ptr<DartIsolate> root_isolate = root_isolate_.lock();
+  std::shared_ptr<DartIsolate> root_isolate = GetRootIsolate().lock();
   return root_isolate ? root_isolate->platform_configuration() : nullptr;
 }
 
@@ -360,17 +395,17 @@ RuntimeController::ComputePlatformResolvedLocale(
 }
 
 Dart_Port RuntimeController::GetMainPort() {
-  std::shared_ptr<DartIsolate> root_isolate = root_isolate_.lock();
+  std::shared_ptr<DartIsolate> root_isolate = GetRootIsolate().lock();
   return root_isolate ? root_isolate->main_port() : ILLEGAL_PORT;
 }
 
 std::string RuntimeController::GetIsolateName() {
-  std::shared_ptr<DartIsolate> root_isolate = root_isolate_.lock();
+  std::shared_ptr<DartIsolate> root_isolate = GetRootIsolate().lock();
   return root_isolate ? root_isolate->debug_name() : "";
 }
 
 bool RuntimeController::HasLivePorts() {
-  std::shared_ptr<DartIsolate> root_isolate = root_isolate_.lock();
+  std::shared_ptr<DartIsolate> root_isolate = GetRootIsolate().lock();
   if (!root_isolate) {
     return false;
   }
@@ -379,11 +414,20 @@ bool RuntimeController::HasLivePorts() {
 }
 
 tonic::DartErrorHandleType RuntimeController::GetLastError() {
-  std::shared_ptr<DartIsolate> root_isolate = root_isolate_.lock();
+  std::shared_ptr<DartIsolate> root_isolate = GetRootIsolate().lock();
   return root_isolate ? root_isolate->GetLastError() : tonic::kNoError;
 }
 
 std::weak_ptr<DartIsolate> RuntimeController::GetRootIsolate() {
+  std::shared_ptr<DartIsolate> root_isolate = root_isolate_.lock();
+  if (root_isolate) {
+    return root_isolate_;
+  }
+
+  // Root isolate is not yet created, get it and do some configuration.
+  FML_DCHECK(create_and_config_root_isolate_.valid());
+  create_and_config_root_isolate_.get();
+
   return root_isolate_;
 }
 

runtime/runtime_controller.h

@@ -5,6 +5,7 @@
 #ifndef FLUTTER_RUNTIME_RUNTIME_CONTROLLER_H_
 #define FLUTTER_RUNTIME_RUNTIME_CONTROLLER_H_
 
+#include <future>
 #include <memory>
 #include <vector>
 
@@ -348,7 +349,7 @@ class RuntimeController : public PlatformConfigurationClient {
   ///
   /// @return     True if root isolate running, False otherwise.
   ///
-  virtual bool IsRootIsolateRunning() const;
+  virtual bool IsRootIsolateRunning();
 
   //----------------------------------------------------------------------------
   /// @brief      Dispatch the specified platform message to running root
@@ -430,7 +431,10 @@ class RuntimeController : public PlatformConfigurationClient {
   /// @brief      Get a weak pointer to the root Dart isolate. This isolate may
   ///             only be locked on the UI task runner. Callers use this
   ///             accessor to transition to the root isolate to the running
-  ///             phase.
+  ///             phase. Note that it might take times if the isolate is not yet
+  ///             created, which should be done in a subsequence task after
+  ///             constructing `RuntimeController`, or it should get a quick
+  ///             return otherwise.
   ///
   /// @return     The root isolate reference.
   ///
@@ -480,11 +484,16 @@ class RuntimeController : public PlatformConfigurationClient {
   std::string advisory_script_entrypoint_;
   std::function<void(int64_t)> idle_notification_callback_;
   PlatformData platform_data_;
+  std::future<void> create_and_config_root_isolate_;
+  // Note that `root_isolate_` is created asynchronously from the constructor of
+  // `RuntimeController`, be careful to use it directly while it might have not
+  // been created yet. Call `GetRootIsolate()` instead which guarantees that.
   std::weak_ptr<DartIsolate> root_isolate_;
   std::pair<bool, uint32_t> root_isolate_return_code_ = {false, 0};
   const fml::closure isolate_create_callback_;
   const fml::closure isolate_shutdown_callback_;
   std::shared_ptr<const fml::Mapping> persistent_isolate_data_;
+  fml::WeakPtrFactory<RuntimeController> weak_factory_;
 
   PlatformConfiguration* GetPlatformConfigurationIfAvailable();
 

runtime/runtime_delegate.h

@@ -32,6 +32,8 @@ class RuntimeDelegate {
 
   virtual FontCollection& GetFontCollection() = 0;
 
+  virtual void OnRootIsolateCreated() = 0;
+
   virtual void UpdateIsolateDescription(const std::string isolate_name,
                                         int64_t isolate_port) = 0;
 

shell/common/engine.cc

@@ -501,6 +501,10 @@ void Engine::HandlePlatformMessage(fml::RefPtr<PlatformMessage> message) {
   }
 }
 
+void Engine::OnRootIsolateCreated() {
+  delegate_.OnRootIsolateCreated();
+}
+
 void Engine::UpdateIsolateDescription(const std::string isolate_name,
                                       int64_t isolate_port) {
   delegate_.UpdateIsolateDescription(isolate_name, isolate_port);

shell/common/engine.h

@@ -189,6 +189,15 @@ class Engine final : public RuntimeDelegate,
     ///
     virtual void OnPreEngineRestart() = 0;
 
+    //--------------------------------------------------------------------------
+    /// @brief      Notifies the shell that the root isolate is created.
+    ///             Currently, this information is to add to the service
+    ///             protocol list of available root isolates running in the VM
+    ///             and their names so that the appropriate isolate can be
+    ///             selected in the tools for debugging and instrumentation.
+    ///
+    virtual void OnRootIsolateCreated() = 0;
+
     //--------------------------------------------------------------------------
     /// @brief      Notifies the shell of the name of the root isolate and its
     ///             port when that isolate is launched, restarted (in the
@@ -800,6 +809,9 @@ class Engine final : public RuntimeDelegate,
   // |RuntimeDelegate|
   void HandlePlatformMessage(fml::RefPtr<PlatformMessage> message) override;
 
+  // |RuntimeDelegate|
+  void OnRootIsolateCreated() override;
+
   // |RuntimeDelegate|
   void UpdateIsolateDescription(const std::string isolate_name,
                                 int64_t isolate_port) override;

shell/common/engine_unittests.cc

@@ -27,6 +27,7 @@ class MockDelegate : public Engine::Delegate {
   MOCK_METHOD1(OnEngineHandlePlatformMessage,
                void(fml::RefPtr<PlatformMessage>));
   MOCK_METHOD0(OnPreEngineRestart, void());
+  MOCK_METHOD0(OnRootIsolateCreated, void());
   MOCK_METHOD2(UpdateIsolateDescription, void(const std::string, int64_t));
   MOCK_METHOD1(SetNeedsReportTimings, void(bool));
   MOCK_METHOD1(ComputePlatformResolvedLocale,
@@ -49,6 +50,7 @@ class MockRuntimeDelegate : public RuntimeDelegate {
                void(SemanticsNodeUpdates, CustomAccessibilityActionUpdates));
   MOCK_METHOD1(HandlePlatformMessage, void(fml::RefPtr<PlatformMessage>));
   MOCK_METHOD0(GetFontCollection, FontCollection&());
+  MOCK_METHOD0(OnRootIsolateCreated, void());
   MOCK_METHOD2(UpdateIsolateDescription, void(const std::string, int64_t));
   MOCK_METHOD1(SetNeedsReportTimings, void(bool));
   MOCK_METHOD1(ComputePlatformResolvedLocale,
@@ -60,7 +62,7 @@ class MockRuntimeController : public RuntimeController {
  public:
   MockRuntimeController(RuntimeDelegate& client, TaskRunners p_task_runners)
       : RuntimeController(client, p_task_runners) {}
-  MOCK_CONST_METHOD0(IsRootIsolateRunning, bool());
+  MOCK_METHOD0(IsRootIsolateRunning, bool());
   MOCK_METHOD1(DispatchPlatformMessage, bool(fml::RefPtr<PlatformMessage>));
 };
 

shell/common/shell.cc

@@ -564,8 +564,6 @@ bool Shell::Setup(std::unique_ptr<PlatformView> platform_view,
 
   is_setup_ = true;
 
-  vm_->GetServiceProtocol()->AddHandler(this, GetServiceProtocolDescription());
-
   PersistentCache::GetCacheForProcess()->AddWorkerTaskRunner(
       task_runners_.GetIOTaskRunner());
 
@@ -1150,6 +1148,23 @@ void Shell::OnPreEngineRestart() {
   latch.Wait();
 }
 
+// |Engine::Delegate|
+void Shell::OnRootIsolateCreated() {
+  if (is_added_to_service_protocol_) {
+    return;
+  }
+  auto description = GetServiceProtocolDescription();
+  fml::TaskRunner::RunNowOrPostTask(
+      task_runners_.GetPlatformTaskRunner(),
+      [self = weak_factory_.GetWeakPtr(),
+       description = std::move(description)]() {
+        if (self) {
+          self->vm_->GetServiceProtocol()->AddHandler(self.get(), description);
+        }
+      });
+  is_added_to_service_protocol_ = true;
+}
+
 // |Engine::Delegate|
 void Shell::UpdateIsolateDescription(const std::string isolate_name,
                                      int64_t isolate_port) {
@@ -1295,9 +1310,15 @@ bool Shell::HandleServiceProtocolMessage(
 // |ServiceProtocol::Handler|
 ServiceProtocol::Handler::Description Shell::GetServiceProtocolDescription()
     const {
+  FML_DCHECK(task_runners_.GetUITaskRunner()->RunsTasksOnCurrentThread());
+
+  if (!weak_engine_) {
+    return ServiceProtocol::Handler::Description();
+  }
+
   return {
-      engine_->GetUIIsolateMainPort(),
-      engine_->GetUIIsolateName(),
+      weak_engine_->GetUIIsolateMainPort(),
+      weak_engine_->GetUIIsolateName(),
   };
 }
 

shell/common/shell.h

@@ -411,6 +411,7 @@ class Shell final : public PlatformView::Delegate,
                      >
       service_protocol_handlers_;
   bool is_setup_ = false;
+  bool is_added_to_service_protocol_ = false;
   uint64_t next_pointer_flow_id_ = 0;
 
   bool first_frame_rasterized_ = false;
@@ -537,6 +538,9 @@ class Shell final : public PlatformView::Delegate,
   // |Engine::Delegate|
   void OnPreEngineRestart() override;
 
+  // |Engine::Delegate|
+  void OnRootIsolateCreated() override;
+
   // |Engine::Delegate|
   void UpdateIsolateDescription(const std::string isolate_name,
                                 int64_t isolate_port) override;