ozone/wayland: Fix possible race with surfaces and buffers.

There is a possible race that results in a terminated gpu process.

That is, a surface may have been destroyed right before a request
that creates or commits buffers. To fix that, always create
buffers anonymously aka not attached to any surfaces, and simply
remove the pointer that the WaylandBufferManagerHost::Surface has
to the WaylandWindow so that it does not try to do anything except
the destruction of buffers, the call for which can come later
from the GPU process.

Bug: 1006813
Change-Id: I9f207ec8dac92b94a1e8f8b9fc9cd3fdb0186580
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1924247Reviewed-by: Robert Kroeger <rjkroege@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Maksim Sisov <msisov@igalia.com>
Cr-Commit-Position: refs/heads/master@{#718271}

ui/ozone/platform/wayland/gpu/gbm_pixmap_wayland.cc

@@ -203,7 +203,7 @@ void GbmPixmapWayland::CreateDmabufBasedBuffer() {
   }
   // Asks Wayland to create a wl_buffer based on the |file| fd.
   buffer_manager_->CreateDmabufBasedBuffer(
-      widget_, std::move(fd), GetBufferSize(), strides, offsets, modifiers,
+      std::move(fd), GetBufferSize(), strides, offsets, modifiers,
       gbm_bo_->GetFormat(), plane_count, GetUniqueId());
 }
 

ui/ozone/platform/wayland/gpu/wayland_buffer_manager_gpu.cc

