[Fuchsia] Don't hold ptr to ScenicSurface from ScenicOverlayView

This CL addresses the crash that occurs when ScenicSurface is
destructed before ScenicOverlayView. When that occurs raw pointer
held by ScenicOverlayView is invalid, so we should check
ScenicSurfaceFactory for a reference instead.

Bug: 1148797
Change-Id: I7972a09227e72c212254044f8e6800d50cbe470f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2536775
Commit-Queue: Emircan Uysaler <emircan@chromium.org>
Reviewed-by: Sergey Ulanov <sergeyu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#827559}

ui/ozone/platform/scenic/scenic_overlay_view.cc

@@ -8,6 +8,7 @@
 #include <lib/ui/scenic/cpp/view_token_pair.h>
 
 #include "base/fuchsia/fuchsia_logging.h"
+#include "ui/ozone/platform/scenic/scenic_surface_factory.h"
 
 namespace ui {
 
@@ -28,8 +29,10 @@ fuchsia::ui::views::ViewToken CreateViewToken(
 }  // namespace
 
 ScenicOverlayView::ScenicOverlayView(
-    scenic::SessionPtrAndListenerRequest session_and_listener_request)
+    scenic::SessionPtrAndListenerRequest session_and_listener_request,
+    ScenicSurfaceFactory* scenic_surface_factory)
     : scenic_session_(std::move(session_and_listener_request)),
