From 48799ceebf0985bf7acdb5354cba1e024758dfc5 Mon Sep 17 00:00:00 2001 From: Tong Mu Date: Thu, 6 Jun 2024 00:18:20 -0700 Subject: [PATCH] Make pointer events ignore invalid views (flutter/engine#51925) With this PR, Flutter will now ignore pointer events for an invalid view, i.e. views that have not been add or have been removed. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style --- .../window/pointer_data_packet_converter.cc | 7 +- .../ui/window/pointer_data_packet_converter.h | 26 ++++++- ...pointer_data_packet_converter_unittests.cc | 67 +++++++++++++++---- .../src/flutter/runtime/runtime_controller.cc | 16 ++++- .../src/flutter/runtime/runtime_controller.h | 6 +- .../shell/common/input_events_unittests.cc | 11 ++- engine/src/flutter/shell/common/shell_test.cc | 6 +- engine/src/flutter/shell/common/shell_test.h | 4 +- .../embedder/tests/embedder_unittests.cc | 26 ++++++- 9 files changed, 146 insertions(+), 23 deletions(-) diff --git a/engine/src/flutter/lib/ui/window/pointer_data_packet_converter.cc b/engine/src/flutter/lib/ui/window/pointer_data_packet_converter.cc index 5cf200530e5..45b05a00dd8 100644 --- a/engine/src/flutter/lib/ui/window/pointer_data_packet_converter.cc +++ b/engine/src/flutter/lib/ui/window/pointer_data_packet_converter.cc @@ -11,7 +11,8 @@ namespace flutter { -PointerDataPacketConverter::PointerDataPacketConverter() {} +PointerDataPacketConverter::PointerDataPacketConverter(const Delegate& delegate) + : delegate_(delegate) {} PointerDataPacketConverter::~PointerDataPacketConverter() = default; @@ -39,6 +40,10 @@ std::unique_ptr PointerDataPacketConverter::Convert( void PointerDataPacketConverter::ConvertPointerData( PointerData pointer_data, std::vector& converted_pointers) { + // Ignores pointer events with an invalid view ID. + if (!delegate_.ViewExists(pointer_data.view_id)) { + return; + } if (pointer_data.signal_kind == PointerData::SignalKind::kNone) { switch (pointer_data.change) { case PointerData::Change::kCancel: { diff --git a/engine/src/flutter/lib/ui/window/pointer_data_packet_converter.h b/engine/src/flutter/lib/ui/window/pointer_data_packet_converter.h index e903e4e1d06..146ade258a4 100644 --- a/engine/src/flutter/lib/ui/window/pointer_data_packet_converter.h +++ b/engine/src/flutter/lib/ui/window/pointer_data_packet_converter.h @@ -75,7 +75,26 @@ struct PointerState { /// class PointerDataPacketConverter { public: - PointerDataPacketConverter(); + // Used by PointerDataPacketConverter to query the system status. + // + // Typically RuntimeController. + class Delegate { + public: + Delegate() = default; + + virtual ~Delegate() = default; + + // Returns true if the specified view exists. + virtual bool ViewExists(int64_t view_id) const = 0; + }; + + //---------------------------------------------------------------------------- + /// @brief Create a PointerDataPacketConverter. + /// + /// @param[in] delegate A delegate to fulfill the query to the app state. + /// The delegate must exist throughout the lifetime + /// of this class. Typically `RuntimeController`. + explicit PointerDataPacketConverter(const Delegate& delegate); ~PointerDataPacketConverter(); //---------------------------------------------------------------------------- @@ -85,6 +104,8 @@ class PointerDataPacketConverter { /// pointer transitions. This method will fill out that /// information and attempt to correct pointer transitions. /// + /// Pointer data with invalid view IDs will be ignored. + /// /// @param[in] packet The raw pointer packet sent from /// embedding. /// @@ -95,6 +116,9 @@ class PointerDataPacketConverter { std::unique_ptr Convert(const PointerDataPacket& packet); private: + const Delegate& delegate_; + + // A map from pointer device ID to the state of the pointer. std::map states_; int64_t pointer_ = 0; diff --git a/engine/src/flutter/lib/ui/window/pointer_data_packet_converter_unittests.cc b/engine/src/flutter/lib/ui/window/pointer_data_packet_converter_unittests.cc index 69f798dffc5..4f6a37f4536 100644 --- a/engine/src/flutter/lib/ui/window/pointer_data_packet_converter_unittests.cc +++ b/engine/src/flutter/lib/ui/window/pointer_data_packet_converter_unittests.cc @@ -5,12 +5,34 @@ #include "flutter/lib/ui/window/pointer_data_packet_converter.h" #include +#include #include "gtest/gtest.h" namespace flutter { namespace testing { +namespace { + +constexpr int64_t kImplicitViewId = 0; + +} + +class TestDelegate : public PointerDataPacketConverter::Delegate { + public: + // |PointerDataPacketConverter::Delegate| + bool ViewExists(int64_t view_id) const override { + return views_.count(view_id) != 0; + } + + void AddView(int64_t view_id) { views_.insert(view_id); } + + void RemoveView(int64_t view_id) { views_.erase(view_id); } + + private: + std::unordered_set views_; +}; + void CreateSimulatedPointerData(PointerData& data, // NOLINT PointerData::Change change, int64_t device, @@ -45,7 +67,7 @@ void CreateSimulatedPointerData(PointerData& data, // NOLINT data.platformData = 0; data.scroll_delta_x = 0.0; data.scroll_delta_y = 0.0; - data.view_id = 0; + data.view_id = kImplicitViewId; } void CreateSimulatedMousePointerData(PointerData& data, // NOLINT @@ -85,7 +107,7 @@ void CreateSimulatedMousePointerData(PointerData& data, // NOLINT data.platformData = 0; data.scroll_delta_x = scroll_delta_x; data.scroll_delta_y = scroll_delta_y; - data.view_id = 0; + data.view_id = kImplicitViewId; } void CreateSimulatedTrackpadGestureData(PointerData& data, // NOLINT @@ -144,7 +166,9 @@ void UnpackPointerPacket(std::vector& output, // NOLINT } TEST(PointerDataPacketConverterTest, CanConvertPointerDataPacket) { - PointerDataPacketConverter converter; + TestDelegate delegate; + delegate.AddView(kImplicitViewId); + PointerDataPacketConverter converter(delegate); auto packet = std::make_unique(6); PointerData data; CreateSimulatedPointerData(data, PointerData::Change::kAdd, 0, 0.0, 0.0, 0); @@ -193,7 +217,9 @@ TEST(PointerDataPacketConverterTest, CanConvertPointerDataPacket) { } TEST(PointerDataPacketConverterTest, CanSynthesizeDownAndUp) { - PointerDataPacketConverter converter; + TestDelegate delegate; + delegate.AddView(kImplicitViewId); + PointerDataPacketConverter converter(delegate); auto packet = std::make_unique(4); PointerData data; CreateSimulatedPointerData(data, PointerData::Change::kAdd, 0, 0.0, 0.0, 0); @@ -244,7 +270,9 @@ TEST(PointerDataPacketConverterTest, CanSynthesizeDownAndUp) { } TEST(PointerDataPacketConverterTest, CanUpdatePointerIdentifier) { - PointerDataPacketConverter converter; + TestDelegate delegate; + delegate.AddView(kImplicitViewId); + PointerDataPacketConverter converter(delegate); auto packet = std::make_unique(7); PointerData data; CreateSimulatedPointerData(data, PointerData::Change::kAdd, 0, 0.0, 0.0, 0); @@ -299,7 +327,9 @@ TEST(PointerDataPacketConverterTest, CanUpdatePointerIdentifier) { } TEST(PointerDataPacketConverterTest, AlwaysForwardMoveEvent) { - PointerDataPacketConverter converter; + TestDelegate delegate; + delegate.AddView(kImplicitViewId); + PointerDataPacketConverter converter(delegate); auto packet = std::make_unique(4); PointerData data; CreateSimulatedPointerData(data, PointerData::Change::kAdd, 0, 0.0, 0.0, 0); @@ -336,7 +366,9 @@ TEST(PointerDataPacketConverterTest, AlwaysForwardMoveEvent) { } TEST(PointerDataPacketConverterTest, CanWorkWithDifferentDevices) { - PointerDataPacketConverter converter; + TestDelegate delegate; + delegate.AddView(kImplicitViewId); + PointerDataPacketConverter converter(delegate); auto packet = std::make_unique(12); PointerData data; CreateSimulatedPointerData(data, PointerData::Change::kAdd, 0, 0.0, 0.0, 0); @@ -433,7 +465,9 @@ TEST(PointerDataPacketConverterTest, CanWorkWithDifferentDevices) { } TEST(PointerDataPacketConverterTest, CanSynthesizeAdd) { - PointerDataPacketConverter converter; + TestDelegate delegate; + delegate.AddView(kImplicitViewId); + PointerDataPacketConverter converter(delegate); auto packet = std::make_unique(2); PointerData data; CreateSimulatedPointerData(data, PointerData::Change::kDown, 0, 330.0, 450.0, @@ -478,7 +512,9 @@ TEST(PointerDataPacketConverterTest, CanSynthesizeAdd) { TEST(PointerDataPacketConverterTest, CanHandleThreeFingerGesture) { // Regression test https://github.com/flutter/flutter/issues/20517. - PointerDataPacketConverter converter; + TestDelegate delegate; + delegate.AddView(kImplicitViewId); + PointerDataPacketConverter converter(delegate); PointerData data; std::vector result; // First finger down. @@ -558,7 +594,9 @@ TEST(PointerDataPacketConverterTest, CanConvertPointerSignals) { PointerData::SignalKind::kScale, }; for (const PointerData::SignalKind& kind : signal_kinds) { - PointerDataPacketConverter converter; + TestDelegate delegate; + delegate.AddView(kImplicitViewId); + PointerDataPacketConverter converter(delegate); auto packet = std::make_unique(6); PointerData data; CreateSimulatedMousePointerData(data, PointerData::Change::kAdd, @@ -666,7 +704,9 @@ TEST(PointerDataPacketConverterTest, CanConvertPointerSignals) { } TEST(PointerDataPacketConverterTest, CanConvertTrackpadGesture) { - PointerDataPacketConverter converter; + TestDelegate delegate; + delegate.AddView(kImplicitViewId); + PointerDataPacketConverter converter(delegate); auto packet = std::make_unique(3); PointerData data; CreateSimulatedTrackpadGestureData(data, PointerData::Change::kPanZoomStart, @@ -717,7 +757,10 @@ TEST(PointerDataPacketConverterTest, CanConvertTrackpadGesture) { } TEST(PointerDataPacketConverterTest, CanConvertViewId) { - PointerDataPacketConverter converter; + TestDelegate delegate; + delegate.AddView(100); + delegate.AddView(200); + PointerDataPacketConverter converter(delegate); auto packet = std::make_unique(2); PointerData data; CreateSimulatedPointerData(data, PointerData::Change::kAdd, 0, 0.0, 0.0, 0); diff --git a/engine/src/flutter/runtime/runtime_controller.cc b/engine/src/flutter/runtime/runtime_controller.cc index ad4fe0985c9..6a607ed5d88 100644 --- a/engine/src/flutter/runtime/runtime_controller.cc +++ b/engine/src/flutter/runtime/runtime_controller.cc @@ -23,7 +23,10 @@ namespace flutter { RuntimeController::RuntimeController(RuntimeDelegate& p_client, const TaskRunners& task_runners) - : client_(p_client), vm_(nullptr), context_(task_runners) {} + : client_(p_client), + vm_(nullptr), + context_(task_runners), + pointer_data_packet_converter_(*this) {} RuntimeController::RuntimeController( RuntimeDelegate& p_client, @@ -43,7 +46,8 @@ RuntimeController::RuntimeController( isolate_create_callback_(p_isolate_create_callback), isolate_shutdown_callback_(p_isolate_shutdown_callback), persistent_isolate_data_(std::move(p_persistent_isolate_data)), - context_(p_context) {} + context_(p_context), + pointer_data_packet_converter_(*this) {} std::unique_ptr RuntimeController::Spawn( RuntimeDelegate& p_client, @@ -207,6 +211,10 @@ bool RuntimeController::RemoveView(int64_t view_id) { return platform_configuration->RemoveView(view_id); } +bool RuntimeController::ViewExists(int64_t view_id) const { + return platform_data_.viewport_metrics_for_views.count(view_id) != 0; +} + bool RuntimeController::SetViewportMetrics(int64_t view_id, const ViewportMetrics& metrics) { TRACE_EVENT0("flutter", "SetViewportMetrics"); @@ -360,7 +368,9 @@ bool RuntimeController::DispatchPointerDataPacket( TRACE_EVENT0("flutter", "RuntimeController::DispatchPointerDataPacket"); std::unique_ptr converted_packet = pointer_data_packet_converter_.Convert(packet); - platform_configuration->DispatchPointerDataPacket(*converted_packet); + if (converted_packet->GetLength() != 0) { + platform_configuration->DispatchPointerDataPacket(*converted_packet); + } return true; } diff --git a/engine/src/flutter/runtime/runtime_controller.h b/engine/src/flutter/runtime/runtime_controller.h index 89f3eaf86cf..93da07092e1 100644 --- a/engine/src/flutter/runtime/runtime_controller.h +++ b/engine/src/flutter/runtime/runtime_controller.h @@ -49,7 +49,8 @@ class Window; /// `RuntimeController` and flushed to the Dart VM when the isolate becomes /// ready before the entrypoint function. See `PlatformData`. /// -class RuntimeController : public PlatformConfigurationClient { +class RuntimeController : public PlatformConfigurationClient, + PointerDataPacketConverter::Delegate { public: /// A callback that's invoked after this `RuntimeController` attempts to /// add a view to the Dart isolate. @@ -723,6 +724,9 @@ class RuntimeController : public PlatformConfigurationClient { bool FlushRuntimeStateToIsolate(); + // |PointerDataPacketConverter::Delegate| + bool ViewExists(int64_t view_id) const override; + // |PlatformConfigurationClient| std::string DefaultRouteName() override; diff --git a/engine/src/flutter/shell/common/input_events_unittests.cc b/engine/src/flutter/shell/common/input_events_unittests.cc index d8ee75fc332..a0a22d14ded 100644 --- a/engine/src/flutter/shell/common/input_events_unittests.cc +++ b/engine/src/flutter/shell/common/input_events_unittests.cc @@ -19,6 +19,12 @@ using UnitlessTime = int; // returns the time of that frame. using Generator = std::function; +namespace { + +constexpr int64_t kImplicitViewId = 0; + +} + //---------------------------------------------------------------------------- /// Simulate n input events where the i-th one is delivered at delivery_time(i). /// @@ -124,7 +130,9 @@ static void TestSimulatedInputEvents( for (int i = 0, j = 0; i < num_events; j += 1) { double t = j * frame_time; while (i < num_events && delivery_time(i) <= t) { - ShellTest::DispatchFakePointerData(shell.get()); + // Use a different x every time for the pointer data converter to + // generate non-empty events. + ShellTest::DispatchFakePointerData(shell.get(), /*x=*/i); i += 1; } ShellTest::VSyncFlush(shell.get(), &will_draw_new_frame); @@ -182,6 +190,7 @@ void CreateSimulatedPointerData(PointerData& data, data.platformData = 0; data.scroll_delta_x = 0.0; data.scroll_delta_y = 0.0; + data.view_id = kImplicitViewId; } TEST_F(ShellTest, MissAtMostOneFrameForIrregularInputEvents) { diff --git a/engine/src/flutter/shell/common/shell_test.cc b/engine/src/flutter/shell/common/shell_test.cc index 90dcf3511f6..66833999477 100644 --- a/engine/src/flutter/shell/common/shell_test.cc +++ b/engine/src/flutter/shell/common/shell_test.cc @@ -258,8 +258,12 @@ void ShellTest::PumpOneFrame(Shell* shell, FrameContent frame_content) { latch.Wait(); } -void ShellTest::DispatchFakePointerData(Shell* shell) { +void ShellTest::DispatchFakePointerData(Shell* shell, double x) { auto packet = std::make_unique(1); + packet->SetPointerData(0, PointerData{ + .change = PointerData::Change::kHover, + .physical_x = x, + }); DispatchPointerData(shell, std::move(packet)); } diff --git a/engine/src/flutter/shell/common/shell_test.h b/engine/src/flutter/shell/common/shell_test.h index c11ad1174dc..b5f832a20a6 100644 --- a/engine/src/flutter/shell/common/shell_test.h +++ b/engine/src/flutter/shell/common/shell_test.h @@ -110,7 +110,9 @@ class ShellTest : public FixtureTest { static void PumpOneFrame(Shell* shell); static void PumpOneFrame(Shell* shell, FrameContent frame_content); - static void DispatchFakePointerData(Shell* shell); + // Dispatch a PointerHoverEvent with the specified `x` as the pointer + // position. + static void DispatchFakePointerData(Shell* shell, double x); static void DispatchPointerData(Shell* shell, std::unique_ptr packet); // Declare |UnreportedTimingsCount|, |GetNeedsReportTimings| and diff --git a/engine/src/flutter/shell/platform/embedder/tests/embedder_unittests.cc b/engine/src/flutter/shell/platform/embedder/tests/embedder_unittests.cc index 1e1075fca19..2b03e5105ed 100644 --- a/engine/src/flutter/shell/platform/embedder/tests/embedder_unittests.cc +++ b/engine/src/flutter/shell/platform/embedder/tests/embedder_unittests.cc @@ -3534,7 +3534,7 @@ TEST_F(EmbedderTest, CanSendPointerEventWithViewId) { builder.SetSoftwareRendererConfig(); builder.SetDartEntrypoint("pointer_data_packet_view_id"); - fml::AutoResetWaitableEvent ready_latch, count_latch, message_latch; + fml::AutoResetWaitableEvent ready_latch, add_view_latch, message_latch; context.AddNativeCallback( "SignalNativeTest", CREATE_NATIVE_ENTRY( @@ -3550,9 +3550,31 @@ TEST_F(EmbedderTest, CanSendPointerEventWithViewId) { auto engine = builder.LaunchEngine(); ASSERT_TRUE(engine.is_valid()); - ready_latch.Wait(); + // Add view 2 + FlutterWindowMetricsEvent metrics = {}; + metrics.struct_size = sizeof(FlutterWindowMetricsEvent); + metrics.width = 800; + metrics.height = 600; + metrics.pixel_ratio = 1.0; + metrics.view_id = 2; + + FlutterAddViewInfo info = {}; + info.struct_size = sizeof(FlutterAddViewInfo); + info.view_id = 2; + info.view_metrics = &metrics; + info.add_view_callback = [](const FlutterAddViewResult* result) { + EXPECT_TRUE(result->added); + fml::AutoResetWaitableEvent* add_view_latch = + reinterpret_cast(result->user_data); + add_view_latch->Signal(); + }; + info.user_data = &add_view_latch; + ASSERT_EQ(FlutterEngineAddView(engine.get(), &info), kSuccess); + add_view_latch.Wait(); + + // Send a pointer event for view 2 FlutterPointerEvent pointer_event = {}; pointer_event.struct_size = sizeof(FlutterPointerEvent); pointer_event.phase = FlutterPointerPhase::kAdd;