@@ -81,7 +81,6 @@ WaylandSurfaceGpu* WaylandBufferManagerGpu::GetSurface(
 }
 
 void WaylandBufferManagerGpu::CreateDmabufBasedBuffer(
-    gfx::AcceleratedWidget widget,
     base::ScopedFD dmabuf_fd,
     gfx::Size size,
     const std::vector<uint32_t>& strides,
@@ -96,14 +95,13 @@ void WaylandBufferManagerGpu::CreateDmabufBasedBuffer(
   io_thread_runner_->PostTask(
       FROM_HERE,
       base::BindOnce(&WaylandBufferManagerGpu::CreateDmabufBasedBufferInternal,
-                     base::Unretained(this), widget, std::move(dmabuf_fd),
+                     base::Unretained(this), std::move(dmabuf_fd),
                      std::move(size), std::move(strides), std::move(offsets),
                      std::move(modifiers), current_format, planes_count,
                      buffer_id));
 }
 
 void WaylandBufferManagerGpu::CreateShmBasedBuffer(
-    gfx::AcceleratedWidget widget,
     base::ScopedFD shm_fd,
     size_t length,
     gfx::Size size,
@@ -114,7 +112,7 @@ void WaylandBufferManagerGpu::CreateShmBasedBuffer(
   io_thread_runner_->PostTask(
       FROM_HERE,
       base::BindOnce(&WaylandBufferManagerGpu::CreateShmBasedBufferInternal,
-                     base::Unretained(this), widget, std::move(shm_fd), length,
+                     base::Unretained(this), std::move(shm_fd), length,
                      std::move(size), buffer_id));
 }
 
@@ -160,7 +158,6 @@ WaylandBufferManagerGpu::GetModifiersForBufferFormat(
 }
 
 void WaylandBufferManagerGpu::CreateDmabufBasedBufferInternal(
-    gfx::AcceleratedWidget widget,
     base::ScopedFD dmabuf_fd,
     gfx::Size size,
     const std::vector<uint32_t>& strides,
@@ -172,14 +169,12 @@ void WaylandBufferManagerGpu::CreateDmabufBasedBufferInternal(
   DCHECK(io_thread_runner_->BelongsToCurrentThread());
   DCHECK(remote_host_);
   remote_host_->CreateDmabufBasedBuffer(
-      widget,
       mojo::WrapPlatformHandle(mojo::PlatformHandle(std::move(dmabuf_fd))),
       size, strides, offsets, modifiers, current_format, planes_count,
       buffer_id);
 }
 
 void WaylandBufferManagerGpu::CreateShmBasedBufferInternal(
-    gfx::AcceleratedWidget widget,
     base::ScopedFD shm_fd,
     size_t length,
     gfx::Size size,
@@ -187,8 +182,8 @@ void WaylandBufferManagerGpu::CreateShmBasedBufferInternal(
   DCHECK(io_thread_runner_->BelongsToCurrentThread());
   DCHECK(remote_host_);
   remote_host_->CreateShmBasedBuffer(
-      widget, mojo::WrapPlatformHandle(mojo::PlatformHandle(std::move(shm_fd))),
-      length, size, buffer_id);
+      mojo::WrapPlatformHandle(mojo::PlatformHandle(std::move(shm_fd))), length,
+      size, buffer_id);
 }
 
 void WaylandBufferManagerGpu::CommitBufferInternal(

ui/ozone/platform/wayland/gpu/wayland_buffer_manager_gpu.h

@@ -75,8 +75,7 @@ class WaylandBufferManagerGpu : public ozone::mojom::WaylandBufferManagerGpu {
   // ui/ozone/public/mojom/wayland/wayland_connection.mojom.
   //
   // Asks Wayland to create generic dmabuf-based wl_buffer.
-  void CreateDmabufBasedBuffer(gfx::AcceleratedWidget widget,
-                               base::ScopedFD dmabuf_fd,
+  void CreateDmabufBasedBuffer(base::ScopedFD dmabuf_fd,
                                gfx::Size size,
                                const std::vector<uint32_t>& strides,
                                const std::vector<uint32_t>& offsets,
@@ -86,8 +85,7 @@ class WaylandBufferManagerGpu : public ozone::mojom::WaylandBufferManagerGpu {
                                uint32_t buffer_id);
 
   // Asks Wayland to create a shared memory based wl_buffer.
-  void CreateShmBasedBuffer(gfx::AcceleratedWidget widget,
-                            base::ScopedFD shm_fd,
+  void CreateShmBasedBuffer(base::ScopedFD shm_fd,
                             size_t length,
                             gfx::Size size,
                             uint32_t buffer_id);
@@ -126,8 +124,7 @@ class WaylandBufferManagerGpu : public ozone::mojom::WaylandBufferManagerGpu {
       gfx::BufferFormat buffer_format) const;
 
  private:
-  void CreateDmabufBasedBufferInternal(gfx::AcceleratedWidget widget,
-                                       base::ScopedFD dmabuf_fd,
+  void CreateDmabufBasedBufferInternal(base::ScopedFD dmabuf_fd,
                                        gfx::Size size,
                                        const std::vector<uint32_t>& strides,
                                        const std::vector<uint32_t>& offsets,
@@ -135,8 +132,7 @@ class WaylandBufferManagerGpu : public ozone::mojom::WaylandBufferManagerGpu {
                                        uint32_t current_format,
                                        uint32_t planes_count,
                                        uint32_t buffer_id);
-  void CreateShmBasedBufferInternal(gfx::AcceleratedWidget widget,
-                                    base::ScopedFD shm_fd,
+  void CreateShmBasedBufferInternal(base::ScopedFD shm_fd,
                                     size_t length,
                                     gfx::Size size,
                                     uint32_t buffer_id);

ui/ozone/platform/wayland/gpu/wayland_canvas_surface.cc

@@ -78,9 +78,8 @@ class WaylandCanvasSurface::SharedMemoryBuffer {
         base::UnsafeSharedMemoryRegion::TakeHandleForSerialization(
             std::move(shm_region));
     base::subtle::ScopedFDPair fd_pair = platform_shm.PassPlatformHandle();
-    buffer_manager_->CreateShmBasedBuffer(widget_, std::move(fd_pair.fd),
-                                          checked_length.ValueOrDie(), size,
-                                          buffer_id_);
+    buffer_manager_->CreateShmBasedBuffer(
+        std::move(fd_pair.fd), checked_length.ValueOrDie(), size, buffer_id_);
 
     sk_surface_ = SkSurface::MakeRasterDirect(
         SkImageInfo::MakeN32Premul(size.width(), size.height()),

ui/ozone/platform/wayland/host/wayland_buffer_manager_host.h

@@ -117,8 +117,7 @@ class WaylandBufferManagerHost : public ozone::mojom::WaylandBufferManagerHost,
   // Called by the GPU and asks to import a wl_buffer based on a gbm file
   // descriptor using zwp_linux_dmabuf protocol. Check comments in the
   // ui/ozone/public/mojom/wayland/wayland_connection.mojom.
-  void CreateDmabufBasedBuffer(gfx::AcceleratedWidget widget,
-                               mojo::ScopedHandle dmabuf_fd,
+  void CreateDmabufBasedBuffer(mojo::ScopedHandle dmabuf_fd,
                                const gfx::Size& size,
                                const std::vector<uint32_t>& strides,
                                const std::vector<uint32_t>& offsets,
@@ -129,8 +128,7 @@ class WaylandBufferManagerHost : public ozone::mojom::WaylandBufferManagerHost,
   // Called by the GPU and asks to import a wl_buffer based on a shared memory
   // file descriptor using wl_shm protocol. Check comments in the
   // ui/ozone/public/mojom/wayland/wayland_connection.mojom.
-  void CreateShmBasedBuffer(gfx::AcceleratedWidget widget,
-                            mojo::ScopedHandle shm_fd,
+  void CreateShmBasedBuffer(mojo::ScopedHandle shm_fd,
                             uint64_t length,
                             const gfx::Size& size,
                             uint32_t buffer_id) override;
@@ -160,16 +158,13 @@ class WaylandBufferManagerHost : public ozone::mojom::WaylandBufferManagerHost,
   // presentation callbacks for that window's surface.
   class Surface;
 
-  bool CreateBuffer(gfx::AcceleratedWidget& widget,
-                    const gfx::Size& size,
-                    uint32_t buffer_id);
+  bool CreateBuffer(const gfx::Size& size, uint32_t buffer_id);
 
   Surface* GetSurface(gfx::AcceleratedWidget widget) const;
 
   // Validates data sent from GPU. If invalid, returns false and sets an error
   // message to |error_message_|.
-  bool ValidateDataFromGpu(const gfx::AcceleratedWidget& widget,
-                           const base::ScopedFD& file,
+  bool ValidateDataFromGpu(const base::ScopedFD& file,
                            const gfx::Size& size,
                            const std::vector<uint32_t>& strides,
                            const std::vector<uint32_t>& offsets,
@@ -177,18 +172,15 @@ class WaylandBufferManagerHost : public ozone::mojom::WaylandBufferManagerHost,
                            uint32_t format,
                            uint32_t planes_count,
                            uint32_t buffer_id);
-  bool ValidateDataFromGpu(const gfx::AcceleratedWidget& widget,
-                           uint32_t buffer_id);
-  bool ValidateDataFromGpu(const gfx::AcceleratedWidget& widget,
-                           const base::ScopedFD& file,
+  bool ValidateBufferIdFromGpu(uint32_t buffer_id);
+  bool ValidateDataFromGpu(const base::ScopedFD& file,
                            size_t length,
                            const gfx::Size& size,
                            uint32_t buffer_id);
 
   // Callback method. Receives a result for the request to create a wl_buffer
   // backend by dmabuf file descriptor from ::CreateBuffer call.
-  void OnCreateBufferComplete(gfx::AcceleratedWidget widget,
-                              uint32_t buffer_id,
+  void OnCreateBufferComplete(uint32_t buffer_id,
                               wl::Object<struct wl_buffer> new_buffer);
 
   // Tells the |buffer_manager_gpu_ptr_| the result of a swap call and provides
@@ -203,6 +195,8 @@ class WaylandBufferManagerHost : public ozone::mojom::WaylandBufferManagerHost,
   // Terminates the GPU process on invalid data received
   void TerminateGpuProcess();
 
+  bool DestroyAnonymousBuffer(uint32_t buffer_id);
+
   base::flat_map<gfx::AcceleratedWidget, std::unique_ptr<Surface>> surfaces_;
 
   // Set when invalid data is received from the GPU process.

ui/ozone/public/mojom/wayland/wayland_buffer_manager.mojom

@@ -22,18 +22,20 @@ interface WaylandBufferManagerHost {
   // The following two methods are used either for hardware accelerated
   // rendering or for the software rendering.
   //
-  // If hardware accelerated rendering path is taken, this methnod can be used
-  // to ask Wayland to create a wl_buffer based on the |dmabuf_fd| descriptor
-  // for the WaylandWindow, which has the following |widget|. The |size|
-  // is the size of the buffer, the |strides|, |offsets| and |modifiers|
-  // are the descriptions of the drm buffer object. The |format| describes
-  // the buffer format (check gfx::BufferFormat) in fourcc form. The
-  // |planes_count| says how many planes the buffer, backed by the |file|
-  // descriptor has. And the |buffer_id| is a unique id for the buffer, which
-  // is used to identify imported wl_buffers on the browser process side and
-  // map them with the buffer objects on the gpu process side.
-  CreateDmabufBasedBuffer(gfx.mojom.AcceleratedWidget widget,
-                          handle dmabuf_fd,
+  // If the hardware accelerated rendering path is taken, this method can be
+  // used to ask Wayland to create a wl_buffer based on the |dmabuf_fd|
+  // descriptor. The |size| is the size of the buffer, the |strides|,
+  // |offsets| and |modifiers| are the descriptions of the drm buffer object.
+  // The |format| describes the buffer format (check gfx::BufferFormat) in
+  // fourcc form. The |planes_count| says how many planes the buffer, backed
+  // by the |file| descriptor has. And the |buffer_id| is a unique id for the
+  // buffer, which is used to identify imported wl_buffers on the browser
+  // process side and map them with the buffer objects on the gpu process side.
+  // The buffer will be associated with an AcceleratedWidget as soon as the
+  // very first CommitBuffer request comes from viz to browser process.
+  // If the buffer has been committed at least once, it is not possible to
+  // reassign it to another AcceleratedWidget.
+  CreateDmabufBasedBuffer(handle dmabuf_fd,
                           gfx.mojom.Size size,
                           array<uint32> strides,
                           array<uint32> offsets,
@@ -46,8 +48,11 @@ interface WaylandBufferManagerHost {
   // Wayland to create a wl_buffer based on the |shm_fd| descriptor.
   // The |length| is the length of the shared memory, |size|
   // is the size of buffer and |buffer_id| is the id of the buffer.
-  CreateShmBasedBuffer(gfx.mojom.AcceleratedWidget widget,
-                       handle shm_fd,
+  // The buffer will be associated with an AcceleratedWidget as soon as the
+  // very first CommitBuffer request comes from viz to browser process. If
+  // the buffer has been committed at least once, it is not possible to
+  // reassign it to another AcceleratedWidget.
+  CreateShmBasedBuffer(handle shm_fd,
                        uint64 length,
                        gfx.mojom.Size size,
                        uint32 buffer_id);
@@ -57,11 +62,14 @@ interface WaylandBufferManagerHost {
   // Destroys a wl_buffer created by WaylandConnection based on the |buffer_id|
   // for the WaylandWindow, which has the following |widget|. The |buffer_id|
   // is the unique id of the buffer objects being destroyed on the browser
-  // process side.
+  // process side. If the buffer with |buffer_id| has never been assigned to an
+  // AcceleratedWidget, it can be destroyed by passing a null widget
+  // with a correct buffer id. Providing wrong pair of the |widget| and the
+  // |buffer_id| will result in the termination of the GPU process.
   DestroyBuffer(gfx.mojom.AcceleratedWidget widget, uint32 buffer_id);
 
   // Attaches a wl_buffer to a WaylandWindow's surface with the following
-  // |widget|. The |damage_region| describes changed the region of the buffer.
+  // |widget|. The |damage_region| describes the changed region of the buffer.
   // The |buffer_id| is a unique id for the buffer, which is used to
   // identify imported wl_buffers on the browser process side mapped with
   // the ones on the gpu process.