path: root/0011-ozone-wayland-Do-not-use-possibly-blocking-dispatch-.patch

From 4f4c199c6654ef75319eabf3977e916b2a744d3f Mon Sep 17 00:00:00 2001
From: Maksim Sisov <msisov@igalia.com>
Date: Thu, 8 Aug 2019 11:47:41 +0000
Subject: [PATCH 11/11] [ozone/wayland] Do not use possibly blocking dispatch
 API

Using wl_display_dispatch is a wrong thing to do as it is a blocking
method (see [1]). It can cause freezes and deadlocks.

Instead, we need to do something like this -

while (wl_display_prepare_read_queue(display, queue) != 0)
        wl_display_dispatch_queue_pending(display, queue);
wl_display_flush(display);

ret = poll(fds, nfds, -1);
if (has_error(ret))
        wl_display_cancel_read(display);
else
        wl_display_read_events(display);

wl_display_dispatch_queue_pending(display, queue);

However, there is no need to do a second poll as long
as we are notified by libevent that the display fd is
non-blocking. However, to ensure we can read, we have
to prepare the queue, which tells other clients we
are going to read from the queue. If prepare fails,
it means there is nothing to read. Thus, we push
the events into the queue and return early instead.

We have also to add an "automatic" flush as part
of the polling mechanism. This poll must ensure
that all the data has been written to the queue.
If not, -1 is returned and EAGAIN is set. In this
case, we have to stop watching the fd for read
access and start watching it for the write access.

As soon as all the data is written, we can get
back to reading.

[1] https://people.freedesktop.org/~whot/wayland-doxygen/wayland/Client/classwl__display.html#a30a9c4f020f3e77581c7a81ecdb4913d

