From 190fd8eb5131c76fdbda260846416e05aab669b9 Mon Sep 17 00:00:00 2001 From: Yuqian Li Date: Mon, 12 Oct 2020 12:02:30 -0700 Subject: [PATCH] Reland "Create root isolate asynchronously (#20142)" (#21747) This reverts commit 5585ed99039efb3705025e1c58170cdb86af111b. 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 | 144 ++++++++++++++++++++----------- runtime/runtime_controller.h | 13 ++- runtime/runtime_delegate.h | 2 + shell/common/engine.cc | 4 + shell/common/engine.h | 12 +++ shell/common/engine_unittests.cc | 4 +- shell/common/shell.cc | 29 ++++++- shell/common/shell.h | 4 + 8 files changed, 155 insertions(+), 57 deletions(-) diff --git a/runtime/runtime_controller.cc b/runtime/runtime_controller.cc index e2b26c2f2f8ab..36e4e1b4e74e1 100644 --- a/runtime/runtime_controller.cc +++ b/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(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(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 root_isolate = root_isolate_.lock(); +bool RuntimeController::IsRootIsolateRunning() { + std::shared_ptr root_isolate = GetRootIsolate().lock(); if (root_isolate) { return root_isolate->GetPhase() == DartIsolate::Phase::Running; } @@ -238,7 +273,7 @@ bool RuntimeController::ReportTimings(std::vector timings) { } bool RuntimeController::NotifyIdle(int64_t deadline, size_t freed_hint) { - std::shared_ptr root_isolate = root_isolate_.lock(); + std::shared_ptr 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 root_isolate = root_isolate_.lock(); + std::shared_ptr 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 root_isolate = root_isolate_.lock(); + std::shared_ptr root_isolate = GetRootIsolate().lock(); return root_isolate ? root_isolate->main_port() : ILLEGAL_PORT; } std::string RuntimeController::GetIsolateName() { - std::shared_ptr root_isolate = root_isolate_.lock(); + std::shared_ptr root_isolate = GetRootIsolate().lock(); return root_isolate ? root_isolate->debug_name() : ""; } bool RuntimeController::HasLivePorts() { - std::shared_ptr root_isolate = root_isolate_.lock(); + std::shared_ptr root_isolate = GetRootIsolate().lock(); if (!root_isolate) { return false; } @@ -379,11 +414,20 @@ bool RuntimeController::HasLivePorts() { } tonic::DartErrorHandleType RuntimeController::GetLastError() { - std::shared_ptr root_isolate = root_isolate_.lock(); + std::shared_ptr root_isolate = GetRootIsolate().lock(); return root_isolate ? root_isolate->GetLastError() : tonic::kNoError; } std::weak_ptr RuntimeController::GetRootIsolate() { + std::shared_ptr 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_; } diff --git a/runtime/runtime_controller.h b/runtime/runtime_controller.h index 89ed112055b74..20e7abc532984 100644 --- a/runtime/runtime_controller.h +++ b/runtime/runtime_controller.h @@ -5,6 +5,7 @@ #ifndef FLUTTER_RUNTIME_RUNTIME_CONTROLLER_H_ #define FLUTTER_RUNTIME_RUNTIME_CONTROLLER_H_ +#include #include #include @@ -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 idle_notification_callback_; PlatformData platform_data_; + std::future 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 root_isolate_; std::pair root_isolate_return_code_ = {false, 0}; const fml::closure isolate_create_callback_; const fml::closure isolate_shutdown_callback_; std::shared_ptr persistent_isolate_data_; + fml::WeakPtrFactory weak_factory_; PlatformConfiguration* GetPlatformConfigurationIfAvailable(); diff --git a/runtime/runtime_delegate.h b/runtime/runtime_delegate.h index 20059827b8150..55978b4dbc39f 100644 --- a/runtime/runtime_delegate.h +++ b/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; diff --git a/shell/common/engine.cc b/shell/common/engine.cc index 348e69a00a9aa..310ebc4ce925b 100644 --- a/shell/common/engine.cc +++ b/shell/common/engine.cc @@ -501,6 +501,10 @@ void Engine::HandlePlatformMessage(fml::RefPtr message) { } } +void Engine::OnRootIsolateCreated() { + delegate_.OnRootIsolateCreated(); +} + void Engine::UpdateIsolateDescription(const std::string isolate_name, int64_t isolate_port) { delegate_.UpdateIsolateDescription(isolate_name, isolate_port); diff --git a/shell/common/engine.h b/shell/common/engine.h index 81771c00935ab..b35a7ff85ab07 100644 --- a/shell/common/engine.h +++ b/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 message) override; + // |RuntimeDelegate| + void OnRootIsolateCreated() override; + // |RuntimeDelegate| void UpdateIsolateDescription(const std::string isolate_name, int64_t isolate_port) override; diff --git a/shell/common/engine_unittests.cc b/shell/common/engine_unittests.cc index 3390507f68d9b..4ce3c6d6d7aad 100644 --- a/shell/common/engine_unittests.cc +++ b/shell/common/engine_unittests.cc @@ -27,6 +27,7 @@ class MockDelegate : public Engine::Delegate { MOCK_METHOD1(OnEngineHandlePlatformMessage, void(fml::RefPtr)); 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)); 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)); }; diff --git a/shell/common/shell.cc b/shell/common/shell.cc index c250ca25a8552..787d69b51653d 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -564,8 +564,6 @@ bool Shell::Setup(std::unique_ptr 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(), }; } diff --git a/shell/common/shell.h b/shell/common/shell.h index a345b0461e5fc..8e8fd18b48b3b 100644 --- a/shell/common/shell.h +++ b/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;