+      scenic_surface_factory_(scenic_surface_factory),
       view_(&scenic_session_,
             CreateViewToken(&view_holder_token_),
             kSessionDebugName) {
@@ -40,14 +43,20 @@ ScenicOverlayView::ScenicOverlayView(
 }
 
 ScenicOverlayView::~ScenicOverlayView() {
-  if (surface_)
-    surface_->RemoveOverlayView(buffer_collection_id_);
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+
+  ScenicSurface* surface = scenic_surface_factory_->GetSurface(widget_);
+  if (surface) {
+    surface->AssertBelongsToCurrentThread();
+    surface->RemoveOverlayView(buffer_collection_id_);
+  }
 }
 
 void ScenicOverlayView::Initialize(
     fidl::InterfaceHandle<fuchsia::sysmem::BufferCollectionToken>
         collection_token) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+
   uint32_t image_pipe_id = scenic_session_.AllocResourceId();
   scenic_session_.Enqueue(
       scenic::NewCreateImagePipe2Cmd(image_pipe_id, image_pipe_.NewRequest()));
@@ -78,6 +87,8 @@ void ScenicOverlayView::Initialize(
 
 bool ScenicOverlayView::AddImages(uint32_t buffer_count,
                                   const gfx::Size& size) {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+
   fuchsia::sysmem::ImageFormat_2 image_format = {};
   image_format.coded_width = size.width();
   image_format.coded_height = size.height();
@@ -91,6 +102,8 @@ bool ScenicOverlayView::AddImages(uint32_t buffer_count,
 bool ScenicOverlayView::PresentImage(uint32_t buffer_index,
                                      std::vector<zx::event> acquire_fences,
                                      std::vector<zx::event> release_fences) {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+
   image_pipe_->PresentImage(buffer_index + 1, zx_clock_get_monotonic(),
                             std::move(acquire_fences),
                             std::move(release_fences), [](auto) {});
@@ -98,6 +111,8 @@ bool ScenicOverlayView::PresentImage(uint32_t buffer_index,
 }
 
 void ScenicOverlayView::SetBlendMode(bool enable_blend) {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+
   if (enable_blend_ == enable_blend)
     return;
 
@@ -117,10 +132,11 @@ bool ScenicOverlayView::CanAttachToAcceleratedWidget(
 }
 
 bool ScenicOverlayView::AttachToScenicSurface(
-    ScenicSurface* surface,
     gfx::AcceleratedWidget widget,
     gfx::SysmemBufferCollectionId id) {
-  if (surface_ && surface_ == surface)
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+
+  if (widget_ != gfx::kNullAcceleratedWidget && widget_ == widget)
     return true;
 
   if (!view_holder_token_.value.is_valid()) {
@@ -128,11 +144,13 @@ bool ScenicOverlayView::AttachToScenicSurface(
     return false;
   }
 
-  surface_ = surface;
   buffer_collection_id_ = id;
   widget_ = widget;
-  return surface_->PresentOverlayView(buffer_collection_id_,
-                                      std::move(view_holder_token_));
+
+  ScenicSurface* surface = scenic_surface_factory_->GetSurface(widget_);
+  DCHECK(surface);
+  return surface->PresentOverlayView(buffer_collection_id_,
+                                     std::move(view_holder_token_));
 }
 
 }  // namespace ui

ui/ozone/platform/scenic/scenic_overlay_view.h

@@ -24,7 +24,8 @@ namespace ui {
 class ScenicOverlayView {
  public:
   ScenicOverlayView(
-      scenic::SessionPtrAndListenerRequest session_and_listener_request);
+      scenic::SessionPtrAndListenerRequest session_and_listener_request,
+      ScenicSurfaceFactory* scenic_surface_factory);
   ~ScenicOverlayView();
   ScenicOverlayView(const ScenicOverlayView&) = delete;
   ScenicOverlayView& operator=(const ScenicOverlayView&) = delete;
@@ -53,21 +54,20 @@ class ScenicOverlayView {
   bool CanAttachToAcceleratedWidget(gfx::AcceleratedWidget widget);
 
   // Return true if |view_holder_token_| is attached to the scene graph of
-  // |surface|.
-  bool AttachToScenicSurface(ScenicSurface* surface,
-                             gfx::AcceleratedWidget widget,
+  // surface corresponding to |widget|.
+  bool AttachToScenicSurface(gfx::AcceleratedWidget widget,
                              gfx::SysmemBufferCollectionId id);
 
  private:
   scenic::Session scenic_session_;
+  ScenicSurfaceFactory* const scenic_surface_factory_;
   fuchsia::ui::views::ViewHolderToken view_holder_token_;
   scenic::View view_;
   fuchsia::images::ImagePipe2Ptr image_pipe_;
   std::unique_ptr<scenic::Material> image_material_;
 
   bool enable_blend_ = false;
-  ScenicSurface* surface_ = nullptr;
-  gfx::AcceleratedWidget widget_;
+  gfx::AcceleratedWidget widget_ = gfx::kNullAcceleratedWidget;
   gfx::SysmemBufferCollectionId buffer_collection_id_;
 
   THREAD_CHECKER(thread_checker_);

ui/ozone/platform/scenic/scenic_surface_factory.cc

@@ -271,7 +271,9 @@ void ScenicSurfaceFactory::RemoveSurface(gfx::AcceleratedWidget widget) {
 ScenicSurface* ScenicSurfaceFactory::GetSurface(gfx::AcceleratedWidget widget) {
   base::AutoLock lock(surface_lock_);
   auto it = surface_map_.find(widget);
-  DCHECK(it != surface_map_.end());
+  if (it == surface_map_.end())
+    return nullptr;
+
   ScenicSurface* surface = it->second;
   surface->AssertBelongsToCurrentThread();
   return surface;

ui/ozone/platform/scenic/sysmem_buffer_collection.cc

@@ -140,7 +140,8 @@ bool SysmemBufferCollection::Initialize(
   is_protected_ = force_protected;
 
   if (register_with_image_pipe) {
-    scenic_overlay_view_.emplace(scenic_surface_factory->CreateScenicSession());
+    scenic_overlay_view_.emplace(scenic_surface_factory->CreateScenicSession(),
+                                 scenic_surface_factory);
     surface_factory_ = scenic_surface_factory;
   }
 

ui/ozone/platform/scenic/sysmem_native_pixmap.cc

@@ -106,8 +106,7 @@ bool SysmemNativePixmap::ScheduleOverlayPlane(
   DCHECK(collection_->scenic_overlay_view());
   ScenicOverlayView* overlay_view = collection_->scenic_overlay_view();
   const auto& buffer_collection_id = handle_.buffer_collection_id.value();
-  if (!overlay_view->AttachToScenicSurface(surface, widget,
-                                           buffer_collection_id)) {
+  if (!overlay_view->AttachToScenicSurface(widget, buffer_collection_id)) {
     DLOG(ERROR) << "Failed to attach to surface.";
     return false;
   }