[ozone/x11] Fixed the crash during drag and drop.

The X11Window sometimes did not provide data to the drag and drop client
on drag entering the window, which later (on handling further events)
resulted in DesktopDragDropClientOzone::UpdateTargetAndCreateDropEvent()
dereferencing nullptr and thus creating an invalid reference to data
in the DropTargetEvent, which then caused the crash.

Why exactly the window might not provide data cannot be known for sure.
The essential point in the X11 drag and drop session is XdndPosition
event tht is handled by X11Window::UpdateDrag(), where the drag data is
always created.  The newly created data should then be passed to
DesktopDragDropClientOzone::OnDragEnter(), but because this call must
happen exactly once per drag and drop session, the X11Window has the
notified_enter_ flag for that; the call to OnDragEnter() happens iff
the flag was reset, and the flag is then set and kept till the end of
the session.  So the likely reason of the crash was that notified_enter_
was true before the call to DesktopDragDropClientOzone::OnDragEnter(),
which resulted in the call not happening.

X11Window::StartDrag() resets the flag, so the only case when things
could go wrong is the drag incoming from another window.

The code around had DCHECKs that assert valid state of the data, but
they did not fire neither in tests nor during development, which
suggests that the event is quite rare.

Theoretically, the reason could be incorrect sequence of incoming
events: either the previous drag and drop did not end properly, or the
entering drag did not send some events.

This CL fixes the situation with two changes:
1. The notified_enter_ flag is reset on entering the foreign drag, thus
   ensuring the correct state at the beginning of the incoming drag
   session.
2. The DesktopDragDropClientOzone::UpdateTargetAndCreateDropEvent()
   checks data_to_drop_ before passing it further, so nullptr should
   never be dereferenced anymore.

Bug: 1151836
Change-Id: Id007d5ee463fbaeaf4311166c72a397f48a8e9ec
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2558344
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Maksim Sisov (GMT+2) <msisov@igalia.com>
Commit-Queue: Alexander Dunaev <adunaev@igalia.com>
Cr-Commit-Position: refs/heads/master@{#832822}
diff --git a/ui/platform_window/x11/x11_window.cc b/ui/platform_window/x11/x11_window.cc
index 63a44b1..45e4235 100644
--- a/ui/platform_window/x11/x11_window.cc
+++ b/ui/platform_window/x11/x11_window.cc
@@ -883,6 +883,7 @@
 }
 
 void X11Window::OnBeginForeignDrag(x11::Window window) {
+  notified_enter_ = false;
   source_window_events_ = std::make_unique<ui::XScopedEventSelector>(
       window, x11::EventMask::PropertyChange);
 }
