path: root/0002-ozone-wayland-Use-mutex-before-accessing-surfaces-ma.patch

From 296c7b4e9935aaed1d05d32416043a03f67e268d Mon Sep 17 00:00:00 2001
From: Maksim Sisov <msisov@igalia.com>
Date: Mon, 5 Aug 2019 16:14:47 +0300
Subject: [PATCH 2/6] [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 95f20b1fb288..3dbb00bc6239 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 72d2419b5f84..48f2a7f655bd 100644
--- a/ui/ozone/platform/wayland/gpu/gbm_pixmap_wayland.cc
+++ b/ui/ozone/platform/wayland/gpu/gbm_pixmap_wayland.cc
@@ -135,10 +135,10 @@ bool GbmPixmapWayland::ScheduleOverlayPlane(
     const gfx::RectF& crop_rect,
     bool enable_blend,
     std::unique_ptr<gfx::GpuFence> gpu_fence) {
-  auto* surface = buffer_manager_->GetSurface(widget);
-  DCHECK(surface);
+  auto surface_weak = buffer_manager_->GetSurface(widget);
+  DCHECK(surface_weak);
   GbmSurfacelessWayland* surfaceless =
-      static_cast<GbmSurfacelessWayland*>(surface);
+      static_cast<GbmSurfacelessWayland*>(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<PendingFrame>());
 }
 
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<WaylandSurfaceGpu> 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<WaylandSurfaceGpu> 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 <memory>
 
 #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<WaylandSurfaceGpu> 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<WaylandSurfaceGpu> 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<ozone::mojom::WaylandBufferManagerGpu>
       associated_binding_;
 
-  std::map<gfx::AcceleratedWidget, WaylandSurfaceGpu*> widget_to_surface_map_;
+  std::map<gfx::AcceleratedWidget, base::WeakPtr<WaylandSurfaceGpu>>
+      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<base::SingleThreadTaskRunner> 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 231d590296d9..d1e244e910c6 100644
--- a/ui/ozone/platform/wayland/gpu/wayland_canvas_surface.cc
+++ b/ui/ozone/platform/wayland/gpu/wayland_canvas_surface.cc
@@ -147,7 +147,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> 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 <memory>
 
 #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<WaylandSurfaceGpu> GetWeakPtr();
+
+ private:
+  base::WeakPtrFactory<WaylandSurfaceGpu> weak_factory_{this};
+
+  DISALLOW_COPY_AND_ASSIGN(WaylandSurfaceGpu);
 };
 
 }  // namespace ui
-- 
2.23.0