Isolates launched by the engine instance use the settings of that instance. (#22052)

This regression was introduced in #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 #22041

Author: chinmaygarde

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

runtime/runtime_controller.cc

@@ -342,6 +342,7 @@ tonic::DartErrorHandleType RuntimeController::GetLastError() {
 }
 
 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 @@ bool RuntimeController::LaunchRootIsolate(
 
   auto strong_root_isolate =
       DartIsolate::CreateRunningRootIsolate(
-          vm_->GetVMData()->GetSettings(),                //
+          settings,                                       //
           isolate_snapshot_,                              //
           task_runners_,                                  //
           std::make_unique<PlatformConfiguration>(this),  //

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<std::string> dart_entrypoint,
       std::optional<std::string> dart_entrypoint_library,
       std::unique_ptr<IsolateConfiguration> isolate_configuration);

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())  //

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