From 30e4d30f53b3811a509ad899be7a38abf748af06 Mon Sep 17 00:00:00 2001
From: kylechar <[email protected]>
Date: Wed, 21 Oct 2026 06:30:22 -0700
Subject: [PATCH] Fix ImageContext thread safety

SkiaOutputSurfaceImpl was resetting the ImageContextImpl SkImage in two
places. This isn't thread safe as it's possible SkImage is being
accessed on the GPU thread at the same time.

First, before sending the ImageContextImpl to the GPU main thread to be
deleted. This just wasn't needed.

Second, it was resetting non-overlay render pass SkImages each time they
were drawn. This was in case the render pass backing changed and a new
SkImage was needed. SkiaRenderer::UpdateRenderPassTextures() already
calls RemoveRenderPassResource() for any render pass backings that need
to be reallocated which deletes the ImageContextImpl. A new
ImageContextImpl and SkImage will be created in that case.

Bug: 574030524
Change-Id: Ic25d3d10daf8f719757a42d73710dbab7095328e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8608392
Commit-Queue: Kyle Charbonneau <[email protected]>
Reviewed-by: Bo Liu <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1723735}
---
diff --git a/components/viz/service/display/external_use_client.h b/components/viz/service/display/external_use_client.h
index 1f2f104..1657489 100644
--- a/components/viz/service/display/external_use_client.h
+++ b/components/viz/service/display/external_use_client.h
@@ -51,10 +51,9 @@
     virtual void OnContextLost();
 
     //
-    // Thread safety is guaranteed by these invariants: (a) only the compositor
-    // thread modifies ImageContext, (b) ImageContext is not modified after
-    // |image| is set, and (c) GPU thread only reads ImageContext after |image|
-    // is set.
+    // Thread safety is guaranteed by these invariants: (a) only the viz thread
+    // modifies ImageContext, (b) ImageContext is not modified after |image| is
+    // set, and (c) GPU thread only reads ImageContext after |image| is set.
     //
     const gpu::Mailbox& mailbox() const { return mailbox_; }
     uint32_t texture_target() const { return texture_target_; }
@@ -83,11 +82,6 @@
 
     bool has_image() { return !!image_; }
     sk_sp<SkImage> image() { return image_; }
-    void SetImage(sk_sp<SkImage> image,
-                  std::vector<GrBackendFormat> backend_formats);
-    void SetImage(sk_sp<SkImage> image,
-                  std::vector<skgpu::graphite::TextureInfo> texture_infos);
-    void clear_image() { image_.reset(); }
     const std::vector<GrBackendFormat>& backend_formats() {
       return backend_formats_;
     }
@@ -95,10 +89,22 @@
       return texture_infos_;
     }
 
+    // SkImage and texture info MUST not be modified after they are initially
+    // set on the viz thread.
+    void SetImage(sk_sp<SkImage> image,
+                  std::vector<GrBackendFormat> backend_formats);
+    void SetImage(sk_sp<SkImage> image,
+                  std::vector<skgpu::graphite::TextureInfo> texture_infos);
+
+   protected:
+    // Resetting the image is only safe to do when ImageContext is no longer
+    // held on the viz thread, eg. during the destructor.
+    void clear_image() { image_.reset(); }
+
    private:
-    gpu::Mailbox mailbox_;
+    const gpu::Mailbox mailbox_;
     std::vector<gpu::SyncToken> sync_tokens_;
-    uint32_t texture_target_;
+    const uint32_t texture_target_;
 
     const gfx::Size size_;
     const SharedImageFormat format_;
@@ -113,7 +119,7 @@
     // android video.
     std::optional<gpu::VulkanYCbCrInfo> ycbcr_info_;
 
-    // The promise image which is used on display thread.
+    // The promise image which is used on the GPU thread.
     sk_sp<SkImage> image_;
     std::vector<GrBackendFormat> backend_formats_;
     std::vector<skgpu::graphite::TextureInfo> texture_infos_;
diff --git a/components/viz/service/display_embedder/skia_output_surface_impl.cc b/components/viz/service/display_embedder/skia_output_surface_impl.cc
index 1ce7523..33821d5 100644
--- a/components/viz/service/display_embedder/skia_output_surface_impl.cc
+++ b/components/viz/service/display_embedder/skia_output_surface_impl.cc
@@ -793,16 +793,6 @@
     current_paint_.emplace(characterization, mailbox, skip_draw_for_tests_);
   }
 
-  // We are going to overwrite the render pass when it is not for overlay, so we
-  // need to reset the image_context and a new promise image will be created
-  // when MakePromiseSkImageFromRenderPass() is called.
-  if (!is_overlay) {
-    auto it = render_pass_image_cache_.find(id);
-    if (it != render_pass_image_cache_.end()) {
-      it->second->clear_image();
-    }
-  }
-
   return current_paint_->canvas();
 }
 
@@ -902,12 +892,13 @@
   if (!image_context) {
     image_context =
         std::make_unique<ImageContextImpl>(mailbox, size, format, color_space);
-  }
-  if (!image_context->has_image()) {
     MakePromiseSkImageSinglePlane(image_context.get(), mipmap, false);
     if (!image_context->has_image()) {
+      image_context.reset();
       return nullptr;
     }
+  } else {
+    CHECK(image_context->mailbox() == mailbox);
   }
   images_in_current_paint_.push_back(image_context.get());
   return image_context->image();
@@ -925,7 +916,6 @@
     // If the render pass was only used for a copy request, there won't be a
     // matching entry in |render_pass_image_cache_|.
     if (it != render_pass_image_cache_.end()) {
-      it->second->clear_image();
       image_contexts.push_back(std::move(it->second));
       render_pass_image_cache_.erase(it);
     }