From f3e0fce1f6a7c6d315db249f8ebd1fa618998633 Mon Sep 17 00:00:00 2001 From: "auto-submit[bot]" <98614782+auto-submit[bot]@users.noreply.github.com> Date: Fri, 22 Nov 2024 23:51:26 +0000 Subject: [PATCH] Reverts "[engine] more consistently flush dart event loop, run vsync callback immediately (#56738)" (#56767) Reverts: flutter/engine#56738 Initiated by: jonahwilliams Reason for reverting: speculative revert for framework failures. Original PR Author: jonahwilliams Reviewed By: {jason-simmons} This change reverts the following previous change: Changes the following shell callbacks to flush the dart event loop: * OnPlatformViewSetViewportMetrics * OnPlatformViewDispatchPointerDataPacket * OnPlatformViewDispatchPlatformMessage * OnPlatformViewSetSemanticsEnabled * OnPlatformViewSetAccessibilityFeatures Using a new TaskRunner API RunNowAndFlushMessages. If the task runner can run tasks on the current thread, this will immediately invoke a callback and then post an empty task to the event loop to ensure dart listeners fire. This also updates the vsync waiter to use RunNowOrPostTask, so that we start vsync events as early as possible. --- fml/task_runner.cc | 17 -------- fml/task_runner.h | 8 ---- shell/common/fixtures/shell_test.dart | 10 ----- shell/common/shell.cc | 61 +++++++++++++++------------ shell/common/shell_unittests.cc | 34 +-------------- shell/common/vsync_waiter.cc | 5 +-- 6 files changed, 36 insertions(+), 99 deletions(-) diff --git a/fml/task_runner.cc b/fml/task_runner.cc index 8920a8f73ab25..5f9dd019c685e 100644 --- a/fml/task_runner.cc +++ b/fml/task_runner.cc @@ -52,7 +52,6 @@ bool TaskRunner::RunsTasksOnCurrentThread() { loop_queue_id); } -// static void TaskRunner::RunNowOrPostTask(const fml::RefPtr& runner, const fml::closure& task) { FML_DCHECK(runner); @@ -63,20 +62,4 @@ 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 6bc1c1a06cd8a..885c1b3efb566 100644 --- a/fml/task_runner.h +++ b/fml/task_runner.h @@ -62,14 +62,6 @@ 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/fixtures/shell_test.dart b/shell/common/fixtures/shell_test.dart index 24dc227aa8f40..60727567e5737 100644 --- a/shell/common/fixtures/shell_test.dart +++ b/shell/common/fixtures/shell_test.dart @@ -640,13 +640,3 @@ 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.cc b/shell/common/shell.cc index ba28d8e950514..c6078f78c7de6 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -2,7 +2,6 @@ // 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" @@ -1037,7 +1036,7 @@ void Shell::OnPlatformViewSetViewportMetrics(int64_t view_id, } }); - fml::TaskRunner::RunNowAndFlushMessages( + fml::TaskRunner::RunNowOrPostTask( task_runners_.GetUITaskRunner(), [engine = engine_->GetWeakPtr(), view_id, metrics]() { if (engine) { @@ -1076,16 +1075,24 @@ void Shell::OnPlatformViewDispatchPlatformMessage( } #endif // FLUTTER_RUNTIME_MODE == FLUTTER_RUNTIME_MODE_DEBUG - // 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)); - } - })); + 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)); + } + })); + } } // |PlatformView::Delegate| @@ -1097,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::RunNowAndFlushMessages( + fml::TaskRunner::RunNowOrPostTask( task_runners_.GetUITaskRunner(), fml::MakeCopyable([engine = weak_engine_, packet = std::move(packet), flow_id = next_pointer_flow_id_]() mutable { @@ -1115,8 +1122,7 @@ void Shell::OnPlatformViewDispatchSemanticsAction(int32_t node_id, FML_DCHECK(is_set_up_); FML_DCHECK(task_runners_.GetPlatformTaskRunner()->RunsTasksOnCurrentThread()); - fml::TaskRunner::RunNowAndFlushMessages( - task_runners_.GetUITaskRunner(), + task_runners_.GetUITaskRunner()->PostTask( fml::MakeCopyable([engine = engine_->GetWeakPtr(), node_id, action, args = std::move(args)]() mutable { if (engine) { @@ -1130,13 +1136,12 @@ void Shell::OnPlatformViewSetSemanticsEnabled(bool enabled) { FML_DCHECK(is_set_up_); FML_DCHECK(task_runners_.GetPlatformTaskRunner()->RunsTasksOnCurrentThread()); - fml::TaskRunner::RunNowAndFlushMessages( - task_runners_.GetUITaskRunner(), - [engine = engine_->GetWeakPtr(), enabled] { - if (engine) { - engine->SetSemanticsEnabled(enabled); - } - }); + fml::TaskRunner::RunNowOrPostTask(task_runners_.GetUITaskRunner(), + [engine = engine_->GetWeakPtr(), enabled] { + if (engine) { + engine->SetSemanticsEnabled(enabled); + } + }); } // |PlatformView::Delegate| @@ -1144,12 +1149,12 @@ void Shell::OnPlatformViewSetAccessibilityFeatures(int32_t flags) { FML_DCHECK(is_set_up_); FML_DCHECK(task_runners_.GetPlatformTaskRunner()->RunsTasksOnCurrentThread()); - fml::TaskRunner::RunNowAndFlushMessages( - task_runners_.GetUITaskRunner(), [engine = engine_->GetWeakPtr(), flags] { - if (engine) { - engine->SetAccessibilityFeatures(flags); - } - }); + fml::TaskRunner::RunNowOrPostTask(task_runners_.GetUITaskRunner(), + [engine = engine_->GetWeakPtr(), flags] { + if (engine) { + engine->SetAccessibilityFeatures(flags); + } + }); } // |PlatformView::Delegate| diff --git a/shell/common/shell_unittests.cc b/shell/common/shell_unittests.cc index 8c6a0c2f4788c..ba56e9fb57944 100644 --- a/shell/common/shell_unittests.cc +++ b/shell/common/shell_unittests.cc @@ -4312,8 +4312,7 @@ TEST_F(ShellTest, NavigationMessageDispachedImmediately) { ASSERT_FALSE(DartVMRef::IsInstanceRunning()); } -// Verifies a semantics Action will flush the dart event loop. -TEST_F(ShellTest, SemanticsActionsFlushMessageLoop) { +TEST_F(ShellTest, SemanticsActionsPostTask) { Settings settings = CreateSettingsForFixture(); ThreadHost thread_host("io.flutter.test." + GetCurrentTestName() + ".", ThreadHost::Type::kPlatform); @@ -4328,40 +4327,13 @@ TEST_F(ShellTest, SemanticsActionsFlushMessageLoop) { configuration.SetEntrypoint("testSemanticsActions"); RunEngine(shell.get(), std::move(configuration)); - 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(); })); task_runners.GetPlatformTaskRunner()->PostTask([&] { SendSemanticsAction(shell.get(), 0, SemanticsAction::kTap, fml::MallocMapping(nullptr, 0)); }); - 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)); + // 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 @@ -4369,8 +4341,6 @@ 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); diff --git a/shell/common/vsync_waiter.cc b/shell/common/vsync_waiter.cc index 7457fd397392f..80074fa3ef982 100644 --- a/shell/common/vsync_waiter.cc +++ b/shell/common/vsync_waiter.cc @@ -128,10 +128,7 @@ void VsyncWaiter::FireCallback(fml::TimePoint frame_start_time, fml::TaskQueueId ui_task_queue_id = task_runners_.GetUITaskRunner()->GetTaskQueueId(); - // 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(), + task_runners_.GetUITaskRunner()->PostTask( [ui_task_queue_id, callback, flow_identifier, frame_start_time, frame_target_time, pause_secondary_tasks]() { FML_TRACE_EVENT_WITH_FLOW_IDS(