Fix a crash in ash::FastInkHost when the underlying frame sink is lost

This CL also updates the comments for
`FrameSinkHost::OnFirstFrameRequested()`

Bug: b:401587130
Change-Id: Ied15cc1a8b5298ed3fe1c10be053e03a9291061c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6344345
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: Zoraiz Naeem <zoraiznaeem@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1432216}

ash/fast_ink/fast_ink_host.cc

@@ -59,11 +59,7 @@ FastInkHost::ScopedPaint::~ScopedPaint() {
 
 FastInkHost::FastInkHost() = default;
 FastInkHost::~FastInkHost() {
-  if (client_shared_image_) {
-    CHECK(context_provider_);
-    context_provider_->SharedImageInterface()->DestroySharedImage(
-        sync_token_, std::move(client_shared_image_));
-  }
+  ResetGpuBuffer();
 }
 
 void FastInkHost::Init(aura::Window* host_window) {
@@ -104,7 +100,19 @@ std::unique_ptr<viz::CompositorFrame> FastInkHost::CreateCompositorFrame(
 }
 
 void FastInkHost::OnFirstFrameRequested() {
+  // Only create a buffer if not initialized as `OnFirstFrameRequested()` is
+  // called for every first begin frame for a FrameSink.
+  if (!client_shared_image_) {
+    InitializeFastInkBuffer(host_window());
+  }
+}
+
+void FastInkHost::OnFrameSinkLost() {
+  // The fast ink buffer becomes unusable and a new buffer must be created when
+  // the GPU crashes, which is one of the most common causes of FrameSink loss.
+  ResetGpuBuffer();
   InitializeFastInkBuffer(host_window());
+  FrameSinkHost::OnFrameSinkLost();
 }
 
 void FastInkHost::InitBufferMetadata(aura::Window* host_window) {
@@ -143,7 +151,7 @@ void FastInkHost::InitializeFastInkBuffer(aura::Window* host_window) {
     usage |= gpu::SHARED_IMAGE_USAGE_SCANOUT;
   }
 
-  CHECK(!client_shared_image_) << "GPU buffer should be created once";
+  CHECK(!client_shared_image_) << "GPU buffer is already initialized";
   client_shared_image_ = fast_ink_internal::CreateMappableSharedImage(
       buffer_size_, usage, gfx::BufferUsage::SCANOUT_CPU_READ_WRITE);
 
@@ -172,6 +180,7 @@ void FastInkHost::InitializeFastInkBuffer(aura::Window* host_window) {
   for (auto pending_bitmap : pending_bitmaps_) {
     DrawBitmap(pending_bitmap.bitmap, pending_bitmap.damage_rect);
   }
+
   pending_bitmaps_.clear();
 }
 
@@ -191,6 +200,7 @@ void FastInkHost::Draw(SkBitmap bitmap, const gfx::Rect& damage_rect) {
     pending_bitmaps_.push_back(PendingBitmap(bitmap, damage_rect));
     return;
   }
+
   DrawBitmap(bitmap, damage_rect);
 }
 
@@ -228,4 +238,14 @@ void FastInkHost::DrawBitmap(SkBitmap bitmap, const gfx::Rect& damage_rect) {
   }
 }
 
+void FastInkHost::ResetGpuBuffer() {
+  if (client_shared_image_) {
+    CHECK(context_provider_);
+    context_provider_->SharedImageInterface()->DestroySharedImage(
+        sync_token_, std::move(client_shared_image_));
+    client_shared_image_.reset();
+    sync_token_.Clear();
+  }
+}
+
 }  // namespace ash

ash/fast_ink/fast_ink_host.h

