Fix incorrect handling of error handling in case an isolate initialization fails (#31207)

For isolates spawned by the application via `Isolate.spawn()`ed, the VM
will create a "lightweight" isolate and invoke the `initialize_isolate`
embedder callback to initialize it.

The embedder-provided callback will be invoked with the active isolate
and is expected to return with that active isolate - irrespective of
whether it succeeded to initialize or not.
=> The unsuccessful path was using `Dart_ExitIsolate()` - which is
   incorrect.

This PR fixes that by not exiting the isolate. As a side-effect of the
fix, we also do less `Dart_EnterIsolate()`/`Dart_ExitIsolate()` calls in
initialization (which makes it faster) and handle failure to spawn the
root isolate. Furthermore this PR removes some dead code and replaces it
with `FML_DCHECK()`s instead.

The PR adds a test that will set the root library to null which will make the
engine fail initializing of the isolate and therefore trigger this error handling
path.

Fixes flutter/flutter#90478

Author: mkustermann

runtime/dart_isolate.cc

@@ -7,6 +7,7 @@
 #include <cstdlib>
 #include <tuple>
 
+#include "flutter/fml/logging.h"
 #include "flutter/fml/posix_wrappers.h"
 #include "flutter/fml/trace_event.h"
 #include "flutter/lib/io/dart_io.h"
@@ -314,25 +315,12 @@ bool DartIsolate::Initialize(Dart_Isolate dart_isolate) {
     return false;
   }
 
-  if (dart_isolate == nullptr) {
-    return false;
-  }
-
-  if (Dart_CurrentIsolate() != dart_isolate) {
-    return false;
-  }
+  FML_DCHECK(dart_isolate != nullptr);
+  FML_DCHECK(dart_isolate == Dart_CurrentIsolate());
 
   // After this point, isolate scopes can be safely used.
   SetIsolate(dart_isolate);
 
-  // We are entering a new scope (for the first time since initialization) and
-  // we want to restore the current scope to null when we exit out of this
-  // method. This balances the implicit Dart_EnterIsolate call made by
-  // Dart_CreateIsolateGroup (which calls the Initialize).
-  Dart_ExitIsolate();
-
-  tonic::DartIsolateScope scope(isolate());
-
   // For the root isolate set the "AppStartUp" as soon as the root isolate
   // has been initialized. This is to ensure that all the timeline events
   // that have the set user-tag will be listed user AppStartUp.
@@ -997,7 +985,6 @@ bool DartIsolate::DartIsolateInitializeCallback(void** child_callback_data,
   // only reference returned to the caller is weak.
   *child_callback_data = embedder_isolate.release();
 
-  Dart_EnterIsolate(isolate);
   return true;
 }
 
@@ -1017,15 +1004,23 @@ Dart_Isolate DartIsolate::CreateDartIsolateGroup(
     return nullptr;
   }
 
-  // Ownership of the isolate data objects has been transferred to the Dart VM.
-  std::shared_ptr<DartIsolate> embedder_isolate(*isolate_data);
-  isolate_group_data.release();
-  isolate_data.release();
+  bool success = false;
+  {
+    // Ownership of the isolate data objects has been transferred to the Dart
+    // VM.
+    std::shared_ptr<DartIsolate> embedder_isolate(*isolate_data);
+    isolate_group_data.release();
+    isolate_data.release();
 
-  if (!InitializeIsolate(std::move(embedder_isolate), isolate, error)) {
+    success = InitializeIsolate(std::move(embedder_isolate), isolate, error);
+  }
+  if (!success) {
+    Dart_ShutdownIsolate();
     return nullptr;
   }
 
+  // Balances the implicit [Dart_EnterIsolate] by [make_isolate] above.
+  Dart_ExitIsolate();
   return isolate;
 }
 
@@ -1070,6 +1065,14 @@ void DartIsolate::DartIsolateShutdownCallback(
     std::shared_ptr<DartIsolateGroupData>* isolate_group_data,
     std::shared_ptr<DartIsolate>* isolate_data) {
   TRACE_EVENT0("flutter", "DartIsolate::DartIsolateShutdownCallback");
+
+  // If the isolate initialization failed there will be nothing to do.
+  // This can happen e.g. during a [DartIsolateInitializeCallback] invocation
+  // that fails to initialize the VM-created isolate.
+  if (isolate_data == nullptr) {
+    return;
+  }
+
   isolate_data->get()->OnShutdownCallback();
 }
 

runtime/dart_isolate.h

@@ -421,7 +421,16 @@ class DartIsolate : public UIDartState {
               bool is_root_isolate,
               const UIDartState::Context& context);
 
-  [[nodiscard]] bool Initialize(Dart_Isolate isolate);
+  //----------------------------------------------------------------------------
+  /// @brief      Initializes the given (current) isolate.
+  ///
+  /// @param[in]  dart_isolate  The current isolate that is to be initialized.
+  ///
+  /// @return     Whether the initialization succeeded. Irrespective of whether
+  ///             the initialization suceeded, the current isolate will still be
+  ///             active.
+  ///
+  [[nodiscard]] bool Initialize(Dart_Isolate dart_isolate);
 
   void SetMessageHandlingTaskRunner(fml::RefPtr<fml::TaskRunner> runner);
 

runtime/dart_isolate_unittests.cc

@@ -13,6 +13,7 @@
 #include "flutter/testing/dart_isolate_runner.h"
 #include "flutter/testing/fixture_test.h"
 #include "flutter/testing/testing.h"
+#include "third_party/dart/runtime/include/dart_api.h"
 #include "third_party/tonic/converter/dart_converter.h"
 #include "third_party/tonic/scopes/dart_isolate_scope.h"
 
@@ -258,6 +259,44 @@ TEST_F(DartSecondaryIsolateTest, CanLaunchSecondaryIsolates) {
   // root isolate will be auto-shutdown
 }
 
+/// Tests error handling path of `Isolate.spawn()` in the engine.
+class IsolateStartupFailureTest : public FixtureTest {
+ public:
+  IsolateStartupFailureTest() : latch_(1) {}
+  void NotifyDone() { latch_.CountDown(); }
+  void WaitForDone() { latch_.Wait(); }
+
+ private:
+  fml::CountDownLatch latch_;
+  FML_DISALLOW_COPY_AND_ASSIGN(IsolateStartupFailureTest);
+};
+
+TEST_F(IsolateStartupFailureTest,
+       HandlesIsolateInitializationFailureCorrectly) {
+  AddNativeCallback("MakeNextIsolateSpawnFail",
+                    CREATE_NATIVE_ENTRY(([](Dart_NativeArguments args) {
+                      Dart_SetRootLibrary(Dart_Null());
+                    })));
+  AddNativeCallback("NotifyNative",
+                    CREATE_NATIVE_ENTRY(
+                        ([this](Dart_NativeArguments args) { NotifyDone(); })));
+  auto settings = CreateSettingsForFixture();
+  auto vm_ref = DartVMRef::Create(settings);
+  auto thread = CreateNewThread();
+  TaskRunners task_runners(GetCurrentTestName(),  //
+                           thread,                //
+                           thread,                //
+                           thread,                //
+                           thread                 //
+  );
+  auto isolate = RunDartCodeInIsolate(vm_ref, settings, task_runners,
+                                      "testIsolateStartupFailure", {},
+                                      GetDefaultKernelFilePath());
+  ASSERT_TRUE(isolate);
+  ASSERT_EQ(isolate->get()->GetPhase(), DartIsolate::Phase::Running);
+  WaitForDone();
+}
+
 TEST_F(DartIsolateTest, CanReceiveArguments) {
   AddNativeCallback("NotifyNative",
                     CREATE_NATIVE_ENTRY(([this](Dart_NativeArguments args) {

runtime/fixtures/runtime_test.dart

@@ -64,6 +64,56 @@ void testCanLaunchSecondaryIsolate() {
   Isolate.spawn(secondaryIsolateMain, 'Hello from root isolate.', onExit: onExit.sendPort);
 }
 
+
+@pragma('vm:entry-point')
+void testIsolateStartupFailure() async {
+  Future mainTest(dynamic _) async {
+    Future testSuccessfullIsolateLaunch() async {
+      final onMessage = ReceivePort();
+      final onExit = ReceivePort();
+
+      final messages = StreamIterator<dynamic>(onMessage);
+      final exits = StreamIterator<dynamic>(onExit);
+
+      await Isolate.spawn((SendPort port) => port.send('good'),
+          onMessage.sendPort, onExit: onExit.sendPort);
+      if (!await messages.moveNext()) {
+        throw 'Failed to receive message';
+      }
+      if (messages.current != 'good') {
+        throw 'Failed to receive correct message';
+      }
+      if (!await exits.moveNext()) {
+        throw 'Failed to receive onExit';
+      }
+      messages.cancel();
+      exits.cancel();
+    }
+
+    Future testUnsuccessfullIsolateLaunch() async {
+      IsolateSpawnException? error;
+      try {
+        await Isolate.spawn((_) {}, null);
+      } on IsolateSpawnException catch (e) {
+        error = e;
+      }
+      if (error == null) {
+        throw 'Expected isolate spawn to fail.';
+      }
+    }
+
+    await testSuccessfullIsolateLaunch();
+    makeNextIsolateSpawnFail();
+    await testUnsuccessfullIsolateLaunch();
+    notifyNative();
+  }
+
+  // The root isolate will not run an eventloop, so we have to run the actual
+  // test in an isolate.
+  Isolate.spawn(mainTest, null);
+}
+void makeNextIsolateSpawnFail() native 'MakeNextIsolateSpawnFail';
+
 @pragma('vm:entry-point')
 void testCanReceiveArguments(List<String> args) {
   notifyResult(args.length == 1 && args[0] == 'arg1');