Isolates launched by the engine instance use the settings of that instance. (#22052)
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
diff --git a/runtime/dart_isolate_unittests.cc b/runtime/dart_isolate_unittests.cc
index f6263dd..a90599f 100644
--- a/runtime/dart_isolate_unittests.cc
+++ b/runtime/dart_isolate_unittests.cc
@@ -402,5 +402,33 @@
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 a2ea4a0..83d1292 100644
--- a/runtime/runtime_controller.cc
+++ b/runtime/runtime_controller.cc
@@ -342,6 +342,7 @@
}
bool RuntimeController::LaunchRootIsolate(
+ const Settings& settings,
std::optional<std::string> dart_entrypoint,
std::optional<std::string> dart_entrypoint_library,
std::unique_ptr<IsolateConfiguration> isolate_configuration) {
@@ -352,7 +353,7 @@
auto strong_root_isolate =
DartIsolate::CreateRunningRootIsolate(
- vm_->GetVMData()->GetSettings(), //
+ settings, //
isolate_snapshot_, //
task_runners_, //
std::make_unique<PlatformConfiguration>(this), //
diff --git a/runtime/runtime_controller.h b/runtime/runtime_controller.h
index 9b5ef99..17b02b9 100644
--- a/runtime/runtime_controller.h
+++ b/runtime/runtime_controller.h
@@ -144,6 +144,7 @@
/// 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 @@
/// `DartIsolate::Phase::Running` phase.
///
[[nodiscard]] bool LaunchRootIsolate(
+ const Settings& settings,
std::optional<std::string> dart_entrypoint,
std::optional<std::string> dart_entrypoint_library,
std::unique_ptr<IsolateConfiguration> isolate_configuration);
diff --git a/shell/common/engine.cc b/shell/common/engine.cc
index f3374a0..6c9304f 100644
--- a/shell/common/engine.cc
+++ b/shell/common/engine.cc
@@ -160,6 +160,7 @@
}
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 d483773..e15182e 100644
--- a/shell/common/shell_unittests.cc
+++ b/shell/common/shell_unittests.cc
@@ -2185,5 +2185,28 @@
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
