Skip to content

Commit

Permalink
[lacros] Crash when attaching/detaching tabs fast
Browse files Browse the repository at this point in the history
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 <nickdiego@igalia.com>
Commit-Queue: Antonio Gomes <tonikitoo@igalia.com>
Cr-Commit-Position: refs/heads/main@{#980254}
  • Loading branch information
tonikitoo authored and Chromium LUCI CQ committed Mar 12, 2022
1 parent c006e9a commit ad1e8d2
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 4 deletions.
16 changes: 14 additions & 2 deletions ui/ozone/platform/wayland/host/xdg_surface_wrapper_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,20 @@ void XDGSurfaceWrapperImpl::Configure(void* data,
auto* surface = static_cast<XDGSurfaceWrapperImpl*>(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 {
Expand Down
9 changes: 7 additions & 2 deletions ui/ozone/platform/wayland/host/xdg_surface_wrapper_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

#include <cstdint>

#include "base/memory/weak_ptr.h"
#include "ui/ozone/platform/wayland/common/wayland_object.h"

namespace gfx {
Expand All @@ -35,21 +36,25 @@ 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_;

bool is_configured_ = false;

wl::Object<struct xdg_surface> xdg_surface_;

base::WeakPtrFactory<XDGSurfaceWrapperImpl> weak_ptr_factory_{this};
};

} // namespace ui
Expand Down

0 comments on commit ad1e8d2

Please sign in to comment.