ozone/wayland: dnd: Gracefully handle early origin surface destruction

The drag-and-drop origin surface may get suddenly destroyed, when it is
a xdg-popup, eg. dragging bookmarks bar's menu item in Chrome, which
currently leads to crashes in WaylandDataDragController. This CL fixes
it as well as adds unit tests to prevent future similar regressions.

R=msisov@igalia.com, tonikitoo@igalia.com

Bug: 1143707
Test: Covered by ozone_unittests --ozone-platform=wayland
Change-Id: I1bac3032c428d3456bf644b35f869ec6ff5122e4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2635449
Auto-Submit: Nick Yamane <nickdiego@igalia.com>
Commit-Queue: Maksim Sisov <msisov@igalia.com>
Reviewed-by: Maksim Sisov <msisov@igalia.com>
Cr-Commit-Position: refs/heads/master@{#845701}

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

@@ -163,7 +163,7 @@ void WaylandDataDevice::OnLeave(void* data, wl_data_device* data_device) {
   // potential use-after-free. Above call to OnDragLeave() may result in
   // |drag_delegate_| being reset, so it must be checked here as well.
   if (self->drag_delegate_ && !self->drag_delegate_->IsDragSource())
-    self->drag_delegate_ = nullptr;
+    self->ResetDragDelegate();
 }
 
 void WaylandDataDevice::OnSelection(void* data,

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

@@ -64,8 +64,6 @@ WaylandDataDragController::WaylandDataDragController(
   DCHECK(window_manager_);
   DCHECK(data_device_manager_);
   DCHECK(data_device_);
-
-  window_manager_->AddObserver(this);
 }
 
 WaylandDataDragController::~WaylandDataDragController() = default;
@@ -99,14 +97,15 @@ void WaylandDataDragController::StartSession(const OSExchangeData& data,
   state_ = State::kStarted;
   data_device_->StartDrag(*data_source_, *origin_window_, icon_surface_.get(),
                           this);
+
+  window_manager_->AddObserver(this);
 }
 
-// Sessions initiated from Chromium, will have |origin_window_| pointing to the
-// window where the drag started in. In such cases, |data_| is expected to be
-// non-null, which can be used to save some IO cycles.
+// Sessions initiated from Chromium, will have |data_source_| set. In which
+// case, |data_| is expected to be non-null as well.
 bool WaylandDataDragController::IsDragSource() const {
-  DCHECK(!origin_window_ || data_);
-  return !!origin_window_;
+  DCHECK(!data_source_ || data_);
+  return !!data_source_;
 }
 
 void WaylandDataDragController::DrawIcon() {
@@ -218,16 +217,17 @@ void WaylandDataDragController::OnDragDrop() {
 
 void WaylandDataDragController::OnDataSourceFinish(bool completed) {
   DCHECK(data_source_);
-  DCHECK(origin_window_);
-
-  origin_window_->OnDragSessionClose(data_source_->dnd_action());
 
-  // DnD handlers expect DragLeave to be sent for drag sessions that end up
-  // with no data transfer (wl_data_source::cancelled event).
-  if (!completed)
-    origin_window_->OnDragLeave();
+  if (origin_window_) {
+    origin_window_->OnDragSessionClose(data_source_->dnd_action());
+    // DnD handlers expect DragLeave to be sent for drag sessions that end up
+    // with no data transfer (wl_data_source::cancelled event).
+    if (!completed)
+      origin_window_->OnDragLeave();
+    origin_window_ = nullptr;
+  }
 
-  origin_window_ = nullptr;
+  window_manager_->RemoveObserver(this);
   data_source_.reset();
   data_offer_.reset();
   data_.reset();
@@ -249,6 +249,9 @@ void WaylandDataDragController::OnDataSourceSend(const std::string& mime_type,
 void WaylandDataDragController::OnWindowRemoved(WaylandWindow* window) {
   if (window == window_)
     window_ = nullptr;
+
+  if (window == origin_window_)
+    origin_window_ = nullptr;
 }
 
 void WaylandDataDragController::Offer(const OSExchangeData& data,
@@ -287,9 +290,8 @@ void WaylandDataDragController::Offer(const OSExchangeData& data,
 // |received_data_| to the drop handler.
 void WaylandDataDragController::HandleUnprocessedMimeTypes(
     base::TimeTicks start_time) {
-  DCHECK_EQ(state_, State::kTransferring);
   std::string mime_type = GetNextUnprocessedMimeType();
-  if (mime_type.empty() || is_leave_pending_) {
+  if (mime_type.empty() || is_leave_pending_ || state_ == State::kIdle) {
     OnDataTransferFinished(start_time, std::move(received_data_));
   } else {
     DCHECK(data_offer_);
@@ -303,7 +305,6 @@ void WaylandDataDragController::HandleUnprocessedMimeTypes(
 void WaylandDataDragController::OnMimeTypeDataTransferred(
     base::TimeTicks start_time,
     PlatformClipboard::Data contents) {
-  DCHECK_EQ(state_, State::kTransferring);
   DCHECK(contents);
   if (!contents->data().empty()) {
     std::string mime_type = unprocessed_mime_types_.front();
@@ -319,6 +320,9 @@ void WaylandDataDragController::OnDataTransferFinished(
     base::TimeTicks start_time,
     std::unique_ptr<OSExchangeData> received_data) {
   unprocessed_mime_types_.clear();
+  if (state_ == State::kIdle)
+    return;
+
   state_ = State::kIdle;
 
   // If |is_leave_pending_| is set, it means a 'leave' event was fired while

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

@@ -536,6 +536,7 @@ TEST_P(WaylandDataDragControllerTest, ForeignDragHandleAskAction) {
   data_device_manager_->data_device()->OnLeave();
 }
 
+// Verifies entered surface destruction is properly handled.
 // Regression test for https://crbug.com/1143707.
 TEST_P(WaylandDataDragControllerTest, DestroyEnteredSurface) {
   auto* window_1 = window_.get();
@@ -556,6 +557,7 @@ TEST_P(WaylandDataDragControllerTest, DestroyEnteredSurface) {
 
     // Destroy the entered window at client side and emulates a
     // wl_data_device::leave to ensure no UAF happens.
+    window_2->PrepareForShutdown();
     window_2.reset();
     self->SendDndLeave();
     self->Sync();
@@ -575,6 +577,55 @@ TEST_P(WaylandDataDragControllerTest, DestroyEnteredSurface) {
   window_1->SetPointerFocus(restored_focus);
 }
 
+// Verifies that early origin surface destruction is properly handled.
+// Regression test for https://crbug.com/1143707.
+TEST_P(WaylandDataDragControllerTest, DestroyOriginSurface) {
+  auto* window_1 = window_.get();
+  const bool restored_focus = window_1->has_pointer_focus();
+  window_1->SetPointerFocus(false);
+  ASSERT_EQ(PlatformWindowType::kWindow, window_1->type());
+
+  auto test = [](WaylandDataDragControllerTest* self,
+                 std::unique_ptr<WaylandWindow>* origin) {
+    // Leave origin surface and enter |window_|.
+    self->SendDndLeave();
+    self->SendDndEnter(self->window(), gfx::Point(20, 20));
+    self->Sync();
+
+    // Shutdown and destroy the popup window where the drag session was
+    // initiated, which leads to the drag loop to finish.
+    (*origin)->PrepareForShutdown();
+    origin->reset();
+  };
+
+  // Init and open |target_window|.
+  MockPlatformWindowDelegate delegate_2;
+  auto window_2 = CreateTestWindow(PlatformWindowType::kPopup,
+                                   gfx::Size(80, 80), &delegate_2);
+  window_2->SetPointerFocus(true);
+  Sync();
+
+  // Post test task to be performed asynchronously once the drag session gets
+  // started.
+  ScheduleTestTask(base::BindOnce(test, base::Unretained(this),
+                                  base::Unretained(&window_2)));
+
+  // Request to start the drag session, which spins a nested run loop.
+  OSExchangeData os_exchange_data;
+  os_exchange_data.SetString(sample_text_for_dnd());
+  window_2->StartDrag(os_exchange_data, DragDropTypes::DRAG_COPY, {}, true,
+                      drag_handler_delegate_.get());
+  Sync();
+
+  // Send wl_data_source::cancelled event. The drag controller is then
+  // expected to gracefully reset its internal state.
+  SendDndLeave();
+  SendDndCancelled();
+  Sync();
+
+  window_1->SetPointerFocus(restored_focus);
+}
+
 // Ensures drag/drop events are properly propagated to non-toplevel windows.
 TEST_P(WaylandDataDragControllerTest, DragToNonToplevelWindows) {
   auto* origin_window = window_.get();

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

@@ -76,12 +76,6 @@ WaylandWindow::~WaylandWindow() {
 
   if (parent_window_)
     parent_window_->set_child_window(nullptr);
-
-  if (drag_handler_delegate_) {
-    drag_handler_delegate_->OnDragFinished(
-        DragDropTypes::DragOperation::DRAG_NONE);
-  }
-  CancelDrag();
 }
 
 void WaylandWindow::OnWindowLostCapture() {
@@ -184,7 +178,10 @@ bool WaylandWindow::IsVisible() const {
   return false;
 }
 
-void WaylandWindow::PrepareForShutdown() {}
+void WaylandWindow::PrepareForShutdown() {
+  if (drag_handler_delegate_)
+    OnDragSessionClose(DragDropTypes::DRAG_NONE);
+}
 
 void WaylandWindow::SetBounds(const gfx::Rect& bounds_px) {
   if (bounds_px_ == bounds_px)