From ed6375fbb66669350a1a9fdd8f7678b6f1f3d311 Mon Sep 17 00:00:00 2001 From: Chinmay Garde Date: Wed, 21 Oct 2020 13:56:27 -0700 Subject: [PATCH] Ensure root isolate create callback is invoked before the isolate is in the running phase. Embedders that have access to the Dart native API (only Fuchsia now) may perform library setup in the isolate create callback. The engine used to depend on the fact the root isolate entrypoint is invoked in the next iteration of message loop (via the `_startIsolate` trampoline in `isolate_patch.dart`) to ensure that library setup occur before the main entrypoint was invoked. However, due to differences in the way in which message loops are setup in Fuchsia, this entrypoint was run before the callback could be executed. Dart code on Fuchsia also has the ability to access the underlying event loops directly. This patch moves the invocation of the create callback to before user dart code has a chance to run. This difference in behavior on Fuchsia became an issue when the isolate initialization was reworked in https://github.com/flutter/engine/pull/21820 for null-safety. Another issue was discovered in that the callback was being invoked twice, I fixed that too and added a test. Fixes https://github.com/flutter/flutter/issues/68732 --- common/settings.h | 6 +++++- runtime/dart_isolate.cc | 26 +++++++++++--------------- runtime/dart_isolate.h | 8 +------- runtime/dart_isolate_unittests.cc | 27 +++++++++++++++++++++++++++ shell/platform/embedder/embedder.cc | 5 ++--- 5 files changed, 46 insertions(+), 26 deletions(-) diff --git a/common/settings.h b/common/settings.h index 8de173511d4c4..c956423ca8914 100644 --- a/common/settings.h +++ b/common/settings.h @@ -59,6 +59,8 @@ using MappingsCallback = std::function; using FrameRasterizedCallback = std::function; +class DartIsolate; + struct Settings { Settings(); @@ -169,7 +171,9 @@ struct Settings { TaskObserverRemove task_observer_remove; // The main isolate is current when this callback is made. This is a good spot // to perform native Dart bindings for libraries not built in. - fml::closure root_isolate_create_callback; + std::function root_isolate_create_callback; + // TODO(68738): Update isolate callbacks in settings to accept an additional + // DartIsolate parameter. fml::closure isolate_create_callback; // The isolate is not current and may have already been destroyed when this // call is made. diff --git a/runtime/dart_isolate.cc b/runtime/dart_isolate.cc index ae655f00e07cf..b834957a9281a 100644 --- a/runtime/dart_isolate.cc +++ b/runtime/dart_isolate.cc @@ -148,21 +148,21 @@ std::weak_ptr DartIsolate::CreateRunningRootIsolate( return {}; } - if (!isolate->RunFromLibrary(dart_entrypoint_library, // - dart_entrypoint, // - settings.dart_entrypoint_args, // - settings.root_isolate_create_callback // + if (settings.root_isolate_create_callback) { + // Isolate callbacks always occur in isolate scope and before user code has + // had a chance to run. + tonic::DartState::Scope scope(isolate.get()); + settings.root_isolate_create_callback(*isolate.get()); + } + + if (!isolate->RunFromLibrary(dart_entrypoint_library, // + dart_entrypoint, // + settings.dart_entrypoint_args // )) { FML_LOG(ERROR) << "Could not run the run main Dart entrypoint."; return {}; } - if (settings.root_isolate_create_callback) { - // Isolate callbacks always occur in isolate scope. - tonic::DartState::Scope scope(isolate.get()); - settings.root_isolate_create_callback(); - } - if (settings.root_isolate_shutdown_callback) { isolate->AddIsolateShutdownCallback( settings.root_isolate_shutdown_callback); @@ -617,8 +617,7 @@ bool DartIsolate::MarkIsolateRunnable() { bool DartIsolate::RunFromLibrary(std::optional library_name, std::optional entrypoint, - const std::vector& args, - const fml::closure& on_run) { + const std::vector& args) { TRACE_EVENT0("flutter", "DartIsolate::RunFromLibrary"); if (phase_ != Phase::Ready) { return false; @@ -644,9 +643,6 @@ bool DartIsolate::RunFromLibrary(std::optional library_name, phase_ = Phase::Running; - if (on_run) { - on_run(); - } return true; } diff --git a/runtime/dart_isolate.h b/runtime/dart_isolate.h index b7ef2be67e3fb..5e6913d52b185 100644 --- a/runtime/dart_isolate.h +++ b/runtime/dart_isolate.h @@ -342,20 +342,14 @@ class DartIsolate : public UIDartState { /// supplied entrypoint. /// @param[in] entrypoint The entrypoint in `library_name` /// @param[in] args A list of string arguments to the entrypoint. - /// @param[in] on_run A callback to run in isolate scope after the - /// main entrypoint has been invoked. There is no - /// isolate scope current on the thread once this - /// method returns. /// /// @return If the isolate successfully transitioned to the running phase /// and the main entrypoint was invoked. /// [[nodiscard]] bool RunFromLibrary(std::optional library_name, std::optional entrypoint, - const std::vector& args, - const fml::closure& on_run = nullptr); + const std::vector& args); - public: //---------------------------------------------------------------------------- /// @brief Transition the isolate to the `Phase::Shutdown` phase. The /// only thing left to do is to collect the isolate. diff --git a/runtime/dart_isolate_unittests.cc b/runtime/dart_isolate_unittests.cc index bfdbfa9914669..f6263ddcd882d 100644 --- a/runtime/dart_isolate_unittests.cc +++ b/runtime/dart_isolate_unittests.cc @@ -375,5 +375,32 @@ TEST_F(DartIsolateTest, CanCreateServiceIsolate) { ASSERT_TRUE(root_isolate->Shutdown()); } +TEST_F(DartIsolateTest, + RootIsolateCreateCallbackIsMadeOnceAndBeforeIsolateRunning) { + ASSERT_FALSE(DartVMRef::IsInstanceRunning()); + auto settings = CreateSettingsForFixture(); + size_t create_callback_count = 0u; + settings.root_isolate_create_callback = + [&create_callback_count](const auto& isolate) { + ASSERT_EQ(isolate.GetPhase(), DartIsolate::Phase::Ready); + create_callback_count++; + ASSERT_NE(::Dart_CurrentIsolate(), nullptr); + }; + auto vm_ref = DartVMRef::Create(settings); + TaskRunners task_runners(GetCurrentTestName(), // + GetCurrentTaskRunner(), // + GetCurrentTaskRunner(), // + GetCurrentTaskRunner(), // + GetCurrentTaskRunner() // + ); + { + auto isolate = RunDartCodeInIsolate(vm_ref, settings, task_runners, "main", + {}, GetFixturesPath()); + ASSERT_TRUE(isolate); + ASSERT_EQ(isolate->get()->GetPhase(), DartIsolate::Phase::Running); + } + ASSERT_EQ(create_callback_count, 1u); +} + } // namespace testing } // namespace flutter diff --git a/shell/platform/embedder/embedder.cc b/shell/platform/embedder/embedder.cc index 9248eb7710b30..c72a272e44990 100644 --- a/shell/platform/embedder/embedder.cc +++ b/shell/platform/embedder/embedder.cc @@ -841,9 +841,8 @@ FlutterEngineResult FlutterEngineInitialize(size_t version, if (SAFE_ACCESS(args, root_isolate_create_callback, nullptr) != nullptr) { VoidCallback callback = SAFE_ACCESS(args, root_isolate_create_callback, nullptr); - settings.root_isolate_create_callback = [callback, user_data]() { - callback(user_data); - }; + settings.root_isolate_create_callback = + [callback, user_data](const auto& isolate) { callback(user_data); }; } flutter::PlatformViewEmbedder::UpdateSemanticsNodesCallback