From e753820ca64886471febb7a9ea78db7f964c3268 Mon Sep 17 00:00:00 2001 From: Chinmay Garde Date: Thu, 22 Oct 2020 00:54:18 -0700 Subject: [PATCH] Isolates launched by the engine instance use the settings of that instance. This regression was introduced in https://github.com/flutter/engine/pull/21820 for sound-null safety. The settings used to launch the VM were incorrectly used to determine the isolate lifecycle callbacks. Since the first shell/engine in the process also starts the VM, these objects are usually identical. However, for subsequent engine shell/engine launches, the callbacks attached to the new settings object would be ignored. The unit-test harness is also structured in such a way that each test case tears down the VM before the next. So all existing tests created a bespoke VM for the test run, and, the tests that did create multiple isolates did not also test attaching callbacks to the settings object. Fixes https://github.com/flutter/engine/pull/22041 --- runtime/dart_isolate_unittests.cc | 28 ++++++++++++++++++++++++++++ runtime/runtime_controller.cc | 3 ++- runtime/runtime_controller.h | 2 ++ shell/common/engine.cc | 1 + shell/common/shell_unittests.cc | 23 +++++++++++++++++++++++ 5 files changed, 56 insertions(+), 1 deletion(-) diff --git a/runtime/dart_isolate_unittests.cc b/runtime/dart_isolate_unittests.cc index f6263ddcd882d..a90599fd8a3fb 100644 --- a/runtime/dart_isolate_unittests.cc +++ b/runtime/dart_isolate_unittests.cc @@ -402,5 +402,33 @@ TEST_F(DartIsolateTest, ASSERT_EQ(create_callback_count, 1u); } +TEST_F(DartIsolateTest, + IsolateCreateCallbacksTakeInstanceSettingsInsteadOfVMSettings) { + ASSERT_FALSE(DartVMRef::IsInstanceRunning()); + auto vm_settings = CreateSettingsForFixture(); + auto vm_ref = DartVMRef::Create(vm_settings); + auto instance_settings = vm_settings; + size_t create_callback_count = 0u; + instance_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); + }; + TaskRunners task_runners(GetCurrentTestName(), // + GetCurrentTaskRunner(), // + GetCurrentTaskRunner(), // + GetCurrentTaskRunner(), // + GetCurrentTaskRunner() // + ); + { + auto isolate = RunDartCodeInIsolate(vm_ref, instance_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/runtime/runtime_controller.cc b/runtime/runtime_controller.cc index a2ea4a00d24df..83d1292d3fd00 100644 --- a/runtime/runtime_controller.cc +++ b/runtime/runtime_controller.cc @@ -342,6 +342,7 @@ tonic::DartErrorHandleType RuntimeController::GetLastError() { } bool RuntimeController::LaunchRootIsolate( + const Settings& settings, std::optional dart_entrypoint, std::optional dart_entrypoint_library, std::unique_ptr isolate_configuration) { @@ -352,7 +353,7 @@ bool RuntimeController::LaunchRootIsolate( auto strong_root_isolate = DartIsolate::CreateRunningRootIsolate( - vm_->GetVMData()->GetSettings(), // + settings, // isolate_snapshot_, // task_runners_, // std::make_unique(this), // diff --git a/runtime/runtime_controller.h b/runtime/runtime_controller.h index 9b5ef99da617c..17b02b935f986 100644 --- a/runtime/runtime_controller.h +++ b/runtime/runtime_controller.h @@ -144,6 +144,7 @@ class RuntimeController : public PlatformConfigurationClient { /// runtime controller, `Clone` this runtime controller and /// Launch an isolate in that runtime controller instead. /// + /// @param[in] settings The per engine instance settings. /// @param[in] dart_entrypoint The dart entrypoint. If /// `std::nullopt` or empty, `main` will /// be attempted. @@ -156,6 +157,7 @@ class RuntimeController : public PlatformConfigurationClient { /// `DartIsolate::Phase::Running` phase. /// [[nodiscard]] bool LaunchRootIsolate( + const Settings& settings, std::optional dart_entrypoint, std::optional dart_entrypoint_library, std::unique_ptr isolate_configuration); diff --git a/shell/common/engine.cc b/shell/common/engine.cc index f3374a036070a..6c9304f1ad945 100644 --- a/shell/common/engine.cc +++ b/shell/common/engine.cc @@ -160,6 +160,7 @@ Engine::RunStatus Engine::Run(RunConfiguration configuration) { } if (!runtime_controller_->LaunchRootIsolate( + settings_, // configuration.GetEntrypoint(), // configuration.GetEntrypointLibrary(), // configuration.TakeIsolateConfiguration()) // diff --git a/shell/common/shell_unittests.cc b/shell/common/shell_unittests.cc index d4837731b6691..e15182e8d264c 100644 --- a/shell/common/shell_unittests.cc +++ b/shell/common/shell_unittests.cc @@ -2185,5 +2185,28 @@ TEST_F(ShellTest, OnServiceProtocolSetAssetBundlePathWorks) { DestroyShell(std::move(shell)); } +TEST_F(ShellTest, EngineRootIsolateLaunchesDontTakeVMDataSettings) { + ASSERT_FALSE(DartVMRef::IsInstanceRunning()); + // Make sure the shell launch does not kick off the creation of the VM + // instance by already creating one upfront. + auto vm_settings = CreateSettingsForFixture(); + auto vm_ref = DartVMRef::Create(vm_settings); + ASSERT_TRUE(DartVMRef::IsInstanceRunning()); + + auto settings = vm_settings; + fml::AutoResetWaitableEvent isolate_create_latch; + settings.root_isolate_create_callback = [&](const auto& isolate) { + isolate_create_latch.Signal(); + }; + auto shell = CreateShell(settings); + ASSERT_TRUE(ValidateShell(shell.get())); + auto configuration = RunConfiguration::InferFromSettings(settings); + ASSERT_TRUE(configuration.IsValid()); + RunEngine(shell.get(), std::move(configuration)); + ASSERT_TRUE(DartVMRef::IsInstanceRunning()); + DestroyShell(std::move(shell)); + isolate_create_latch.Wait(); +} + } // namespace testing } // namespace flutter