ozone/wayland: tabdrag: Ignore wl_pointer events until session finishes

A regression was introduced with initial extended-drag integration in
Ozone/Wayland, which causes some misbehaviours described in more details
at https://crbug.com/1148021.

This CL addresses the issue the following issue: wl_pointer events may
come in while the drag session is still running, which is particularly
troublesome when it happens just after the drop event, making the event
to be dispatched to the wrong window, e.g: the latest one for which an
wl_data_device::enter was received. Further details can be found in the
above mentioned link.

To make window drag controller more resistant to such cases, this
modifies it such that it ignores wl_pointer events until the session is
finished.

Bug: 1148021
Change-Id: I559ec271dd2d5bfd5609a8f89528ddda760fe321
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2533693
Commit-Queue: Nick Yamane <nickdiego@igalia.com>
Reviewed-by: Antonio Gomes <tonikitoo@igalia.com>
Cr-Commit-Position: refs/heads/master@{#826997}

ui/ozone/platform/wayland/host/wayland_window_drag_controller.cc

@@ -124,6 +124,7 @@ bool WaylandWindowDragController::StartDragSession() {
   data_device_->StartDrag(*data_source_, *origin_window_,
                           /*icon_surface=*/nullptr, this);
   pointer_grab_owner_ = origin_window_;
+  should_process_drag_event_ = false;
 
   // Observe window so we can take ownership of the origin surface in case it
   // is destroyed during the DND session.
@@ -217,6 +218,7 @@ void WaylandWindowDragController::OnDragMotion(const gfx::PointF& location) {
   VLOG(2) << "OnMotion. location=" << location.ToString();
 
   // Forward cursor location update info to the input handling delegate.
+  should_process_drag_event_ = true;
   pointer_location_ = location;
   pointer_delegate_->OnPointerMotionEvent(location);
 }
@@ -261,12 +263,8 @@ void WaylandWindowDragController::OnDragDrop() {
   DCHECK_GE(state_, State::kAttached);
   VLOG(1) << "Dropped. state=" << state_;
 
-  // Some compositors, e.g: Exo, may delay the wl_data_source::cancelled event
-  // delivery for some seconds, when the drop happens within a toplevel surface.
-  // Such event is handled by OnDataSourceFinish() function below, which is the
-  // single entry point for the drop event in window drag controller. In order
-  // to prevent such delay, the current data offer must be destroyed here.
   DCHECK(data_offer_);
+  data_offer_->FinishOffer();
   data_offer_.reset();
 }
 
@@ -355,6 +353,9 @@ void WaylandWindowDragController::HandleMotionEvent(MouseEvent* event) {
   DCHECK(dragged_window_);
   DCHECK(event);
 
+  if (!should_process_drag_event_)
+    return;
+
   // Update current cursor position, so it can be retrieved later on through
   // |Screen::GetCursorScreenPoint| API.
   int32_t scale = dragged_window_->buffer_scale();
@@ -371,6 +372,8 @@ void WaylandWindowDragController::HandleMotionEvent(MouseEvent* event) {
   gfx::Point new_location = event->location() - drag_offset_;
   gfx::Size size = dragged_window_->GetBounds().size();
   dragged_window_->SetBounds({new_location, size});
+
+  should_process_drag_event_ = false;
 }
 
 // Dispatch mouse release event (to tell clients that the drop just happened)

ui/ozone/platform/wayland/host/wayland_window_drag_controller.h

@@ -153,6 +153,10 @@ class WaylandWindowDragController : public WaylandDataDevice::DragDelegate,
   std::unique_ptr<ScopedEventDispatcher> nested_dispatcher_;
   base::OnceClosure quit_loop_closure_;
 
+  // Tells if the current drag event should be processedc. E.g: received through
+  // wl_data_device::motion wayland event.
+  bool should_process_drag_event_ = false;
+
   base::WeakPtrFactory<WaylandWindowDragController> weak_factory_{this};
 };
 

ui/ozone/platform/wayland/host/wayland_window_drag_controller_unittest.cc

@@ -162,10 +162,15 @@ class WaylandWindowDragControllerTest : public WaylandTest,
 
   void SendPointerMotion(WaylandWindow* window,
                          MockPlatformWindowDelegate* delegate,
-                         gfx::Point location) {
+                         gfx::Point location,
+                         bool sync_and_ensure_dispatched = true) {
     wl_fixed_t x = wl_fixed_from_int(location.x());
     wl_fixed_t y = wl_fixed_from_int(location.y());
     wl_pointer_send_motion(pointer_->resource(), NextTime(), x, y);
+
+    if (!sync_and_ensure_dispatched)
+      return;
+
     EXPECT_CALL(*delegate, DispatchEvent(_)).WillOnce([](Event* event) {
       EXPECT_TRUE(event->IsMouseEvent());
       EXPECT_EQ(ET_MOUSE_DRAGGED, event->type());
@@ -609,6 +614,115 @@ TEST_P(WaylandWindowDragControllerTest, RestoreDuringWindowDragSession) {
   EXPECT_EQ(PlatformWindowState::kMaximized, window_->GetPlatformWindowState());
 }
 
+// Check the following flow works as expected:
+//
+// 1. With a single 1 window open,
+// 2. Move pointer into it, press left button, move cursor a bit (drag),
+// 3. Run move loop, drag it from 200,200 location to 100,100
+// 4. Send a few wl_pointer::motion events targeting 20,20 location and ensure
+//    they are ignored (i.e: window bounds keep unchanged) until drop happens.
+//
+// Verifies window drag controller is resistant to issues such as
+// https://crbug.com/1148021.
+TEST_P(WaylandWindowDragControllerTest, IgnorePointerEventsUntilDrop) {
+  // Ensure there is no window currently focused
+  EXPECT_FALSE(window_manager()->GetCurrentFocusedWindow());
+  EXPECT_EQ(gfx::kNullAcceleratedWidget,
+            screen_->GetLocalProcessWidgetAtPoint({200, 200}, {}));
+
+  SendPointerEnter(window_.get(), &delegate_);
+  SendPointerPress(window_.get(), &delegate_, BTN_LEFT);
+  SendPointerMotion(window_.get(), &delegate_, {200, 200});
+
+  // Set up an "interaction flow", start the drag session and run move loop:
+  //  - Event dispatching and bounds changes are monitored
+  //  - At each event, emulates a new event at server side and proceeds to the
+  //  next test step.
+  auto* wayland_extension = GetWaylandExtension(*window_);
+  wayland_extension->StartWindowDraggingSessionIfNeeded();
+  EXPECT_EQ(State::kAttached, drag_controller()->state());
+
+  auto* move_loop_handler = GetWmMoveLoopHandler(*window_);
+  DCHECK(move_loop_handler);
+
+  enum { kStarted, kDragging, kDropping, kDone } test_step = kStarted;
+
+  EXPECT_CALL(delegate_, DispatchEvent(_)).WillRepeatedly([&](Event* event) {
+    EXPECT_TRUE(event->IsMouseEvent());
+    switch (test_step) {
+      case kStarted:
+        EXPECT_EQ(ET_MOUSE_ENTERED, event->type());
+        EXPECT_EQ(State::kDetached, drag_controller()->state());
+        // Ensure PlatformScreen keeps consistent.
+        EXPECT_EQ(window_->GetWidget(),
+                  screen_->GetLocalProcessWidgetAtPoint({200, 200}, {}));
+        // Drag it a bit more.
+        SendDndMotion({100, 100});
+        test_step = kDragging;
+        break;
+      case kDropping:
+        EXPECT_EQ(ET_MOUSE_RELEASED, event->type());
+        EXPECT_EQ(State::kDropped, drag_controller()->state());
+
+        // Ensure |window_|'s bounds did not change in response to 20,20
+        // wl_pointer::motion events sent at |kDragging| test step.
+        EXPECT_EQ(gfx::kNullAcceleratedWidget,
+                  screen_->GetLocalProcessWidgetAtPoint({20, 20}, {}));
+        EXPECT_EQ(window_->GetWidget(),
+                  screen_->GetLocalProcessWidgetAtPoint({100, 100}, {}));
+
+        // Rather, only PlatformScreen's cursor position is updated accordingly.
+        EXPECT_EQ(gfx::Point(20, 20), screen_->GetCursorScreenPoint());
+        test_step = kDone;
+        break;
+      case kDone:
+        EXPECT_EQ(ET_MOUSE_EXITED, event->type());
+        EXPECT_EQ(window_->GetWidget(),
+                  screen_->GetLocalProcessWidgetAtPoint({100, 100}, {}));
+        break;
+      case kDragging:
+      default:
+        FAIL() << " event=" << event->ToString()
+               << " state=" << drag_controller()->state()
+               << " step=" << static_cast<int>(test_step);
+        return;
+    }
+  });
+
+  EXPECT_CALL(delegate_, OnBoundsChanged(_))
+      .WillOnce([&](const gfx::Rect& bounds) {
+        EXPECT_EQ(State::kDetached, drag_controller()->state());
+        EXPECT_EQ(kDragging, test_step);
+        EXPECT_EQ(gfx::Point(100, 100), bounds.origin());
+
+        // Send a few wl_pointer::motion events skipping sync and dispatch
+        // checks, which will be done at |kDropping| test step handling.
+        SendPointerMotion(window_.get(), &delegate_, {30, 30},
+                          /*sync_and_ensure_dispatched =*/false);
+        SendPointerMotion(window_.get(), &delegate_, {20, 20},
+                          /*sync_and_ensure_dispatched =*/false);
+
+        SendDndDrop();
+        test_step = kDropping;
+      });
+
+  // RunMoveLoop() blocks until the dragging session ends, so resume test
+  // server's run loop until it returns.
+  server_.Resume();
+  move_loop_handler->RunMoveLoop({});
+  server_.Pause();
+
+  SendPointerEnter(window_.get(), &delegate_);
+  Sync();
+
+  EXPECT_EQ(State::kIdle, drag_controller()->state());
+  EXPECT_EQ(window_.get(), window_manager()->GetCurrentFocusedWindow());
+  EXPECT_EQ(window_->GetWidget(),
+            screen_->GetLocalProcessWidgetAtPoint({100, 100}, {}));
+  EXPECT_EQ(gfx::kNullAcceleratedWidget,
+            screen_->GetLocalProcessWidgetAtPoint({20, 20}, {}));
+}
+
 INSTANTIATE_TEST_SUITE_P(XdgVersionStableTest,
                          WaylandWindowDragControllerTest,
                          ::testing::Values(kXdgShellStable));