From 6d208877f4e6be1c9735812b61fa0a3987c5aac6 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Thu, 21 Nov 2024 10:13:32 -0800 Subject: [PATCH 1/4] [engine] more consistently flush dart event loop, run vsync callback immediately. --- fml/task_runner.cc | 17 +++++++++++++++++ fml/task_runner.h | 8 ++++++++ shell/common/shell.cc | 32 +++++++++++++++++--------------- shell/common/vsync_waiter.cc | 5 ++++- 4 files changed, 46 insertions(+), 16 deletions(-) diff --git a/fml/task_runner.cc b/fml/task_runner.cc index 5f9dd019c685e..8920a8f73ab25 100644 --- a/fml/task_runner.cc +++ b/fml/task_runner.cc @@ -52,6 +52,7 @@ bool TaskRunner::RunsTasksOnCurrentThread() { loop_queue_id); } +// static void TaskRunner::RunNowOrPostTask(const fml::RefPtr& runner, const fml::closure& task) { FML_DCHECK(runner); @@ -62,4 +63,20 @@ void TaskRunner::RunNowOrPostTask(const fml::RefPtr& runner, } } +// static +void TaskRunner::RunNowAndFlushMessages( + const fml::RefPtr& runner, + const fml::closure& task) { + FML_DCHECK(runner); + if (runner->RunsTasksOnCurrentThread()) { + task(); + // Post an empty task to make the UI message loop run its task observers. + // The observers will execute any Dart microtasks queued by the platform + // message handler. + runner->PostTask([] {}); + } else { + runner->PostTask(task); + } +} + } // namespace fml diff --git a/fml/task_runner.h b/fml/task_runner.h index 885c1b3efb566..6bc1c1a06cd8a 100644 --- a/fml/task_runner.h +++ b/fml/task_runner.h @@ -62,6 +62,14 @@ class TaskRunner : public fml::RefCountedThreadSafe, static void RunNowOrPostTask(const fml::RefPtr& runner, const fml::closure& task); + /// Like RunNowOrPostTask, except that if the task can be immediately + /// executed, an empty task will still be posted to the runner afterwards. + /// + /// This is used to ensure that messages posted to Dart from the platform + /// thread always flush the Dart event loop. + static void RunNowAndFlushMessages(const fml::RefPtr& runner, + const fml::closure& task); + protected: explicit TaskRunner(fml::RefPtr loop); diff --git a/shell/common/shell.cc b/shell/common/shell.cc index c6078f78c7de6..34b0f07f91d4e 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -1036,7 +1036,7 @@ void Shell::OnPlatformViewSetViewportMetrics(int64_t view_id, } }); - fml::TaskRunner::RunNowOrPostTask( + fml::TaskRunner::RunNowAndFlushMessages( task_runners_.GetUITaskRunner(), [engine = engine_->GetWeakPtr(), view_id, metrics]() { if (engine) { @@ -1104,7 +1104,7 @@ void Shell::OnPlatformViewDispatchPointerDataPacket( TRACE_FLOW_BEGIN("flutter", "PointerEvent", next_pointer_flow_id_); FML_DCHECK(is_set_up_); FML_DCHECK(task_runners_.GetPlatformTaskRunner()->RunsTasksOnCurrentThread()); - fml::TaskRunner::RunNowOrPostTask( + fml::TaskRunner::RunNowAndFlushMessages( task_runners_.GetUITaskRunner(), fml::MakeCopyable([engine = weak_engine_, packet = std::move(packet), flow_id = next_pointer_flow_id_]() mutable { @@ -1122,7 +1122,8 @@ void Shell::OnPlatformViewDispatchSemanticsAction(int32_t node_id, FML_DCHECK(is_set_up_); FML_DCHECK(task_runners_.GetPlatformTaskRunner()->RunsTasksOnCurrentThread()); - task_runners_.GetUITaskRunner()->PostTask( + fml::TaskRunner::RunNowAndFlushMessages( + task_runners_.GetUITaskRunner(), fml::MakeCopyable([engine = engine_->GetWeakPtr(), node_id, action, args = std::move(args)]() mutable { if (engine) { @@ -1136,12 +1137,13 @@ void Shell::OnPlatformViewSetSemanticsEnabled(bool enabled) { FML_DCHECK(is_set_up_); FML_DCHECK(task_runners_.GetPlatformTaskRunner()->RunsTasksOnCurrentThread()); - fml::TaskRunner::RunNowOrPostTask(task_runners_.GetUITaskRunner(), - [engine = engine_->GetWeakPtr(), enabled] { - if (engine) { - engine->SetSemanticsEnabled(enabled); - } - }); + fml::TaskRunner::RunNowAndFlushMessages( + task_runners_.GetUITaskRunner(), + [engine = engine_->GetWeakPtr(), enabled] { + if (engine) { + engine->SetSemanticsEnabled(enabled); + } + }); } // |PlatformView::Delegate| @@ -1149,12 +1151,12 @@ void Shell::OnPlatformViewSetAccessibilityFeatures(int32_t flags) { FML_DCHECK(is_set_up_); FML_DCHECK(task_runners_.GetPlatformTaskRunner()->RunsTasksOnCurrentThread()); - fml::TaskRunner::RunNowOrPostTask(task_runners_.GetUITaskRunner(), - [engine = engine_->GetWeakPtr(), flags] { - if (engine) { - engine->SetAccessibilityFeatures(flags); - } - }); + fml::TaskRunner::RunNowAndFlushMessages( + task_runners_.GetUITaskRunner(), [engine = engine_->GetWeakPtr(), flags] { + if (engine) { + engine->SetAccessibilityFeatures(flags); + } + }); } // |PlatformView::Delegate| diff --git a/shell/common/vsync_waiter.cc b/shell/common/vsync_waiter.cc index 80074fa3ef982..7457fd397392f 100644 --- a/shell/common/vsync_waiter.cc +++ b/shell/common/vsync_waiter.cc @@ -128,7 +128,10 @@ void VsyncWaiter::FireCallback(fml::TimePoint frame_start_time, fml::TaskQueueId ui_task_queue_id = task_runners_.GetUITaskRunner()->GetTaskQueueId(); - task_runners_.GetUITaskRunner()->PostTask( + // This task does not need to use RunNowAndFlushMessages as the + // vsync callback will explicitly flush the Dart event loop. + fml::TaskRunner::RunNowOrPostTask( + task_runners_.GetUITaskRunner(), [ui_task_queue_id, callback, flow_identifier, frame_start_time, frame_target_time, pause_secondary_tasks]() { FML_TRACE_EVENT_WITH_FLOW_IDS( From c02ac53fadd1cf081638cb8e2010322744b883f4 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Thu, 21 Nov 2024 10:16:00 -0800 Subject: [PATCH 2/4] also update OnPlatformViewDispatchPlatformMessage --- shell/common/shell.cc | 29 +++++++++++------------------ 1 file changed, 11 insertions(+), 18 deletions(-) diff --git a/shell/common/shell.cc b/shell/common/shell.cc index 34b0f07f91d4e..ba28d8e950514 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -2,6 +2,7 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +#include "fml/task_runner.h" #define RAPIDJSON_HAS_STDSTRING 1 #include "flutter/shell/common/shell.h" @@ -1075,24 +1076,16 @@ void Shell::OnPlatformViewDispatchPlatformMessage( } #endif // FLUTTER_RUNTIME_MODE == FLUTTER_RUNTIME_MODE_DEBUG - if (task_runners_.GetUITaskRunner()->RunsTasksOnCurrentThread()) { - engine_->DispatchPlatformMessage(std::move(message)); - - // Post an empty task to make the UI message loop run its task observers. - // The observers will execute any Dart microtasks queued by the platform - // message handler. - task_runners_.GetUITaskRunner()->PostTask([] {}); - } else { - // The static leak checker gets confused by the use of fml::MakeCopyable. - // NOLINTNEXTLINE(clang-analyzer-cplusplus.NewDeleteLeaks) - task_runners_.GetUITaskRunner()->PostTask( - fml::MakeCopyable([engine = engine_->GetWeakPtr(), - message = std::move(message)]() mutable { - if (engine) { - engine->DispatchPlatformMessage(std::move(message)); - } - })); - } + // The static leak checker gets confused by the use of fml::MakeCopyable. + // NOLINTNEXTLINE(clang-analyzer-cplusplus.NewDeleteLeaks) + fml::TaskRunner::RunNowAndFlushMessages( + task_runners_.GetUITaskRunner(), + fml::MakeCopyable([engine = engine_->GetWeakPtr(), + message = std::move(message)]() mutable { + if (engine) { + engine->DispatchPlatformMessage(std::move(message)); + } + })); } // |PlatformView::Delegate| From ac0ac639a0a2d6d504eb8b019f8404d66bd784e5 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Thu, 21 Nov 2024 14:33:42 -0800 Subject: [PATCH 3/4] add pointer event test. --- shell/common/fixtures/shell_test.dart | 10 ++++++++ shell/common/shell_unittests.cc | 36 +++++++++++++++++++++++++-- 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/shell/common/fixtures/shell_test.dart b/shell/common/fixtures/shell_test.dart index 60727567e5737..24dc227aa8f40 100644 --- a/shell/common/fixtures/shell_test.dart +++ b/shell/common/fixtures/shell_test.dart @@ -640,3 +640,13 @@ void testSemanticsActions() { }); }; } + +@pragma('vm:entry-point') +void testPointerActions() { + PlatformDispatcher.instance.onPointerDataPacket = (PointerDataPacket pointer) async { + await null; + Future.value().then((_) { + notifyNative(); + }); + }; +} diff --git a/shell/common/shell_unittests.cc b/shell/common/shell_unittests.cc index 3621646601a36..aa5f1f165bd30 100644 --- a/shell/common/shell_unittests.cc +++ b/shell/common/shell_unittests.cc @@ -4312,7 +4312,8 @@ TEST_F(ShellTest, NavigationMessageDispachedImmediately) { ASSERT_FALSE(DartVMRef::IsInstanceRunning()); } -TEST_F(ShellTest, SemanticsActionsPostTask) { +// Verifies a semantics Action will flush the dart event loop. +TEST_F(ShellTest, SemanticsActionsFlushMessageLoop) { Settings settings = CreateSettingsForFixture(); ThreadHost thread_host("io.flutter.test." + GetCurrentTestName() + ".", ThreadHost::Type::kPlatform); @@ -4333,7 +4334,38 @@ TEST_F(ShellTest, SemanticsActionsPostTask) { fml::MallocMapping(nullptr, 0)); }); - // Fulfill native function for the second Shell's entrypoint. + fml::CountDownLatch latch(1); + AddNativeCallback( + // The Dart native function names aren't very consistent but this is + // just the native function name of the second vm entrypoint in the + // fixture. + "NotifyNative", + CREATE_NATIVE_ENTRY([&](auto args) { latch.CountDown(); })); + latch.Wait(); + + DestroyShell(std::move(shell), task_runners); + ASSERT_FALSE(DartVMRef::IsInstanceRunning()); +} + +// Verifies a pointer event will flush the dart event loop. +TEST_F(ShellTest, PointerPacketFlushMessageLoop) { + Settings settings = CreateSettingsForFixture(); + ThreadHost thread_host("io.flutter.test." + GetCurrentTestName() + ".", + ThreadHost::Type::kPlatform); + auto task_runner = thread_host.platform_thread->GetTaskRunner(); + TaskRunners task_runners("test", task_runner, task_runner, task_runner, + task_runner); + + EXPECT_EQ(task_runners.GetPlatformTaskRunner(), + task_runners.GetUITaskRunner()); + auto shell = CreateShell(settings, task_runners); + auto configuration = RunConfiguration::InferFromSettings(settings); + configuration.SetEntrypoint("testPointerActions"); + + RunEngine(shell.get(), std::move(configuration)); + + DispatchFakePointerData(shell.get(), 23); + fml::CountDownLatch latch(1); AddNativeCallback( // The Dart native function names aren't very consistent but this is From 14e6d2f2d90330e8d5b4282e89e0aafaf2485ac5 Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Thu, 21 Nov 2024 22:46:56 -0800 Subject: [PATCH 4/4] Update shell_unittests.cc --- shell/common/shell_unittests.cc | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/shell/common/shell_unittests.cc b/shell/common/shell_unittests.cc index aa5f1f165bd30..adadd0971409c 100644 --- a/shell/common/shell_unittests.cc +++ b/shell/common/shell_unittests.cc @@ -4328,12 +4328,6 @@ TEST_F(ShellTest, SemanticsActionsFlushMessageLoop) { configuration.SetEntrypoint("testSemanticsActions"); RunEngine(shell.get(), std::move(configuration)); - - task_runners.GetPlatformTaskRunner()->PostTask([&] { - SendSemanticsAction(shell.get(), 0, SemanticsAction::kTap, - fml::MallocMapping(nullptr, 0)); - }); - fml::CountDownLatch latch(1); AddNativeCallback( // The Dart native function names aren't very consistent but this is @@ -4341,6 +4335,11 @@ TEST_F(ShellTest, SemanticsActionsFlushMessageLoop) { // fixture. "NotifyNative", CREATE_NATIVE_ENTRY([&](auto args) { latch.CountDown(); })); + + task_runners.GetPlatformTaskRunner()->PostTask([&] { + SendSemanticsAction(shell.get(), 0, SemanticsAction::kTap, + fml::MallocMapping(nullptr, 0)); + }); latch.Wait(); DestroyShell(std::move(shell), task_runners); @@ -4363,9 +4362,6 @@ TEST_F(ShellTest, PointerPacketFlushMessageLoop) { configuration.SetEntrypoint("testPointerActions"); RunEngine(shell.get(), std::move(configuration)); - - DispatchFakePointerData(shell.get(), 23); - fml::CountDownLatch latch(1); AddNativeCallback( // The Dart native function names aren't very consistent but this is @@ -4373,6 +4369,8 @@ TEST_F(ShellTest, PointerPacketFlushMessageLoop) { // fixture. "NotifyNative", CREATE_NATIVE_ENTRY([&](auto args) { latch.CountDown(); })); + + DispatchFakePointerData(shell.get(), 23); latch.Wait(); DestroyShell(std::move(shell), task_runners);