Bug: 987821
Change-Id: I536513034c18a47da079a2b56a75d32f20f72ad6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1730159
Reviewed-by: Robert Kroeger <rjkroege@chromium.org>
Commit-Queue: Maksim Sisov <msisov@igalia.com>
Cr-Commit-Position: refs/heads/master@{#685148}
---
 .../wayland/host/wayland_connection.cc        | 74 +++++++++++++++----
 .../wayland/host/wayland_connection.h         |  4 +
 .../wayland/host/wayland_data_device.cc       |  2 +-
 .../wayland/host/wayland_data_source.cc       |  2 +-
 .../platform/wayland/host/wayland_keyboard.cc |  2 +-
 .../platform/wayland/host/wayland_window.cc   |  3 +-
 .../wayland/host/wayland_window_unittest.cc   | 17 +++--
 .../wayland/host/xdg_surface_wrapper_v5.cc    |  1 +
 .../wayland/host/xdg_surface_wrapper_v6.cc    |  3 +-
 9 files changed, 84 insertions(+), 24 deletions(-)

diff --git a/ui/ozone/platform/wayland/host/wayland_connection.cc b/ui/ozone/platform/wayland/host/wayland_connection.cc
index bf6d12717861..203db09c7f15 100644
--- a/ui/ozone/platform/wayland/host/wayland_connection.cc
+++ b/ui/ozone/platform/wayland/host/wayland_connection.cc
@@ -101,20 +101,29 @@ bool WaylandConnection::StartProcessingEvents() {
     return true;
 
   DCHECK(display_);
+
+  MaybePrepareReadQueue();
+
+  // Displatch event from display to server.
   wl_display_flush(display_.get());
 
-  DCHECK(base::MessageLoopCurrentForUI::IsSet());
-  if (!base::MessageLoopCurrentForUI::Get()->WatchFileDescriptor(
-          wl_display_get_fd(display_.get()), true,
-          base::MessagePumpLibevent::WATCH_READ, &controller_, this))
-    return false;
+  return BeginWatchingFd(base::MessagePumpLibevent::WATCH_READ);
+}
 
-  watching_ = true;
-  return true;
+void WaylandConnection::MaybePrepareReadQueue() {
+  if (prepared_)
+    return;
+
+  if (wl_display_prepare_read(display()) != -1) {
+    prepared_ = true;
+    return;
+  }
+  // Nothing to read, send events to the queue.
+  wl_display_dispatch_pending(display());
 }
 
 void WaylandConnection::ScheduleFlush() {
-  if (scheduled_flush_ || !watching_)
+  if (scheduled_flush_)
     return;
   DCHECK(base::MessageLoopCurrentForUI::IsSet());
   base::ThreadTaskRunnerHandle::Get()->PostTask(
@@ -207,13 +216,38 @@ void WaylandConnection::DispatchUiEvent(Event* event) {
 }
 
 void WaylandConnection::OnFileCanReadWithoutBlocking(int fd) {
-  wl_display_dispatch(display_.get());
-  std::vector<WaylandWindow*> windows = wayland_window_manager_.GetAllWindows();
-  for (auto* window : windows)
-    window->ApplyPendingBounds();
+  if (prepared_) {
+    prepared_ = false;
+    if (wl_display_read_events(display()) == -1)
+      return;
+    wl_display_dispatch_pending(display());
+  }
+
+  MaybePrepareReadQueue();
+
+  if (!prepared_)
+    return;
+
+  // Automatic Flush.
+  int ret = wl_display_flush(display_.get());
+  if (ret != -1 || errno != EAGAIN)
+    return;
+
+  // if all data could not be written, errno will be set to EAGAIN and -1
+  // returned. In that case, use poll on the display file descriptor to wait for
+  // it to become writable again.
+  BeginWatchingFd(base::MessagePumpLibevent::WATCH_WRITE);
 }
 
-void WaylandConnection::OnFileCanWriteWithoutBlocking(int fd) {}
+void WaylandConnection::OnFileCanWriteWithoutBlocking(int fd) {
+  int ret = wl_display_flush(display_.get());
+  if (ret != -1 || errno != EAGAIN)
+    BeginWatchingFd(base::MessagePumpLibevent::WATCH_READ);
+  else if (ret < 0 && errno != EPIPE && prepared_)
+    wl_display_cancel_read(display());
+
+  // Otherwise just continue watching in the same mode.
+}
 
 void WaylandConnection::EnsureDataDevice() {
   if (!data_device_manager_ || !seat_)
@@ -225,6 +259,20 @@ void WaylandConnection::EnsureDataDevice() {
                                                   data_device_.get());
 }
 
+bool WaylandConnection::BeginWatchingFd(
+    base::WatchableIOMessagePumpPosix::Mode mode) {
+  if (watching_) {
+    // Stop watching first.
+    watching_ = !controller_.StopWatchingFileDescriptor();
+    DCHECK(!watching_);
+  }
+
+  DCHECK(base::MessageLoopCurrentForUI::IsSet());
+  watching_ = base::MessageLoopCurrentForUI::Get()->WatchFileDescriptor(
+      wl_display_get_fd(display_.get()), true, mode, &controller_, this);
+  return watching_;
+}
+
 // static
 void WaylandConnection::Global(void* data,
                                wl_registry* registry,
diff --git a/ui/ozone/platform/wayland/host/wayland_connection.h b/ui/ozone/platform/wayland/host/wayland_connection.h
index 8a56ebc6b921..ff587ae9ee6f 100644
--- a/ui/ozone/platform/wayland/host/wayland_connection.h
+++ b/ui/ozone/platform/wayland/host/wayland_connection.h
@@ -146,6 +146,9 @@ class WaylandConnection : public PlatformEventSource,
   // Make sure data device is properly initialized
   void EnsureDataDevice();
 
+  bool BeginWatchingFd(base::WatchableIOMessagePumpPosix::Mode mode);
+  void MaybePrepareReadQueue();
+
   // wl_registry_listener
   static void Global(void* data,
                      wl_registry* registry,
@@ -192,6 +195,7 @@ class WaylandConnection : public PlatformEventSource,
 
   bool scheduled_flush_ = false;
   bool watching_ = false;
+  bool prepared_ = false;
   base::MessagePumpLibevent::FdWatchController controller_;
 
   uint32_t serial_ = 0;
diff --git a/ui/ozone/platform/wayland/host/wayland_data_device.cc b/ui/ozone/platform/wayland/host/wayland_data_device.cc
index a36cd3b7a888..56e568fbd164 100644
--- a/ui/ozone/platform/wayland/host/wayland_data_device.cc
+++ b/ui/ozone/platform/wayland/host/wayland_data_device.cc
@@ -355,7 +355,7 @@ void WaylandDataDevice::RegisterDeferredReadCallback() {
   deferred_read_callback_.reset(wl_display_sync(connection_->display()));
   wl_callback_add_listener(deferred_read_callback_.get(),
                            &kDeferredReadListener, this);
-  wl_display_flush(connection_->display());
+  connection_->ScheduleFlush();
 }
 
 void WaylandDataDevice::DeferredReadCallback(void* data,
diff --git a/ui/ozone/platform/wayland/host/wayland_data_source.cc b/ui/ozone/platform/wayland/host/wayland_data_source.cc
index dcd6b75aa299..1f41fd6786b9 100644
--- a/ui/ozone/platform/wayland/host/wayland_data_source.cc
+++ b/ui/ozone/platform/wayland/host/wayland_data_source.cc
@@ -36,7 +36,7 @@ void WaylandDataSource::WriteToClipboard(
   wl_data_device_set_selection(connection_->data_device(), data_source_.get(),
                                connection_->serial());
 
-  wl_display_flush(connection_->display());
+  connection_->ScheduleFlush();
 }
 
 void WaylandDataSource::UpdateDataMap(
diff --git a/ui/ozone/platform/wayland/host/wayland_keyboard.cc b/ui/ozone/platform/wayland/host/wayland_keyboard.cc
index 181e6e56b4b6..69cd9686bdf8 100644
--- a/ui/ozone/platform/wayland/host/wayland_keyboard.cc
+++ b/ui/ozone/platform/wayland/host/wayland_keyboard.cc
@@ -165,7 +165,7 @@ void WaylandKeyboard::FlushInput(base::OnceClosure closure) {
   // get spurious repeats.
   sync_callback_.reset(wl_display_sync(connection_->display()));
   wl_callback_add_listener(sync_callback_.get(), &callback_listener_, this);
-  wl_display_flush(connection_->display());
+  connection_->ScheduleFlush();
 }
 
 void WaylandKeyboard::DispatchKey(uint32_t key,
diff --git a/ui/ozone/platform/wayland/host/wayland_window.cc b/ui/ozone/platform/wayland/host/wayland_window.cc
index 12b3661985a6..cf0f3c336414 100644
--- a/ui/ozone/platform/wayland/host/wayland_window.cc
+++ b/ui/ozone/platform/wayland/host/wayland_window.cc
@@ -285,7 +285,6 @@ void WaylandWindow::ApplyPendingBounds() {
 
   SetBoundsDip(pending_bounds_dip_);
   xdg_surface_->SetWindowGeometry(pending_bounds_dip_);
-  xdg_surface_->AckConfigure();
   pending_bounds_dip_ = gfx::Rect();
   connection_->ScheduleFlush();
 
@@ -659,6 +658,8 @@ void WaylandWindow::HandleSurfaceConfigure(int32_t width,
     delegate_->OnWindowStateChanged(state_);
   }
 
+  ApplyPendingBounds();
+
   if (did_active_change)
     delegate_->OnActivationChanged(is_active_);
 
diff --git a/ui/ozone/platform/wayland/host/wayland_window_unittest.cc b/ui/ozone/platform/wayland/host/wayland_window_unittest.cc
index 8083f7a84dcd..5fdf4a6184ba 100644
--- a/ui/ozone/platform/wayland/host/wayland_window_unittest.cc
+++ b/ui/ozone/platform/wayland/host/wayland_window_unittest.cc
@@ -677,19 +677,26 @@ TEST_P(WaylandWindowTest, HasCaptureUpdatedOnPointerEvents) {
 
 TEST_P(WaylandWindowTest, ConfigureEvent) {
   ScopedWlArray states;
-  SendConfigureEvent(1000, 1000, 12, states.get());
-  SendConfigureEvent(1500, 1000, 13, states.get());
 
-  // Make sure that the implementation does not call OnBoundsChanged for each
-  // configure event if it receives multiple in a row.
-  EXPECT_CALL(delegate_, OnBoundsChanged(Eq(gfx::Rect(0, 0, 1500, 1000))));
+  // The surface must react on each configure event and send bounds to its
+  // delegate.
+
+  EXPECT_CALL(delegate_, OnBoundsChanged(Eq(gfx::Rect(0, 0, 1000, 1000))));
   // Responding to a configure event, the window geometry in here must respect
   // the sizing negotiations specified by the configure event.
   // |xdg_surface_| must receive the following calls in both xdg_shell_v5 and
   // xdg_shell_v6. Other calls like SetTitle or SetMaximized are recieved by
   // xdg_toplevel in xdg_shell_v6 and by xdg_surface_ in xdg_shell_v5.
+  EXPECT_CALL(*xdg_surface_, SetWindowGeometry(0, 0, 1000, 1000)).Times(1);
+  EXPECT_CALL(*xdg_surface_, AckConfigure(12));
+  SendConfigureEvent(1000, 1000, 12, states.get());
+
+  Sync();
+
+  EXPECT_CALL(delegate_, OnBoundsChanged(Eq(gfx::Rect(0, 0, 1500, 1000))));
   EXPECT_CALL(*xdg_surface_, SetWindowGeometry(0, 0, 1500, 1000)).Times(1);
   EXPECT_CALL(*xdg_surface_, AckConfigure(13));
+  SendConfigureEvent(1500, 1000, 13, states.get());
 }
 
 TEST_P(WaylandWindowTest, ConfigureEventWithNulledSize) {
diff --git a/ui/ozone/platform/wayland/host/xdg_surface_wrapper_v5.cc b/ui/ozone/platform/wayland/host/xdg_surface_wrapper_v5.cc
index 7d4172d2f0cb..c89fa87bff9b 100644
--- a/ui/ozone/platform/wayland/host/xdg_surface_wrapper_v5.cc
+++ b/ui/ozone/platform/wayland/host/xdg_surface_wrapper_v5.cc
@@ -99,6 +99,7 @@ void XDGSurfaceWrapperV5::Configure(void* data,
   surface->pending_configure_serial_ = serial;
   surface->wayland_window_->HandleSurfaceConfigure(width, height, is_maximized,
                                                    is_fullscreen, is_activated);
+  surface->AckConfigure();
 }
 
 // static
diff --git a/ui/ozone/platform/wayland/host/xdg_surface_wrapper_v6.cc b/ui/ozone/platform/wayland/host/xdg_surface_wrapper_v6.cc
index 9cac792bf630..c7c9fc079590 100644
--- a/ui/ozone/platform/wayland/host/xdg_surface_wrapper_v6.cc
+++ b/ui/ozone/platform/wayland/host/xdg_surface_wrapper_v6.cc
@@ -123,8 +123,7 @@ void XDGSurfaceWrapperV6::Configure(void* data,
   XDGSurfaceWrapperV6* surface = static_cast<XDGSurfaceWrapperV6*>(data);
   surface->pending_configure_serial_ = serial;
 
-  if (surface->surface_for_popup_)
-    surface->AckConfigure();
+  surface->AckConfigure();
 }
 
 // static
-- 
2.22.0