diff --git a/runtime/dart_isolate.cc b/runtime/dart_isolate.cc index e2221057d2b90..d60404d9a7e38 100644 --- a/runtime/dart_isolate.cc +++ b/runtime/dart_isolate.cc @@ -7,6 +7,7 @@ #include #include +#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 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 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* isolate_group_data, std::shared_ptr* 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(); } diff --git a/runtime/dart_isolate.h b/runtime/dart_isolate.h index a7c39113408ca..3fe9cf2f81dc4 100644 --- a/runtime/dart_isolate.h +++ b/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 runner); diff --git a/runtime/dart_isolate_unittests.cc b/runtime/dart_isolate_unittests.cc index 53b7749209a8a..f1fe951696854 100644 --- a/runtime/dart_isolate_unittests.cc +++ b/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) { diff --git a/runtime/fixtures/runtime_test.dart b/runtime/fixtures/runtime_test.dart index 6c769d0173591..7dd46823f53eb 100644 --- a/runtime/fixtures/runtime_test.dart +++ b/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(onMessage); + final exits = StreamIterator(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 args) { notifyResult(args.length == 1 && args[0] == 'arg1');