Fix segment fault in gpu::ExternalVkImageSkiaRepresentation dtor

I think the crash is because the SharedContextState is released
before calling the dtor. Fix the problem by holding a ref of the
SharedImageContext in the backing.

Bug: 1127327,1131357
Change-Id: I72667f7ff8362757533c1992636575f4d726ba18
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2435325
Commit-Queue: Peng Huang <penghuang@chromium.org>
Reviewed-by: Vasiliy Telezhnikov <vasilyt@chromium.org>
Cr-Commit-Position: refs/heads/master@{#811686}

gpu/command_buffer/service/external_vk_image_backing.cc

@@ -166,7 +166,7 @@ void WaitSemaphoresOnGrContext(GrDirectContext* gr_context,
 
 // static
 std::unique_ptr<ExternalVkImageBacking> ExternalVkImageBacking::Create(
-    SharedContextState* context_state,
+    scoped_refptr<SharedContextState> context_state,
     VulkanCommandPool* command_pool,
     const Mailbox& mailbox,
     viz::ResourceFormat format,
@@ -206,7 +206,7 @@ std::unique_ptr<ExternalVkImageBacking> ExternalVkImageBacking::Create(
     // Must request all available image usage flags if aliasing GL texture. This
     // is a spec requirement per EXT_memory_object. However, if
     // ANGLE_memory_object_flags is supported, usage flags can be arbitrary.
-    if (UseMinimalUsageFlags(context_state)) {
+    if (UseMinimalUsageFlags(context_state.get())) {
       // The following additional usage flags are provided for ANGLE:
       //
       // - TRANSFER_SRC: Used for copies from this image.
@@ -247,10 +247,11 @@ std::unique_ptr<ExternalVkImageBacking> ExternalVkImageBacking::Create(
   if (!image)
     return nullptr;
 
-  bool use_separate_gl_texture = UseSeparateGLTexture(context_state, format);
+  bool use_separate_gl_texture =
+      UseSeparateGLTexture(context_state.get(), format);
   auto backing = std::make_unique<ExternalVkImageBacking>(
       util::PassKey<ExternalVkImageBacking>(), mailbox, format, size,
-      color_space, surface_origin, alpha_type, usage, context_state,
+      color_space, surface_origin, alpha_type, usage, std::move(context_state),
       std::move(image), command_pool, use_separate_gl_texture);
 
   if (!pixel_data.empty()) {
@@ -263,7 +264,7 @@ std::unique_ptr<ExternalVkImageBacking> ExternalVkImageBacking::Create(
 
 // static
 std::unique_ptr<ExternalVkImageBacking> ExternalVkImageBacking::CreateFromGMB(
-    SharedContextState* context_state,
+    scoped_refptr<SharedContextState> context_state,
     VulkanCommandPool* command_pool,
     const Mailbox& mailbox,
     gfx::GpuMemoryBufferHandle handle,
@@ -293,11 +294,12 @@ std::unique_ptr<ExternalVkImageBacking> ExternalVkImageBacking::CreateFromGMB(
     }
 
     bool use_separate_gl_texture =
-        UseSeparateGLTexture(context_state, resource_format);
+        UseSeparateGLTexture(context_state.get(), resource_format);
     auto backing = std::make_unique<ExternalVkImageBacking>(
         util::PassKey<ExternalVkImageBacking>(), mailbox, resource_format, size,
-        color_space, surface_origin, alpha_type, usage, context_state,
-        std::move(image), command_pool, use_separate_gl_texture);
+        color_space, surface_origin, alpha_type, usage,
+        std::move(context_state), std::move(image), command_pool,
+        use_separate_gl_texture);
     backing->SetCleared();
     return backing;
   }
@@ -313,10 +315,10 @@ std::unique_ptr<ExternalVkImageBacking> ExternalVkImageBacking::CreateFromGMB(
   if (!shared_memory_wrapper.Initialize(handle, size, resource_format))
     return nullptr;
 
-  auto backing =
-      Create(context_state, command_pool, mailbox, resource_format, size,
-             color_space, surface_origin, alpha_type, usage, image_usage_cache,
-             base::span<const uint8_t>(), true /* using_gmb */);
+  auto backing = Create(std::move(context_state), command_pool, mailbox,
+                        resource_format, size, color_space, surface_origin,
+                        alpha_type, usage, image_usage_cache,
+                        base::span<const uint8_t>(), true /* using_gmb */);
   if (!backing)
     return nullptr;
 
@@ -333,7 +335,7 @@ ExternalVkImageBacking::ExternalVkImageBacking(
     GrSurfaceOrigin surface_origin,
     SkAlphaType alpha_type,
     uint32_t usage,
-    SharedContextState* context_state,
+    scoped_refptr<SharedContextState> context_state,
     std::unique_ptr<VulkanImage> image,
     VulkanCommandPool* command_pool,
     bool use_separate_gl_texture)
@@ -346,7 +348,7 @@ ExternalVkImageBacking::ExternalVkImageBacking(
                                       usage,
                                       image->device_size(),
                                       false /* is_thread_safe */),
-      context_state_(context_state),
+      context_state_(std::move(context_state)),
       image_(std::move(image)),
       backend_texture_(size.width(),
                        size.height(),
@@ -554,10 +556,8 @@ void ExternalVkImageBacking::AddSemaphoresToPendingListOrRelease(
     // signalling but have not been signalled. In that case, we have to release
     // them via fence helper to make sure all submitted GPU works is finished
     // before releasing them.
-    // |context_state_| is out live fence_helper, so it is safe to use
-    // base::Unretained(context_state_).
     fence_helper()->EnqueueCleanupTaskForSubmittedWork(base::BindOnce(
-        [](SharedContextState* shared_context_state,
+        [](scoped_refptr<SharedContextState> shared_context_state,
            std::vector<ExternalSemaphore>, VulkanDeviceQueue* device_queue,
            bool device_lost) {
           if (!gl::GLContext::GetCurrent()) {
@@ -565,7 +565,7 @@ void ExternalVkImageBacking::AddSemaphoresToPendingListOrRelease(
                                               /*needs_gl=*/true);
           }
         },
-        base::Unretained(context_state_), std::move(semaphores)));
+        context_state_, std::move(semaphores)));
   }
 }
 
@@ -662,7 +662,7 @@ GLuint ExternalVkImageBacking::ProduceGLTextureInternal() {
     // If ANGLE_memory_object_flags is supported, use that to communicate the
     // exact create and usage flags the image was created with.
     DCHECK(image_->usage() != 0);
-    if (UseMinimalUsageFlags(context_state_)) {
+    if (UseMinimalUsageFlags(context_state())) {
       api->glTexStorageMemFlags2DANGLEFn(
           GL_TEXTURE_2D, 1, internal_format, size().width(), size().height(),
           memory_object->id(), 0, image_->flags(), image_->usage());
@@ -749,7 +749,7 @@ ExternalVkImageBacking::ProduceSkia(
     scoped_refptr<SharedContextState> context_state) {
   // This backing type is only used when vulkan is enabled, so SkiaRenderer
   // should also be using Vulkan.
-  DCHECK_EQ(context_state_, context_state.get());
+  DCHECK_EQ(context_state_, context_state);
   DCHECK(context_state->GrContextIsVulkan());
   return std::make_unique<ExternalVkImageSkiaRepresentation>(manager, this,
                                                              tracker);

gpu/command_buffer/service/external_vk_image_backing.h

@@ -34,7 +34,7 @@ struct VulkanImageUsageCache {
 class ExternalVkImageBacking final : public ClearTrackingSharedImageBacking {
  public:
   static std::unique_ptr<ExternalVkImageBacking> Create(
-      SharedContextState* context_state,
+      scoped_refptr<SharedContextState> context_state,
       VulkanCommandPool* command_pool,
       const Mailbox& mailbox,
       viz::ResourceFormat format,
@@ -48,7 +48,7 @@ class ExternalVkImageBacking final : public ClearTrackingSharedImageBacking {
       bool using_gmb = false);
 
   static std::unique_ptr<ExternalVkImageBacking> CreateFromGMB(
-      SharedContextState* context_state,
+      scoped_refptr<SharedContextState> context_state,
       VulkanCommandPool* command_pool,
       const Mailbox& mailbox,
       gfx::GpuMemoryBufferHandle handle,
@@ -68,14 +68,14 @@ class ExternalVkImageBacking final : public ClearTrackingSharedImageBacking {
                          GrSurfaceOrigin surface_origin,
                          SkAlphaType alpha_type,
                          uint32_t usage,
-                         SharedContextState* context_state,
+                         scoped_refptr<SharedContextState> context_state,
                          std::unique_ptr<VulkanImage> image,
                          VulkanCommandPool* command_pool,
                          bool use_separate_gl_texture);
 
   ~ExternalVkImageBacking() override;
 
-  SharedContextState* context_state() const { return context_state_; }
+  SharedContextState* context_state() const { return context_state_.get(); }
   const GrBackendTexture& backend_texture() const { return backend_texture_; }
   VulkanImage* image() const { return image_.get(); }
   const scoped_refptr<gles2::TexturePassthrough>& GetTexturePassthrough()
@@ -178,7 +178,7 @@ class ExternalVkImageBacking final : public ClearTrackingSharedImageBacking {
   void CopyPixelsFromGLTextureToVkImage();
   void CopyPixelsFromShmToGLTexture();
 
-  SharedContextState* const context_state_;
+  scoped_refptr<SharedContextState> context_state_;
   std::unique_ptr<VulkanImage> image_;
   GrBackendTexture backend_texture_;
   VulkanCommandPool* const command_pool_;

gpu/command_buffer/service/external_vk_image_factory.cc

@@ -61,8 +61,8 @@ VulkanImageUsageCache CreateImageUsageCache(
 }  // namespace
 
 ExternalVkImageFactory::ExternalVkImageFactory(
-    SharedContextState* context_state)
-    : context_state_(context_state),
+    scoped_refptr<SharedContextState> context_state)
+    : context_state_(std::move(context_state)),
       command_pool_(context_state_->vk_context_provider()
                         ->GetDeviceQueue()
                         ->CreateCommandPool()),

gpu/command_buffer/service/external_vk_image_factory.h

@@ -22,7 +22,8 @@ class VulkanCommandPool;
 // that allow it to be exported out and shared with GL.
 class ExternalVkImageFactory : public SharedImageBackingFactory {
  public:
-  explicit ExternalVkImageFactory(SharedContextState* context_state);
+  explicit ExternalVkImageFactory(
+      scoped_refptr<SharedContextState> context_state);
   ~ExternalVkImageFactory() override;
 
   // SharedImageBackingFactory implementation.
@@ -66,7 +67,7 @@ class ExternalVkImageFactory : public SharedImageBackingFactory {
 
   void TransitionToColorAttachment(VkImage image);
 
-  SharedContextState* const context_state_;
+  scoped_refptr<SharedContextState> context_state_;
   std::unique_ptr<VulkanCommandPool> command_pool_;
 
   const VulkanImageUsageCache image_usage_cache_;

gpu/ipc/shared_image_interface_in_process.cc

@@ -84,8 +84,10 @@ void SharedImageInterfaceInProcess::SetUpOnGpu(
 void SharedImageInterfaceInProcess::DestroyOnGpu(
     base::WaitableEvent* completion) {
   bool have_context = MakeContextCurrent();
-  if (shared_image_factory_)
+  if (shared_image_factory_) {
     shared_image_factory_->DestroyAllSharedImages(have_context);
+    shared_image_factory_ = nullptr;
+  }
 
   if (sync_point_client_state_) {
     sync_point_client_state_->Destroy();