From ad1e8d2476e3745fc1c3f7b64e3e3f22e580715f Mon Sep 17 00:00:00 2001 From: Antonio Gomes Date: Sat, 12 Mar 2022 02:39:48 +0000 Subject: [PATCH] [lacros] Crash when attaching/detaching tabs fast During the tab attaching process, XDGSurfaceWrapperImpl::Configure() can be called and result in a WaylandToplevelWindow::Hide() calls, which actually deletes itself (see pseudo stacktrace below). > #2 0x7f369324c07e ui::WaylandToplevelWindow::Hide() > #3 0x7f36936bfbfa views::DesktopWindowTreeHostPlatform::HideImpl() > #4 0x7f3694691720 aura::WindowTreeHost::Hide() > #5 0x7f36936be274 views::DesktopWindowTreeHostPlatform::Close() > #6 0x7f3693690bcf views::Widget::CloseWithReason() > #7 0x55ab161b4382 Browser::TabStripEmpty() > #8 0x55ab1620e6d0 TabStripModel::SendDetachWebContentsNotifications() > (...) > #21 0x7f369324ef38 ui::WaylandWindow::SetBounds() > #22 0x7f369325141c ui::WaylandWindow::ApplyPendingBounds() > #23 0x7f369324bd47 ui::WaylandToplevelWindow::ApplyPendingBounds() > #24 0x7f3693250f20 ui::WaylandWindow::ProcessPendingBoundsDip() > #25 0x7f369324c932 ui::WaylandToplevelWindow::HandleSurfaceConfigure() > #26 0x7f369325c263 ui::XDGSurfaceWrapperImpl::OnConfigure() > #27 0x7f369325c110 ui::XDGSurfaceWrapperImpl::Configure() > #28 0x7f36932739c5 ffi_call_unix64 To protect against such scenarios, which can hard crash both Lacros and Ash, this CL guards XDGSurfaceWrapperImpl with the WeakPtrFactory pattern, particularly within the context of XDGSurfaceWrapperImpl::OnConfigure(). BUG=1303425 R=nickdiego@igalia.com Change-Id: Ic84b3d5cf2ac11b30a82090f49c99a991e4dd243 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3519638 Reviewed-by: Nick Yamane Commit-Queue: Antonio Gomes Cr-Commit-Position: refs/heads/main@{#980254} --- .../wayland/host/xdg_surface_wrapper_impl.cc | 16 ++++++++++++++-- .../wayland/host/xdg_surface_wrapper_impl.h | 9 +++++++-- 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/ui/ozone/platform/wayland/host/xdg_surface_wrapper_impl.cc b/ui/ozone/platform/wayland/host/xdg_surface_wrapper_impl.cc index 3116311fb6f97e..a82227dfe52d86 100644 --- a/ui/ozone/platform/wayland/host/xdg_surface_wrapper_impl.cc +++ b/ui/ozone/platform/wayland/host/xdg_surface_wrapper_impl.cc @@ -70,8 +70,20 @@ void XDGSurfaceWrapperImpl::Configure(void* data, auto* surface = static_cast(data); DCHECK(surface); - surface->wayland_window_->HandleSurfaceConfigure(serial); - surface->wayland_window_->OnSurfaceConfigureEvent(); + surface->OnConfigure(serial); +} + +void XDGSurfaceWrapperImpl::OnConfigure(uint32_t serial) { + // Calls to HandleSurfaceConfigure() might end up hiding the enclosing + // toplevel window, and deleting this object. + auto alive = weak_ptr_factory_.GetWeakPtr(); + + wayland_window_->HandleSurfaceConfigure(serial); + + if (!alive) + return; + + wayland_window_->OnSurfaceConfigureEvent(); } xdg_surface* XDGSurfaceWrapperImpl::xdg_surface() const { diff --git a/ui/ozone/platform/wayland/host/xdg_surface_wrapper_impl.h b/ui/ozone/platform/wayland/host/xdg_surface_wrapper_impl.h index 2571aac25d5422..d90d2fd032f3aa 100644 --- a/ui/ozone/platform/wayland/host/xdg_surface_wrapper_impl.h +++ b/ui/ozone/platform/wayland/host/xdg_surface_wrapper_impl.h @@ -9,6 +9,7 @@ #include +#include "base/memory/weak_ptr.h" #include "ui/ozone/platform/wayland/common/wayland_object.h" namespace gfx { @@ -35,14 +36,16 @@ class XDGSurfaceWrapperImpl : public ShellSurfaceWrapper { bool IsConfigured() override; void SetWindowGeometry(const gfx::Rect& bounds) override; + struct xdg_surface* xdg_surface() const; + + private: // xdg_surface_listener static void Configure(void* data, struct xdg_surface* xdg_surface, uint32_t serial); - struct xdg_surface* xdg_surface() const; + void OnConfigure(uint32_t serial); - private: // Non-owing WaylandWindow that uses this surface wrapper. WaylandWindow* const wayland_window_; WaylandConnection* const connection_; @@ -50,6 +53,8 @@ class XDGSurfaceWrapperImpl : public ShellSurfaceWrapper { bool is_configured_ = false; wl::Object xdg_surface_; + + base::WeakPtrFactory weak_ptr_factory_{this}; }; } // namespace ui