[Impeller] Do not reference this in the submit callback for Metal GPU Surfaces (#50361)

Fixes flutter/flutter#141351 (speculatively - I have not directly reproduced this in an application, but without this change the added test crashes with a segfault in the submit callback).

If the rasterizer gets torn down, the surface gets released and the submit callback may fire on a collected object. Capturing `this` isn't safe. I'm not quite sure how that could happen from the linked stack trace though, since the draw call and the teardown call should be happening on the raster thread, and if the surface was reset then the draw call should've failed earlier...

The added test causes a segfault without the change.

Author: dnfield

shell/gpu/gpu_surface_metal_impeller.h

@@ -47,7 +47,8 @@ class IMPELLER_CA_METAL_LAYER_AVAILABLE GPUSurfaceMetalImpeller
   bool disable_partial_repaint_ = false;
   // Accumulated damage for each framebuffer; Key is address of underlying
   // MTLTexture for each drawable
-  std::map<uintptr_t, SkIRect> damage_;
+  std::shared_ptr<std::map<uintptr_t, SkIRect>> damage_ =
+      std::make_shared<std::map<uintptr_t, SkIRect>>();
 
   // |Surface|
   std::unique_ptr<SurfaceFrame> AcquireFrame(

shell/gpu/gpu_surface_metal_impeller.mm

@@ -108,11 +108,12 @@
 
   id<MTLTexture> last_texture = static_cast<id<MTLTexture>>(last_texture_);
   SurfaceFrame::SubmitCallback submit_callback =
-      fml::MakeCopyable([this,                           //
-                         renderer = impeller_renderer_,  //
-                         aiks_context = aiks_context_,   //
-                         drawable,                       //
-                         last_texture                    //
+      fml::MakeCopyable([damage = damage_,
+                         disable_partial_repaint = disable_partial_repaint_,  //
+                         renderer = impeller_renderer_,                       //
+                         aiks_context = aiks_context_,                        //
+                         drawable,                                            //
+                         last_texture                                         //
   ](SurfaceFrame& surface_frame, DlCanvas* canvas) mutable -> bool {
         if (!aiks_context) {
           return false;
@@ -124,10 +125,10 @@
           return false;
         }
 
-        if (!disable_partial_repaint_) {
+        if (!disable_partial_repaint && damage) {
           uintptr_t texture = reinterpret_cast<uintptr_t>(last_texture);
 
-          for (auto& entry : damage_) {
+          for (auto& entry : *damage) {
             if (entry.first != texture) {
               // Accumulate damage for other framebuffers
               if (surface_frame.submit_info().frame_damage) {
@@ -136,7 +137,7 @@
             }
           }
           // Reset accumulated damage for current framebuffer
-          damage_[texture] = SkIRect::MakeEmpty();
+          (*damage)[texture] = SkIRect::MakeEmpty();
         }
 
         std::optional<impeller::IRect> clip_rect;
@@ -146,8 +147,8 @@
                                                 buffer_damage->width(), buffer_damage->height());
         }
 
-        auto surface = impeller::SurfaceMTL::MakeFromMetalLayerDrawable(
-            impeller_renderer_->GetContext(), drawable, clip_rect);
+        auto surface = impeller::SurfaceMTL::MakeFromMetalLayerDrawable(renderer->GetContext(),
+                                                                        drawable, clip_rect);
 
         if (clip_rect && clip_rect->IsEmpty()) {
           return surface->Present();
@@ -174,8 +175,8 @@
     // Provide accumulated damage to rasterizer (area in current framebuffer that lags behind
     // front buffer)
     uintptr_t texture = reinterpret_cast<uintptr_t>(drawable.texture);
-    auto i = damage_.find(texture);
-    if (i != damage_.end()) {
+    auto i = damage_->find(texture);
+    if (i != damage_->end()) {
       framebuffer_info.existing_damage = i->second;
     }
     framebuffer_info.supports_partial_repaint = true;
@@ -205,7 +206,8 @@
   }
 
   SurfaceFrame::SubmitCallback submit_callback =
-      fml::MakeCopyable([this,                           //
+      fml::MakeCopyable([disable_partial_repaint = disable_partial_repaint_,  //
+                         damage = damage_,
                          renderer = impeller_renderer_,  //
                          aiks_context = aiks_context_,   //
                          texture_info,                   //
@@ -222,10 +224,10 @@
           return false;
         }
 
-        if (!disable_partial_repaint_) {
+        if (!disable_partial_repaint && damage) {
           uintptr_t texture_ptr = reinterpret_cast<uintptr_t>(mtl_texture);
 
-          for (auto& entry : damage_) {
+          for (auto& entry : *damage) {
             if (entry.first != texture_ptr) {
               // Accumulate damage for other framebuffers
               if (surface_frame.submit_info().frame_damage) {
@@ -234,7 +236,7 @@
             }
           }
           // Reset accumulated damage for current framebuffer
-          damage_[texture_ptr] = SkIRect::MakeEmpty();
+          (*damage)[texture_ptr] = SkIRect::MakeEmpty();
         }
 
         std::optional<impeller::IRect> clip_rect;
@@ -278,8 +280,8 @@
     // Provide accumulated damage to rasterizer (area in current framebuffer that lags behind
     // front buffer)
     uintptr_t texture = reinterpret_cast<uintptr_t>(mtl_texture);
-    auto i = damage_.find(texture);
-    if (i != damage_.end()) {
+    auto i = damage_->find(texture);
+    if (i != damage_->end()) {
       framebuffer_info.existing_damage = i->second;
     }
     framebuffer_info.supports_partial_repaint = true;

shell/gpu/gpu_surface_metal_impeller_unittests.mm

@@ -24,6 +24,7 @@
   ~TestGPUSurfaceMetalDelegate() = default;
 
   GPUCAMetalLayerHandle GetCAMetalLayer(const SkISize& frame_info) const override {
+    layer_.drawableSize = CGSizeMake(frame_info.width(), frame_info.height());
     return (__bridge GPUCAMetalLayerHandle)(layer_);
   }
 
@@ -35,6 +36,8 @@ GPUCAMetalLayerHandle GetCAMetalLayer(const SkISize& frame_info) const override
 
   bool AllowsDrawingWhenGpuDisabled() const override { return true; }
 
+  void SetDevice() { layer_.device = ::MTLCreateSystemDefaultDevice(); }
+
  private:
   CAMetalLayer* layer_ = nil;
 };
@@ -77,5 +80,22 @@ GPUCAMetalLayerHandle GetCAMetalLayer(const SkISize& frame_info) const override
   ASSERT_EQ(frame, nullptr);
 }
 
+TEST(GPUSurfaceMetalImpeller, AcquireFrameFromCAMetalLayerDoesNotRetainThis) {
+  auto delegate = std::make_shared<TestGPUSurfaceMetalDelegate>();
+  delegate->SetDevice();
+  std::unique_ptr<Surface> surface =
+      std::make_unique<GPUSurfaceMetalImpeller>(delegate.get(), CreateImpellerContext());
+
+  ASSERT_TRUE(surface->IsValid());
+
+  auto frame = surface->AcquireFrame(SkISize::Make(100, 100));
+  ASSERT_TRUE(frame);
+
+  // Simulate a rasterizer teardown, e.g. due to going to the background.
+  surface.reset();
+
+  ASSERT_TRUE(frame->Submit());
+}
+
 }  // namespace testing
 }  // namespace flutter