From 9b1edbc98d33a9617a9c0aecfaeea2a3d5c4586d Mon Sep 17 00:00:00 2001 From: Emmanuel Garcia Date: Wed, 20 Jan 2021 14:11:31 -0800 Subject: [PATCH 1/6] Call Dart plugin registrant if available --- lib/ui/hooks.dart | 4 ++++ runtime/dart_isolate.cc | 38 ++++++++++++++++++++++++++++++++++---- 2 files changed, 38 insertions(+), 4 deletions(-) diff --git a/lib/ui/hooks.dart b/lib/ui/hooks.dart index 6ffcea0f93d72..273f44edaa0e6 100644 --- a/lib/ui/hooks.dart +++ b/lib/ui/hooks.dart @@ -126,10 +126,14 @@ typedef _ListStringArgFunction(List args); @pragma('vm:entry-point') // ignore: unused_element void _runMainZoned(Function startMainIsolateFunction, + Function? dartPluginRegistrant, Function userMainFunction, List args) { startMainIsolateFunction(() { runZonedGuarded(() { + if (dartPluginRegistrant != null) { + dartPluginRegistrant(); + } if (userMainFunction is _ListStringArgFunction) { (userMainFunction as dynamic)(args); } else { diff --git a/runtime/dart_isolate.cc b/runtime/dart_isolate.cc index 9cb2fce5e91a6..644d3ac9ab4b7 100644 --- a/runtime/dart_isolate.cc +++ b/runtime/dart_isolate.cc @@ -674,6 +674,7 @@ bool DartIsolate::MarkIsolateRunnable() { [[nodiscard]] static bool InvokeMainEntrypoint( Dart_Handle user_entrypoint_function, + Dart_Handle plugin_registrant_function, Dart_Handle args) { if (tonic::LogIfError(user_entrypoint_function)) { FML_LOG(ERROR) << "Could not resolve main entrypoint function."; @@ -689,9 +690,36 @@ bool DartIsolate::MarkIsolateRunnable() { return false; } + if (tonic::GetErrorHandleType(plugin_registrant_function) == + tonic::DartErrorHandleType::kNoError) { + // The Dart plugin registrant is a function named `_registerPlugins` + // generated by the Flutter tool. + // + // This function binds a plugin implementation to their platform + // interface based on the configuration of the app's pubpec.yaml, and the + // plugin's pubspec.yaml. + // + // Since this function may or may not be defined. Check that is + // as a top level function, and call it in hooks.dart before the main + // entrypoint function. + // + // If it's not defined, then just call the main entrypoint function + // as usual. + // + // This allows embeddings to change the name of the entrypoint function. + if (tonic::LogIfError(tonic::DartInvokeField( + Dart_LookupLibrary(tonic::ToDart("dart:ui")), "_runMainZoned", + {start_main_isolate_function, plugin_registrant_function, + user_entrypoint_function, args}))) { + FML_LOG(ERROR) << "Could not invoke the main entrypoint."; + return false; + } + } + if (tonic::LogIfError(tonic::DartInvokeField( Dart_LookupLibrary(tonic::ToDart("dart:ui")), "_runMainZoned", - {start_main_isolate_function, user_entrypoint_function, args}))) { + {start_main_isolate_function, Dart_Null(), user_entrypoint_function, + args}))) { FML_LOG(ERROR) << "Could not invoke the main entrypoint."; return false; } @@ -716,12 +744,14 @@ bool DartIsolate::RunFromLibrary(std::optional library_name, auto entrypoint_handle = entrypoint.has_value() && !entrypoint.value().empty() ? tonic::ToDart(entrypoint.value().c_str()) : tonic::ToDart("main"); + auto entrypoint_args = tonic::ToDart(args); auto user_entrypoint_function = ::Dart_GetField(library_handle, entrypoint_handle); + auto plugin_registrant_function = + ::Dart_GetField(library_handle, tonic::ToDart("_registerPlugins")); - auto entrypoint_args = tonic::ToDart(args); - - if (!InvokeMainEntrypoint(user_entrypoint_function, entrypoint_args)) { + if (!InvokeMainEntrypoint(user_entrypoint_function, + plugin_registrant_function, entrypoint_args)) { return false; } From bfa0e47da50bc9badc8ad41c8b0a0b18e2f412e1 Mon Sep 17 00:00:00 2001 From: Emmanuel Garcia Date: Wed, 20 Jan 2021 17:48:15 -0800 Subject: [PATCH 2/6] Test --- runtime/dart_isolate.cc | 46 +++++++++++++----------------- runtime/dart_isolate_unittests.cc | 38 +++++++++++++++++++++++- runtime/fixtures/runtime_test.dart | 15 ++++++++-- 3 files changed, 70 insertions(+), 29 deletions(-) diff --git a/runtime/dart_isolate.cc b/runtime/dart_isolate.cc index 644d3ac9ab4b7..46aa33f1c67b6 100644 --- a/runtime/dart_isolate.cc +++ b/runtime/dart_isolate.cc @@ -690,36 +690,14 @@ bool DartIsolate::MarkIsolateRunnable() { return false; } - if (tonic::GetErrorHandleType(plugin_registrant_function) == - tonic::DartErrorHandleType::kNoError) { - // The Dart plugin registrant is a function named `_registerPlugins` - // generated by the Flutter tool. - // - // This function binds a plugin implementation to their platform - // interface based on the configuration of the app's pubpec.yaml, and the - // plugin's pubspec.yaml. - // - // Since this function may or may not be defined. Check that is - // as a top level function, and call it in hooks.dart before the main - // entrypoint function. - // - // If it's not defined, then just call the main entrypoint function - // as usual. - // - // This allows embeddings to change the name of the entrypoint function. - if (tonic::LogIfError(tonic::DartInvokeField( - Dart_LookupLibrary(tonic::ToDart("dart:ui")), "_runMainZoned", - {start_main_isolate_function, plugin_registrant_function, - user_entrypoint_function, args}))) { - FML_LOG(ERROR) << "Could not invoke the main entrypoint."; - return false; - } + if (Dart_IsError(plugin_registrant_function)) { + plugin_registrant_function = Dart_Null(); } if (tonic::LogIfError(tonic::DartInvokeField( Dart_LookupLibrary(tonic::ToDart("dart:ui")), "_runMainZoned", - {start_main_isolate_function, Dart_Null(), user_entrypoint_function, - args}))) { + {start_main_isolate_function, plugin_registrant_function, + user_entrypoint_function, args}))) { FML_LOG(ERROR) << "Could not invoke the main entrypoint."; return false; } @@ -747,6 +725,22 @@ bool DartIsolate::RunFromLibrary(std::optional library_name, auto entrypoint_args = tonic::ToDart(args); auto user_entrypoint_function = ::Dart_GetField(library_handle, entrypoint_handle); + + // The Dart plugin registrant is a function named `_registerPlugins` + // generated by the Flutter tool. + // + // This function binds a plugin implementation to their platform + // interface based on the configuration of the app's pubpec.yaml, and the + // plugin's pubspec.yaml. + // + // Since this function may or may not be defined. Check that is + // as a top level function, and call it in hooks.dart before the main + // entrypoint function. + // + // If it's not defined, then just call the main entrypoint function + // as usual. + // + // This allows embeddings to change the name of the entrypoint function. auto plugin_registrant_function = ::Dart_GetField(library_handle, tonic::ToDart("_registerPlugins")); diff --git a/runtime/dart_isolate_unittests.cc b/runtime/dart_isolate_unittests.cc index 3b25c02dec0ae..f51af8f1bbc69 100644 --- a/runtime/dart_isolate_unittests.cc +++ b/runtime/dart_isolate_unittests.cc @@ -330,7 +330,8 @@ TEST_F(DartSecondaryIsolateTest, CanLaunchSecondaryIsolates) { LatchCountDown(); }))); AddNativeCallback( - "PassMessage", CREATE_NATIVE_ENTRY(([this](Dart_NativeArguments args) { + "PassMessageForIsolateTest", + CREATE_NATIVE_ENTRY(([this](Dart_NativeArguments args) { auto message = tonic::DartConverter::FromDart( Dart_GetNativeArgument(args, 0)); ASSERT_EQ("Hello from code is secondary isolate.", message); @@ -597,5 +598,40 @@ TEST_F(DartIsolateTest, DISABLED_ValidLoadingUnitSucceeds) { Wait(); } +TEST_F(DartIsolateTest, DartPluginRegistrantIsCalled) { + ASSERT_FALSE(DartVMRef::IsInstanceRunning()); + + std::vector messages; + fml::CountDownLatch expected_messages(2); + + AddNativeCallback( + "PassMessageForDartRegistrantTest", + CREATE_NATIVE_ENTRY( + ([&expected_messages, &messages](Dart_NativeArguments args) { + auto message = tonic::DartConverter::FromDart( + Dart_GetNativeArgument(args, 0)); + messages.push_back(message); + expected_messages.CountDown(); + }))); + + const 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, "main", + {}, GetFixturesPath()); + ASSERT_TRUE(isolate); + ASSERT_EQ(isolate->get()->GetPhase(), DartIsolate::Phase::Running); + expected_messages.Wait(); + ASSERT_EQ(messages.size(), 2u); + ASSERT_EQ(messages[0], "_registerPlugins"); + ASSERT_EQ(messages[1], "main"); +} + } // namespace testing } // namespace flutter diff --git a/runtime/fixtures/runtime_test.dart b/runtime/fixtures/runtime_test.dart index 9e137e1fb6acb..2c293fd211c42 100644 --- a/runtime/fixtures/runtime_test.dart +++ b/runtime/fixtures/runtime_test.dart @@ -10,7 +10,17 @@ import 'dart:ui'; import 'split_lib_test.dart' deferred as splitlib; +// ignore: unused_element +void _registerPlugins() { + try { + passMessageForDartRegistrantTest('_registerPlugins'); + } catch(_) {} +} + void main() { + try { + passMessageForDartRegistrantTest('main'); + } catch(_) {} } @pragma('vm:entry-point') @@ -58,11 +68,12 @@ void testCanSaveCompilationTrace() { } void notifyResult(bool success) native 'NotifyNative'; -void passMessage(String message) native 'PassMessage'; +void passMessageForIsolateTest(String message) native 'PassMessageForIsolateTest'; +void passMessageForDartRegistrantTest(String message) native 'PassMessageForDartRegistrantTest'; void secondaryIsolateMain(String message) { print('Secondary isolate got message: ' + message); - passMessage('Hello from code is secondary isolate.'); + passMessageForIsolateTest('Hello from code is secondary isolate.'); notifyNative(); } From 8e916ead91b55b3073c0d7c1bfb6e0fd6260a179 Mon Sep 17 00:00:00 2001 From: Emmanuel Garcia Date: Wed, 20 Jan 2021 17:51:43 -0800 Subject: [PATCH 3/6] Comment --- runtime/dart_isolate.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/runtime/dart_isolate.cc b/runtime/dart_isolate.cc index 46aa33f1c67b6..014c6d929d80d 100644 --- a/runtime/dart_isolate.cc +++ b/runtime/dart_isolate.cc @@ -733,9 +733,9 @@ bool DartIsolate::RunFromLibrary(std::optional library_name, // interface based on the configuration of the app's pubpec.yaml, and the // plugin's pubspec.yaml. // - // Since this function may or may not be defined. Check that is - // as a top level function, and call it in hooks.dart before the main - // entrypoint function. + // Since this function may or may not be defined. Check that it is a top + // level function, and call it in hooks.dart before the main entrypoint + // function. // // If it's not defined, then just call the main entrypoint function // as usual. From 05633e82a29c37b94530a82fff2bd1270b4c3201 Mon Sep 17 00:00:00 2001 From: Emmanuel Garcia Date: Wed, 20 Jan 2021 20:33:00 -0800 Subject: [PATCH 4/6] feedback --- runtime/dart_isolate.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/runtime/dart_isolate.cc b/runtime/dart_isolate.cc index 014c6d929d80d..686e88f3a3534 100644 --- a/runtime/dart_isolate.cc +++ b/runtime/dart_isolate.cc @@ -690,10 +690,6 @@ bool DartIsolate::MarkIsolateRunnable() { return false; } - if (Dart_IsError(plugin_registrant_function)) { - plugin_registrant_function = Dart_Null(); - } - if (tonic::LogIfError(tonic::DartInvokeField( Dart_LookupLibrary(tonic::ToDart("dart:ui")), "_runMainZoned", {start_main_isolate_function, plugin_registrant_function, @@ -744,6 +740,10 @@ bool DartIsolate::RunFromLibrary(std::optional library_name, auto plugin_registrant_function = ::Dart_GetField(library_handle, tonic::ToDart("_registerPlugins")); + if (Dart_IsError(plugin_registrant_function)) { + plugin_registrant_function = Dart_Null(); + } + if (!InvokeMainEntrypoint(user_entrypoint_function, plugin_registrant_function, entrypoint_args)) { return false; From fb0502a2a25af9a37ab9a9f7f971f4469e147da4 Mon Sep 17 00:00:00 2001 From: Emmanuel Garcia Date: Thu, 21 Jan 2021 10:29:29 -0800 Subject: [PATCH 5/6] Debug test --- runtime/dart_isolate.cc | 3 +++ runtime/fixtures/runtime_test.dart | 8 ++++++-- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/runtime/dart_isolate.cc b/runtime/dart_isolate.cc index 686e88f3a3534..694dd8c666726 100644 --- a/runtime/dart_isolate.cc +++ b/runtime/dart_isolate.cc @@ -742,6 +742,9 @@ bool DartIsolate::RunFromLibrary(std::optional library_name, if (Dart_IsError(plugin_registrant_function)) { plugin_registrant_function = Dart_Null(); + FML_DLOG(ERROR) << " plugin_registrant_function IS NULL "; + } else { + FML_DLOG(ERROR) << " plugin_registrant_function IS NOT NULL "; } if (!InvokeMainEntrypoint(user_entrypoint_function, diff --git a/runtime/fixtures/runtime_test.dart b/runtime/fixtures/runtime_test.dart index 2c293fd211c42..7317d928038ff 100644 --- a/runtime/fixtures/runtime_test.dart +++ b/runtime/fixtures/runtime_test.dart @@ -10,16 +10,20 @@ import 'dart:ui'; import 'split_lib_test.dart' deferred as splitlib; -// ignore: unused_element -void _registerPlugins() { +@pragma('vm:entry-point') +void _registerPlugins() { // ignore: unused_element try { + print('_registerPlugins'); passMessageForDartRegistrantTest('_registerPlugins'); + print('_registerPlugins 2'); } catch(_) {} } void main() { try { + print('main'); passMessageForDartRegistrantTest('main'); + print('main 2'); } catch(_) {} } From 9b367452812241bdb97ea9ab1a4bfd6142faa5da Mon Sep 17 00:00:00 2001 From: Emmanuel Garcia Date: Thu, 21 Jan 2021 16:58:45 -0800 Subject: [PATCH 6/6] Simplify test --- runtime/dart_isolate.cc | 3 --- runtime/dart_isolate_unittests.cc | 32 +++++++++++----------- runtime/fixtures/runtime_test.dart | 43 +++++++++++++++++------------- 3 files changed, 39 insertions(+), 39 deletions(-) diff --git a/runtime/dart_isolate.cc b/runtime/dart_isolate.cc index 694dd8c666726..686e88f3a3534 100644 --- a/runtime/dart_isolate.cc +++ b/runtime/dart_isolate.cc @@ -742,9 +742,6 @@ bool DartIsolate::RunFromLibrary(std::optional library_name, if (Dart_IsError(plugin_registrant_function)) { plugin_registrant_function = Dart_Null(); - FML_DLOG(ERROR) << " plugin_registrant_function IS NULL "; - } else { - FML_DLOG(ERROR) << " plugin_registrant_function IS NOT NULL "; } if (!InvokeMainEntrypoint(user_entrypoint_function, diff --git a/runtime/dart_isolate_unittests.cc b/runtime/dart_isolate_unittests.cc index f51af8f1bbc69..98c52b4989d0a 100644 --- a/runtime/dart_isolate_unittests.cc +++ b/runtime/dart_isolate_unittests.cc @@ -330,8 +330,7 @@ TEST_F(DartSecondaryIsolateTest, CanLaunchSecondaryIsolates) { LatchCountDown(); }))); AddNativeCallback( - "PassMessageForIsolateTest", - CREATE_NATIVE_ENTRY(([this](Dart_NativeArguments args) { + "PassMessage", CREATE_NATIVE_ENTRY(([this](Dart_NativeArguments args) { auto message = tonic::DartConverter::FromDart( Dart_GetNativeArgument(args, 0)); ASSERT_EQ("Hello from code is secondary isolate.", message); @@ -602,17 +601,16 @@ TEST_F(DartIsolateTest, DartPluginRegistrantIsCalled) { ASSERT_FALSE(DartVMRef::IsInstanceRunning()); std::vector messages; - fml::CountDownLatch expected_messages(2); + fml::AutoResetWaitableEvent latch; AddNativeCallback( - "PassMessageForDartRegistrantTest", - CREATE_NATIVE_ENTRY( - ([&expected_messages, &messages](Dart_NativeArguments args) { - auto message = tonic::DartConverter::FromDart( - Dart_GetNativeArgument(args, 0)); - messages.push_back(message); - expected_messages.CountDown(); - }))); + "PassMessage", + CREATE_NATIVE_ENTRY(([&latch, &messages](Dart_NativeArguments args) { + auto message = tonic::DartConverter::FromDart( + Dart_GetNativeArgument(args, 0)); + messages.push_back(message); + latch.Signal(); + }))); const auto settings = CreateSettingsForFixture(); auto vm_ref = DartVMRef::Create(settings); @@ -623,14 +621,14 @@ TEST_F(DartIsolateTest, DartPluginRegistrantIsCalled) { thread, // thread // ); - auto isolate = RunDartCodeInIsolate(vm_ref, settings, task_runners, "main", - {}, GetFixturesPath()); + auto isolate = RunDartCodeInIsolate(vm_ref, settings, task_runners, + "mainForPluginRegistrantTest", {}, + GetFixturesPath()); ASSERT_TRUE(isolate); ASSERT_EQ(isolate->get()->GetPhase(), DartIsolate::Phase::Running); - expected_messages.Wait(); - ASSERT_EQ(messages.size(), 2u); - ASSERT_EQ(messages[0], "_registerPlugins"); - ASSERT_EQ(messages[1], "main"); + latch.Wait(); + ASSERT_EQ(messages.size(), 1u); + ASSERT_EQ(messages[0], "_registerPlugins was called"); } } // namespace testing diff --git a/runtime/fixtures/runtime_test.dart b/runtime/fixtures/runtime_test.dart index 7317d928038ff..9a0169fb0d1ed 100644 --- a/runtime/fixtures/runtime_test.dart +++ b/runtime/fixtures/runtime_test.dart @@ -10,22 +10,7 @@ import 'dart:ui'; import 'split_lib_test.dart' deferred as splitlib; -@pragma('vm:entry-point') -void _registerPlugins() { // ignore: unused_element - try { - print('_registerPlugins'); - passMessageForDartRegistrantTest('_registerPlugins'); - print('_registerPlugins 2'); - } catch(_) {} -} - -void main() { - try { - print('main'); - passMessageForDartRegistrantTest('main'); - print('main 2'); - } catch(_) {} -} +void main() {} @pragma('vm:entry-point') void sayHi() { @@ -72,12 +57,11 @@ void testCanSaveCompilationTrace() { } void notifyResult(bool success) native 'NotifyNative'; -void passMessageForIsolateTest(String message) native 'PassMessageForIsolateTest'; -void passMessageForDartRegistrantTest(String message) native 'PassMessageForDartRegistrantTest'; +void passMessage(String message) native 'PassMessage'; void secondaryIsolateMain(String message) { print('Secondary isolate got message: ' + message); - passMessageForIsolateTest('Hello from code is secondary isolate.'); + passMessage('Hello from code is secondary isolate.'); notifyNative(); } @@ -130,3 +114,24 @@ void testCanConvertListOfInts(List args){ args[2] == 3 && args[3] == 4); } + +bool didCallRegistrantBeforeEntrypoint = false; + +// Test the Dart plugin registrant. +// _registerPlugins requires the entrypoint annotation, so the compiler doesn't tree shake it. +@pragma('vm:entry-point') +void _registerPlugins() { // ignore: unused_element + if (didCallRegistrantBeforeEntrypoint) { + throw '_registerPlugins is being called twice'; + } + didCallRegistrantBeforeEntrypoint = true; +} + +@pragma('vm:entry-point') +void mainForPluginRegistrantTest() { // ignore: unused_element + if (didCallRegistrantBeforeEntrypoint) { + passMessage('_registerPlugins was called'); + } else { + passMessage('_registerPlugins was not called'); + } +}