From fd1d8bfd55d5b1d073cbad3ebe8f058c49f3702a Mon Sep 17 00:00:00 2001 From: Maksim Sisov Date: Mon, 5 Aug 2019 16:14:47 +0300 Subject: [PATCH 5/5] [ozone/wayland] Use mutex before accessing surfaces map. We must make sure that accessing surfaces map from different threads is safe. Otherwise, this is subject to races and unexpected crashes. Bug: 987950 Change-Id: I2e70e9c1ad48943be518c3571b7ca1fb91f8d51b --- ui/ozone/platform/wayland/BUILD.gn | 1 + .../wayland/gpu/gbm_pixmap_wayland.cc | 6 +- .../wayland/gpu/gbm_surfaceless_wayland.cc | 2 +- .../wayland/gpu/wayland_buffer_manager_gpu.cc | 61 ++++++++----------- .../wayland/gpu/wayland_buffer_manager_gpu.h | 14 ++++- .../wayland/gpu/wayland_canvas_surface.cc | 2 +- .../wayland/gpu/wayland_surface_gpu.cc | 16 +++++ .../wayland/gpu/wayland_surface_gpu.h | 13 +++- 8 files changed, 72 insertions(+), 43 deletions(-) create mode 100644 ui/ozone/platform/wayland/gpu/wayland_surface_gpu.cc diff --git a/ui/ozone/platform/wayland/BUILD.gn b/ui/ozone/platform/wayland/BUILD.gn index 686138bb614a..a1560a20fe23 100644 --- a/ui/ozone/platform/wayland/BUILD.gn +++ b/ui/ozone/platform/wayland/BUILD.gn @@ -30,6 +30,7 @@ source_set("wayland") { "gpu/wayland_canvas_surface.h", "gpu/wayland_surface_factory.cc", "gpu/wayland_surface_factory.h", + "gpu/wayland_surface_gpu.cc", "gpu/wayland_surface_gpu.h", "host/wayland_buffer_manager_connector.cc", "host/wayland_buffer_manager_connector.h", diff --git a/ui/ozone/platform/wayland/gpu/gbm_pixmap_wayland.cc b/ui/ozone/platform/wayland/gpu/gbm_pixmap_wayland.cc index 5d8167bdfd89..917f849811a2 100644 --- a/ui/ozone/platform/wayland/gpu/gbm_pixmap_wayland.cc +++ b/ui/ozone/platform/wayland/gpu/gbm_pixmap_wayland.cc @@ -126,10 +126,10 @@ bool GbmPixmapWayland::ScheduleOverlayPlane( const gfx::RectF& crop_rect, bool enable_blend, std::unique_ptr gpu_fence) { - auto* surface = buffer_manager_->GetSurface(widget); - DCHECK(surface); + auto surface_weak = buffer_manager_->GetSurface(widget); + DCHECK(surface_weak); GbmSurfacelessWayland* surfaceless = - static_cast(surface); + static_cast(surface_weak.get()); DCHECK(surfaceless); surfaceless->QueueOverlayPlane( diff --git a/ui/ozone/platform/wayland/gpu/gbm_surfaceless_wayland.cc b/ui/ozone/platform/wayland/gpu/gbm_surfaceless_wayland.cc index 70ea08acfa11..0554e772f5cb 100644 --- a/ui/ozone/platform/wayland/gpu/gbm_surfaceless_wayland.cc +++ b/ui/ozone/platform/wayland/gpu/gbm_surfaceless_wayland.cc @@ -34,7 +34,7 @@ GbmSurfacelessWayland::GbmSurfacelessWayland( has_implicit_external_sync_( HasEGLExtension("EGL_ARM_implicit_external_sync")), weak_factory_(this) { - buffer_manager_->RegisterSurface(widget_, this); + buffer_manager_->RegisterSurface(widget_, GetWeakPtr()); unsubmitted_frames_.push_back(std::make_unique()); } diff --git a/ui/ozone/platform/wayland/gpu/wayland_buffer_manager_gpu.cc b/ui/ozone/platform/wayland/gpu/wayland_buffer_manager_gpu.cc index 63bfa3032fde..f113a9887bdc 100644 --- a/ui/ozone/platform/wayland/gpu/wayland_buffer_manager_gpu.cc +++ b/ui/ozone/platform/wayland/gpu/wayland_buffer_manager_gpu.cc @@ -43,19 +43,13 @@ void WaylandBufferManagerGpu::OnSubmission(gfx::AcceleratedWidget widget, gfx::SwapResult swap_result) { DCHECK(io_thread_runner_->BelongsToCurrentThread()); DCHECK_NE(widget, gfx::kNullAcceleratedWidget); - auto* surface = GetSurface(widget); - // There can be a race between destruction and submitting the last frames. The - // surface can be destroyed by the time the host receives a request to destroy - // a buffer, and is able to call the OnSubmission for that specific buffer. - if (surface) { - // As long as mojo calls rerouted to the IO child thread, we have to reroute - // them back to the same thread, where the original commit buffer call came - // from. - commit_thread_runner_->PostTask( - FROM_HERE, - base::Bind(&WaylandSurfaceGpu::OnSubmission, base::Unretained(surface), - buffer_id, swap_result)); - } + auto surface = GetSurface(widget); + // As long as mojo calls rerouted to the IO child thread, we have to reroute + // them back to the same thread, where the original commit buffer call came + // from. + commit_thread_runner_->PostTask( + FROM_HERE, base::Bind(&WaylandSurfaceGpu::OnSubmission, surface, + buffer_id, swap_result)); } void WaylandBufferManagerGpu::OnPresentation( @@ -64,36 +58,35 @@ void WaylandBufferManagerGpu::OnPresentation( const gfx::PresentationFeedback& feedback) { DCHECK(io_thread_runner_->BelongsToCurrentThread()); DCHECK_NE(widget, gfx::kNullAcceleratedWidget); - auto* surface = GetSurface(widget); - // There can be a race between destruction and presenting the last frames. The - // surface can be destroyed by the time the host receives a request to destroy - // a buffer, and is able to call the OnPresentation for that specific buffer. - if (surface) { - // As long as mojo calls rerouted to the IO child thread, we have to reroute - // them back to the same thread, where the original commit buffer call came - // from. - commit_thread_runner_->PostTask( - FROM_HERE, base::Bind(&WaylandSurfaceGpu::OnPresentation, - base::Unretained(surface), buffer_id, feedback)); - } -} - -void WaylandBufferManagerGpu::RegisterSurface(gfx::AcceleratedWidget widget, - WaylandSurfaceGpu* surface) { + auto surface = GetSurface(widget); + // As long as mojo calls rerouted to the IO child thread, we have to reroute + // them back to the same thread, where the original commit buffer call came + // from. + commit_thread_runner_->PostTask( + FROM_HERE, base::Bind(&WaylandSurfaceGpu::OnPresentation, surface, + buffer_id, feedback)); +} + +void WaylandBufferManagerGpu::RegisterSurface( + gfx::AcceleratedWidget widget, + base::WeakPtr surface) { + base::AutoLock scoped_lock(lock_); widget_to_surface_map_.insert(std::make_pair(widget, surface)); } void WaylandBufferManagerGpu::UnregisterSurface(gfx::AcceleratedWidget widget) { + base::AutoLock scoped_lock(lock_); widget_to_surface_map_.erase(widget); } -WaylandSurfaceGpu* WaylandBufferManagerGpu::GetSurface( - gfx::AcceleratedWidget widget) const { - WaylandSurfaceGpu* surface = nullptr; +base::WeakPtr WaylandBufferManagerGpu::GetSurface( + gfx::AcceleratedWidget widget) { + base::AutoLock scoped_lock(lock_); + auto it = widget_to_surface_map_.find(widget); if (it != widget_to_surface_map_.end()) - surface = it->second; - return surface; + return it->second; + return nullptr; } void WaylandBufferManagerGpu::CreateDmabufBasedBuffer( diff --git a/ui/ozone/platform/wayland/gpu/wayland_buffer_manager_gpu.h b/ui/ozone/platform/wayland/gpu/wayland_buffer_manager_gpu.h index 87439610cfc3..631d715e719e 100644 --- a/ui/ozone/platform/wayland/gpu/wayland_buffer_manager_gpu.h +++ b/ui/ozone/platform/wayland/gpu/wayland_buffer_manager_gpu.h @@ -8,6 +8,8 @@ #include #include "base/macros.h" +#include "base/memory/weak_ptr.h" +#include "base/synchronization/lock.h" #include "base/threading/sequenced_task_runner_handle.h" #include "base/threading/thread_checker.h" #include "mojo/public/cpp/bindings/associated_binding.h" @@ -64,9 +66,11 @@ class WaylandBufferManagerGpu : public ozone::mojom::WaylandBufferManagerGpu { // result of the below operations, they must register themselves with the // below APIs. void RegisterSurface(gfx::AcceleratedWidget widget, - WaylandSurfaceGpu* surface); + base::WeakPtr surface); void UnregisterSurface(gfx::AcceleratedWidget widget); - WaylandSurfaceGpu* GetSurface(gfx::AcceleratedWidget widget) const; + // A client of this method must always get and acquire lock before doing + // any manipulations with the surface. Once done, the lock must be released. + base::WeakPtr GetSurface(gfx::AcceleratedWidget widget); // Methods, which can be used when in both in-process-gpu and out of process // modes. These calls are forwarded to the browser process through the @@ -156,7 +160,8 @@ class WaylandBufferManagerGpu : public ozone::mojom::WaylandBufferManagerGpu { mojo::AssociatedBinding associated_binding_; - std::map widget_to_surface_map_; + std::map> + widget_to_surface_map_; // Guarded by |lock_|. // This task runner can be used to pass messages back to the same thread, // where the commit buffer request came from. For example, swap requests come @@ -171,6 +176,9 @@ class WaylandBufferManagerGpu : public ozone::mojom::WaylandBufferManagerGpu { // needed to ensure mojo calls happen on a right sequence. scoped_refptr io_thread_runner_; + // Protects access to |widget_to_surface_map_|. + base::Lock lock_; + DISALLOW_COPY_AND_ASSIGN(WaylandBufferManagerGpu); }; diff --git a/ui/ozone/platform/wayland/gpu/wayland_canvas_surface.cc b/ui/ozone/platform/wayland/gpu/wayland_canvas_surface.cc index 6de24d7fd177..ce44ebac2915 100644 --- a/ui/ozone/platform/wayland/gpu/wayland_canvas_surface.cc +++ b/ui/ozone/platform/wayland/gpu/wayland_canvas_surface.cc @@ -29,7 +29,7 @@ WaylandCanvasSurface::WaylandCanvasSurface( WaylandBufferManagerGpu* buffer_manager, gfx::AcceleratedWidget widget) : buffer_manager_(buffer_manager), widget_(widget) { - buffer_manager_->RegisterSurface(widget_, this); + buffer_manager_->RegisterSurface(widget_, GetWeakPtr()); } WaylandCanvasSurface::~WaylandCanvasSurface() { diff --git a/ui/ozone/platform/wayland/gpu/wayland_surface_gpu.cc b/ui/ozone/platform/wayland/gpu/wayland_surface_gpu.cc new file mode 100644 index 000000000000..85296edcca42 --- /dev/null +++ b/ui/ozone/platform/wayland/gpu/wayland_surface_gpu.cc @@ -0,0 +1,16 @@ +// Copyright 2019 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "ui/ozone/platform/wayland/gpu/wayland_surface_gpu.h" + +namespace ui { + +WaylandSurfaceGpu::WaylandSurfaceGpu() = default; +WaylandSurfaceGpu::~WaylandSurfaceGpu() = default; + +base::WeakPtr WaylandSurfaceGpu::GetWeakPtr() { + return weak_factory_.GetWeakPtr(); +} + +} // namespace ui diff --git a/ui/ozone/platform/wayland/gpu/wayland_surface_gpu.h b/ui/ozone/platform/wayland/gpu/wayland_surface_gpu.h index ace5279e838e..38d285317ab3 100644 --- a/ui/ozone/platform/wayland/gpu/wayland_surface_gpu.h +++ b/ui/ozone/platform/wayland/gpu/wayland_surface_gpu.h @@ -8,6 +8,7 @@ #include #include "base/macros.h" +#include "base/memory/weak_ptr.h" namespace gfx { enum class SwapResult; @@ -22,7 +23,7 @@ namespace ui { // the buffer. class WaylandSurfaceGpu { public: - virtual ~WaylandSurfaceGpu() {} + virtual ~WaylandSurfaceGpu(); // Tells the surface the result of the last swap of buffer with the // |buffer_id|. @@ -33,6 +34,16 @@ class WaylandSurfaceGpu { // |buffer_id|. virtual void OnPresentation(uint32_t buffer_id, const gfx::PresentationFeedback& feedback) = 0; + + protected: + WaylandSurfaceGpu(); + + base::WeakPtr GetWeakPtr(); + + private: + base::WeakPtrFactory weak_factory_{this}; + + DISALLOW_COPY_AND_ASSIGN(WaylandSurfaceGpu); }; } // namespace ui -- 2.22.0