@@ -23,7 +23,7 @@ class Rect;
 
 namespace gpu {
 class ClientSharedImage;
-}
+}  // namespace gpu
 
 namespace ash {
 
@@ -76,6 +76,8 @@ class ASH_EXPORT FastInkHost : public FrameSinkHost {
     return pending_bitmaps_.size();
   }
 
+  const gpu::SyncToken& sync_token_for_test() const { return sync_token_; }
+
   // FrameSinkHost:
   void Init(aura::Window* host_window) override;
   void InitForTesting(aura::Window* host_window,
@@ -90,6 +92,7 @@ class ASH_EXPORT FastInkHost : public FrameSinkHost {
       const gfx::Size& last_submitted_frame_size,
       float last_submitted_frame_dsf) override;
   void OnFirstFrameRequested() override;
+  void OnFrameSinkLost() override;
 
  private:
   void InitBufferMetadata(aura::Window* host_window);
@@ -97,6 +100,7 @@ class ASH_EXPORT FastInkHost : public FrameSinkHost {
   gfx::Rect BufferRectFromWindowRect(const gfx::Rect& rect_in_window) const;
   void Draw(SkBitmap bitmap, const gfx::Rect& damage_rect);
   void DrawBitmap(SkBitmap bitmap, const gfx::Rect& damage_rect);
+  void ResetGpuBuffer();
 
   gfx::Transform window_to_buffer_transform_;
 
@@ -106,7 +110,6 @@ class ASH_EXPORT FastInkHost : public FrameSinkHost {
     SkBitmap bitmap;
     gfx::Rect damage_rect;
   };
-
   std::vector<PendingBitmap> pending_bitmaps_;
 
   scoped_refptr<gpu::ClientSharedImage> client_shared_image_;

ash/fast_ink/fast_ink_host_unittest.cc

@@ -9,6 +9,7 @@
 #include <utility>
 #include <vector>
 
+#include "ash/frame_sink/frame_sink_holder.h"
 #include "ash/frame_sink/test/frame_sink_host_test_base.h"
 #include "ash/frame_sink/test/test_begin_frame_source.h"
 #include "ash/frame_sink/test/test_layer_tree_frame_sink.h"
@@ -122,6 +123,36 @@ TEST_P(FastInkHostTest, CorrectFrameSubmittedToLayerTreeFrameSink) {
   EXPECT_EQ(frame.resource_list.back().is_overlay_candidate, auto_update_);
 }
 
+TEST_P(FastInkHostTest, RecreateGpuBufferOnLosingFrameSink) {
+  // Buffer is not initialized when there is no begin frame received.
+  ASSERT_FALSE(frame_sink_host()->client_si_for_test());
+
+  // Request the first frame. It will call
+  // `FrameSinkHost::OnFirstFrameRequested()` initializing the GPU buffer.
+  OnBeginFrame();
+
+  // MappableSI should be initialized after receiving the first begin frame.
+  ASSERT_TRUE(frame_sink_host()->client_si_for_test());
+  auto sync_token = frame_sink_host()->sync_token_for_test();
+
+  // A new frame-sink will be created. FastInkHost should also create a new
+  // shared image.
+  frame_sink_host()
+      ->frame_sink_holder_for_testing()
+      ->DidLoseLayerTreeFrameSink();
+
+  EXPECT_NE(sync_token, frame_sink_host()->sync_token_for_test());
+  sync_token = frame_sink_host()->sync_token_for_test();
+
+  // This will be the first OnBeginFrame for the new frame sink, therefore
+  // `FrameSinkHost::OnFirstFrameRequested()` will be called again.
+  OnBeginFrame();
+
+  // Ensure we do not recreate a shared image on
+  // `FrameSinkHost::OnFirstFrameRequested()`.
+  EXPECT_EQ(sync_token, frame_sink_host()->sync_token_for_test());
+}
+
 TEST_P(FastInkHostTest, DelayPaintingUntilReceivingFirstBeginFrame) {
   // Buffer is not initialized when there is no begin frame received.
   ASSERT_FALSE(frame_sink_host()->client_si_for_test());

ash/frame_sink/frame_sink_host.h

@@ -101,11 +101,17 @@ class ASH_EXPORT FrameSinkHost : public aura::WindowObserver {
       const gfx::Size& last_submitted_frame_size,
       float last_submitted_frame_dsf) = 0;
 
-  // Callback invoked when underlying frame sink holder gets the first begin
-  // frame from viz. This signifies that the gpu process has been fully
-  // initialized.
+  // Callback invoked when frame sink gets the first begin from the display
+  // compositor for existing FrameSink. This signifies that the gpu process has
+  // been fully initialized.
+  // Note: This method will be reinvoked when a new FrameSink is created in the
+  // event of losing the old FrameSink. (See `OnFrameSinkLost()`)
   virtual void OnFirstFrameRequested();
 
+  // Callback invoked when the connection to `LayerTreeFrameSink` is lost. (i.e
+  // gpu crashed, host_window closes etc)
+  virtual void OnFrameSinkLost();
+
   const gfx::Rect& GetTotalDamage() const { return total_damage_rect_; }
 
   void UnionDamage(const gfx::Rect& rect) { total_damage_rect_.Union(rect); }
@@ -127,10 +133,6 @@ class ASH_EXPORT FrameSinkHost : public aura::WindowObserver {
 
   std::unique_ptr<cc::LayerTreeFrameSink> CreateLayerTreeFrameSink();
 
-  // Callback invoked when the connection to `LayerTreeFrameSink` is lost. (i.e
-  // gpu crashed, host_window closes etc)
-  void OnFrameSinkLost();
-
   // Observation to track the lifetime of `host_window_`.
   base::ScopedObservation<aura::Window, aura::WindowObserver>
       host_window_observation_{this};