From ae8d566f4048db9fff5208b5f007116eee92f289 Mon Sep 17 00:00:00 2001 From: Maksim Sisov Date: Thu, 8 Aug 2019 11:47:41 +0000 Subject: [PATCH 5/6] [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 Commit-Queue: Maksim Sisov 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 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 c082b30133e5..fbadb0e3b1e2 100644 --- a/ui/ozone/platform/wayland/host/wayland_keyboard.cc +++ b/ui/ozone/platform/wayland/host/wayland_keyboard.cc @@ -167,7 +167,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 64364a3064f3..a03dde334c19 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(); @@ -662,6 +661,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 699d3cebe703..747b7dd60baf 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(data); surface->pending_configure_serial_ = serial; - if (surface->surface_for_popup_) - surface->AckConfigure(); + surface->AckConfigure(); } // static -- 2.23.0