@@ -901,11 +902,9 @@
 
 int X11Window::PerformDrop() {
   WmDropHandler* drop_handler = GetWmDropHandler(*this);
-  if (!drop_handler)
+  if (!drop_handler || !notified_enter_)
     return DragDropTypes::DRAG_NONE;
 
-  DCHECK(notified_enter_);
-
   // The drop data has been supplied on entering the window.  The drop handler
   // should have it since then.
   auto* target_current_context = drag_drop_client_->target_current_context();
diff --git a/ui/views/widget/desktop_aura/desktop_drag_drop_client_ozone.cc b/ui/views/widget/desktop_aura/desktop_drag_drop_client_ozone.cc
index 80b7084..420b91a 100644
--- a/ui/views/widget/desktop_aura/desktop_drag_drop_client_ozone.cc
+++ b/ui/views/widget/desktop_aura/desktop_drag_drop_client_ozone.cc
@@ -250,26 +250,28 @@
   // before handling the actual drop.
   const bool posponed_enter_and_update = !data_to_drop_;
 
-  // If we had |data_to_drop_| already since the drag had entered the window,
-  // then we don't expect new data to come now, and vice versa.
-  DCHECK((data_to_drop_ && !data) || (!data_to_drop_ && data));
+  // If we didn't have |data_to_drop_| already since the drag had entered the
+  // window, take the new data that comes now.
   if (!data_to_drop_)
     data_to_drop_ = std::move(data);
 
-  // This will call the delegate's OnDragEntered if needed.
-  auto event = UpdateTargetAndCreateDropEvent(last_drag_point_, modifiers);
-  if (drag_drop_delegate_ && event) {
-    if (posponed_enter_and_update) {
-      // TODO(https://crbug.com/1014860): deal with drop refusals.
-      // The delegate's OnDragUpdated returns an operation that the delegate
-      // would accept.  Normally the accepted operation would be propagated
-      // properly, and if the delegate didn't accept it, the drop would never
-      // be called, but in this scenario of postponed updates we send all events
-      // at once.  Now we just drop, but perhaps we could call OnDragLeave
-      // and quit?
-      drag_drop_delegate_->OnDragUpdated(*event);
+  // crbug.com/1151836: check that we have data.
+  if (data_to_drop_) {
+    // This will call the delegate's OnDragEntered if needed.
+    auto event = UpdateTargetAndCreateDropEvent(last_drag_point_, modifiers);
+    if (drag_drop_delegate_ && event) {
+      if (posponed_enter_and_update) {
+        // TODO(https://crbug.com/1014860): deal with drop refusals.
+        // The delegate's OnDragUpdated returns an operation that the delegate
+        // would accept.  Normally the accepted operation would be propagated
+        // properly, and if the delegate didn't accept it, the drop would never
+        // be called, but in this scenario of postponed updates we send all
+        // events at once.  Now we just drop, but perhaps we could call
+        // OnDragLeave and quit?
+        drag_drop_delegate_->OnDragUpdated(*event);
+      }
+      drag_drop_delegate_->OnPerformDrop(*event, std::move(data_to_drop_));
     }
-    drag_drop_delegate_->OnPerformDrop(*event, std::move(data_to_drop_));
   }
   ResetDragDropTarget(false);
 }
@@ -340,6 +342,8 @@
 DesktopDragDropClientOzone::UpdateTargetAndCreateDropEvent(
     const gfx::PointF& location,
     int modifiers) {
+  DCHECK(data_to_drop_);
+
   const gfx::Point point(location.x(), location.y());
   aura::Window* window = GetTargetWindow(root_window_, point);
   if (!window) {
diff --git a/ui/views/widget/desktop_aura/desktop_drag_drop_client_ozone_unittest.cc b/ui/views/widget/desktop_aura/desktop_drag_drop_client_ozone_unittest.cc
index f87da0c..9c6073c 100644
--- a/ui/views/widget/desktop_aura/desktop_drag_drop_client_ozone_unittest.cc
+++ b/ui/views/widget/desktop_aura/desktop_drag_drop_client_ozone_unittest.cc
@@ -158,11 +158,19 @@
  private:
   // aura::client::DragDropDelegate:
   void OnDragEntered(const ui::DropTargetEvent& event) override {
+    // The event must always have valid data.  This will crash if it doesn't.
+    // See crbug.com/1151836.
+    auto dummy_copy = event.data().provider().Clone();
+
     ++num_enters_;
     last_event_flags_ = event.flags();
   }
 
   int OnDragUpdated(const ui::DropTargetEvent& event) override {
+    // The event must always have valid data.  This will crash if it doesn't.
+    // See crbug.com/1151836.
+    auto dummy_copy = event.data().provider().Clone();
+
     ++num_updates_;
     last_event_flags_ = event.flags();
     return destination_operation_;
@@ -172,6 +180,10 @@
 
   int OnPerformDrop(const ui::DropTargetEvent& event,
                     std::unique_ptr<ui::OSExchangeData> data) override {
+    // The event must always have valid data.  This will crash if it doesn't.
+    // See crbug.com/1151836.
+    auto dummy_copy = event.data().provider().Clone();
+
     ++num_drops_;
     received_data_ = std::move(data);
     last_event_flags_ = event.flags();
@@ -407,4 +419,20 @@
   EXPECT_EQ(0, another_dragdrop_delegate->num_exits());
 }
 
+// crbug.com/1151836 was null dereference during drag and drop.
+//
+// A possible reason was invalid sequence of events, so here we just drop data
+// without any notifications that should come before (like drag enter or drag
+// motion).  Before this change, that would hit some DCHECKS in the debug build
+// or cause crash in the release one, now it is handled properly.  Methods of
+// FakeDragDropDelegate ensure that data in the event is always valid.
+//
+// The error message rendered in the console when this test is running is the
+// expected and valid side effect.
+//
+// See more information in the bug.
+TEST_F(DesktopDragDropClientOzoneTest, Bug1151836) {
+  platform_window_->OnDragDrop(nullptr);
+}
+
 }  // namespace views