Reland "[ios_platform_view] only recycle maskView when the view is applying mutators #42115" (#42823)

Relands #42115, which was reverted in #42231 due to a crash in the framework test.

The crash is due to a memory management issue that I fixed it in this PR, I also added a scenario test to catch the crash.

fixes: flutter/flutter#125620

See also: orignal PR #41573 for more details.

[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

Author: Chris Yang

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm

@@ -18,8 +18,6 @@
 #import "flutter/shell/platform/darwin/ios/framework/Source/FlutterViewController_Internal.h"
 #import "flutter/shell/platform/darwin/ios/ios_surface.h"
 
-static const NSUInteger kFlutterClippingMaskViewPoolCapacity = 5;
-
 @implementation UIView (FirstResponder)
 - (BOOL)flt_hasFirstResponderInViewHierarchySubtree {
   if (self.isFirstResponder) {
@@ -447,6 +445,8 @@ static bool ClipRRectContainsPlatformViewBoundingRect(const SkRRect& clip_rrect,
   clipView.maskView = [mask_view_pool_.get() getMaskViewWithFrame:frame];
 }
 
+// This method is only called when the `embedded_view` needs to be re-composited at the current
+// frame. See: `CompositeWithParams` for details.
 void FlutterPlatformViewsController::ApplyMutators(const MutatorsStack& mutators_stack,
                                                    UIView* embedded_view,
                                                    const SkRect& bounding_rect) {
@@ -461,12 +461,10 @@ static bool ClipRRectContainsPlatformViewBoundingRect(const SkRRect& clip_rrect,
   NSMutableArray* blurFilters = [[[NSMutableArray alloc] init] autorelease];
   FML_DCHECK(!clipView.maskView ||
              [clipView.maskView isKindOfClass:[FlutterClippingMaskView class]]);
-  if (mask_view_pool_.get() == nil) {
-    mask_view_pool_.reset([[FlutterClippingMaskViewPool alloc]
-        initWithCapacity:kFlutterClippingMaskViewPoolCapacity]);
+  if (clipView.maskView) {
+    [mask_view_pool_.get() insertViewToPoolIfNeeded:(FlutterClippingMaskView*)(clipView.maskView)];
+    clipView.maskView = nil;
   }
-  [mask_view_pool_.get() recycleMaskViews];
-  clipView.maskView = nil;
   CGFloat screenScale = [UIScreen mainScreen].scale;
   auto iter = mutators_stack.Begin();
   while (iter != mutators_stack.End()) {
@@ -570,6 +568,14 @@ static bool ClipRRectContainsPlatformViewBoundingRect(const SkRRect& clip_rrect,
   embedded_view.layer.transform = flutter::GetCATransform3DFromSkMatrix(transformMatrix);
 }
 
+// Composite the PlatformView with `view_id`.
+//
+// Every frame, during the paint traversal of the layer tree, this method is called for all
+// the PlatformViews in `views_to_recomposite_`.
+//
+// Note that `views_to_recomposite_` does not represent all the views in the view hierarchy,
+// if a PlatformView does not change its composition parameter from last frame, it is not
+// included in the `views_to_recomposite_`.
 void FlutterPlatformViewsController::CompositeWithParams(int64_t view_id,
                                                          const EmbeddedViewParams& params) {
   CGRect frame = CGRectMake(0, 0, params.sizePoints().width(), params.sizePoints().height());

shell/platform/darwin/ios/framework/Source/FlutterPlatformViewsTest.mm

@@ -2703,12 +2703,15 @@ - (void)testFlutterClippingMaskViewPoolReuseViewsAfterRecycle {
       [[[FlutterClippingMaskViewPool alloc] initWithCapacity:2] autorelease];
   FlutterClippingMaskView* view1 = [pool getMaskViewWithFrame:CGRectZero];
   FlutterClippingMaskView* view2 = [pool getMaskViewWithFrame:CGRectZero];
-  [pool recycleMaskViews];
+  [pool insertViewToPoolIfNeeded:view1];
+  [pool insertViewToPoolIfNeeded:view2];
   CGRect newRect = CGRectMake(0, 0, 10, 10);
   FlutterClippingMaskView* view3 = [pool getMaskViewWithFrame:newRect];
   FlutterClippingMaskView* view4 = [pool getMaskViewWithFrame:newRect];
-  XCTAssertEqual(view1, view3);
-  XCTAssertEqual(view2, view4);
+  // view3 and view4 should randomly get either of view1 and view2.
+  NSSet* set1 = [NSSet setWithObjects:view1, view2, nil];
+  NSSet* set2 = [NSSet setWithObjects:view3, view4, nil];
+  XCTAssertEqualObjects(set1, set2);
   XCTAssertTrue(CGRectEqualToRect(view3.frame, newRect));
   XCTAssertTrue(CGRectEqualToRect(view4.frame, newRect));
 }
@@ -2783,17 +2786,17 @@ - (void)testClipMaskViewIsReused {
   auto embeddedViewParams1 = std::make_unique<flutter::EmbeddedViewParams>(
       screenScaleMatrix, SkSize::Make(10, 10), stack1);
 
-  flutter::MutatorsStack stack2;
-  auto embeddedViewParams2 = std::make_unique<flutter::EmbeddedViewParams>(
-      screenScaleMatrix, SkSize::Make(10, 10), stack2);
-
   flutterPlatformViewsController->PrerollCompositeEmbeddedView(1, std::move(embeddedViewParams1));
   flutterPlatformViewsController->CompositeEmbeddedView(1);
   UIView* childClippingView1 = gMockPlatformView.superview.superview;
   UIView* maskView1 = childClippingView1.maskView;
   XCTAssertNotNil(maskView1);
 
   // Composite a new frame.
+  flutterPlatformViewsController->BeginFrame(SkISize::Make(100, 100));
+  flutter::MutatorsStack stack2;
+  auto embeddedViewParams2 = std::make_unique<flutter::EmbeddedViewParams>(
+      screenScaleMatrix, SkSize::Make(10, 10), stack2);
   auto embeddedViewParams3 = std::make_unique<flutter::EmbeddedViewParams>(
       screenScaleMatrix, SkSize::Make(10, 10), stack2);
   flutterPlatformViewsController->PrerollCompositeEmbeddedView(1, std::move(embeddedViewParams3));
@@ -2819,6 +2822,79 @@ - (void)testClipMaskViewIsReused {
   XCTAssertNil(childClippingView1.maskView);
 }
 
+- (void)testDifferentClipMaskViewIsUsedForEachView {
+  flutter::FlutterPlatformViewsTestMockPlatformViewDelegate mock_delegate;
+  auto thread_task_runner = CreateNewThread("FlutterPlatformViewsTest");
+  flutter::TaskRunners runners(/*label=*/self.name.UTF8String,
+                               /*platform=*/thread_task_runner,
+                               /*raster=*/thread_task_runner,
+                               /*ui=*/thread_task_runner,
+                               /*io=*/thread_task_runner);
+  auto flutterPlatformViewsController = std::make_shared<flutter::FlutterPlatformViewsController>();
+  auto platform_view = std::make_unique<flutter::PlatformViewIOS>(
+      /*delegate=*/mock_delegate,
+      /*rendering_api=*/flutter::IOSRenderingAPI::kSoftware,
+      /*platform_views_controller=*/flutterPlatformViewsController,
+      /*task_runners=*/runners,
+      /*worker_task_runner=*/nil,
+      /*is_gpu_disabled_sync_switch=*/nil);
+
+  FlutterPlatformViewsTestMockFlutterPlatformFactory* factory =
+      [[FlutterPlatformViewsTestMockFlutterPlatformFactory new] autorelease];
+  flutterPlatformViewsController->RegisterViewFactory(
+      factory, @"MockFlutterPlatformView",
+      FlutterPlatformViewGestureRecognizersBlockingPolicyEager);
+  FlutterResult result = ^(id result) {
+  };
+
+  flutterPlatformViewsController->OnMethodCall(
+      [FlutterMethodCall
+          methodCallWithMethodName:@"create"
+                         arguments:@{@"id" : @1, @"viewType" : @"MockFlutterPlatformView"}],
+      result);
+  UIView* view1 = gMockPlatformView;
+
+  // This overwrites `gMockPlatformView` to another view.
+  flutterPlatformViewsController->OnMethodCall(
+      [FlutterMethodCall
+          methodCallWithMethodName:@"create"
+                         arguments:@{@"id" : @2, @"viewType" : @"MockFlutterPlatformView"}],
+      result);
+  UIView* view2 = gMockPlatformView;
+
+  XCTAssertNotNil(gMockPlatformView);
+  UIView* mockFlutterView = [[[UIView alloc] initWithFrame:CGRectMake(0, 0, 10, 10)] autorelease];
+  flutterPlatformViewsController->SetFlutterView(mockFlutterView);
+  // Create embedded view params
+  flutter::MutatorsStack stack1;
+  // Layer tree always pushes a screen scale factor to the stack
+  SkMatrix screenScaleMatrix =
+      SkMatrix::Scale([UIScreen mainScreen].scale, [UIScreen mainScreen].scale);
+  stack1.PushTransform(screenScaleMatrix);
+  // Push a clip rect
+  SkRect rect = SkRect::MakeXYWH(2, 2, 3, 3);
+  stack1.PushClipRect(rect);
+
+  auto embeddedViewParams1 = std::make_unique<flutter::EmbeddedViewParams>(
+      screenScaleMatrix, SkSize::Make(10, 10), stack1);
+
+  flutter::MutatorsStack stack2;
+  stack2.PushClipRect(rect);
+  auto embeddedViewParams2 = std::make_unique<flutter::EmbeddedViewParams>(
+      screenScaleMatrix, SkSize::Make(10, 10), stack2);
+
+  flutterPlatformViewsController->PrerollCompositeEmbeddedView(1, std::move(embeddedViewParams1));
+  flutterPlatformViewsController->CompositeEmbeddedView(1);
+  UIView* childClippingView1 = view1.superview.superview;
+
+  flutterPlatformViewsController->PrerollCompositeEmbeddedView(2, std::move(embeddedViewParams2));
+  flutterPlatformViewsController->CompositeEmbeddedView(2);
+  UIView* childClippingView2 = view2.superview.superview;
+  UIView* maskView1 = childClippingView1.maskView;
+  UIView* maskView2 = childClippingView2.maskView;
+  XCTAssertNotEqual(maskView1, maskView2);
+}
+
 // Return true if a correct visual effect view is found. It also implies all the validation in this
 // method passes.
 //

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews_Internal.h

@@ -56,7 +56,7 @@
 // in the pool. If there are none available, a new FlutterClippingMaskView is constructed. If the
 // capacity is reached, the newly constructed FlutterClippingMaskView is not added to the pool.
 //
-// Call |recycleMaskViews| to mark all the FlutterClippingMaskViews in the pool available.
+// Call |insertViewToPoolIfNeeded:| to return a maskView to the pool.
 @interface FlutterClippingMaskViewPool : NSObject
 
 // Initialize the pool with `capacity`. When the `capacity` is reached, a FlutterClippingMaskView is
@@ -66,8 +66,8 @@
 // Reuse a maskView from the pool, or allocate a new one.
 - (FlutterClippingMaskView*)getMaskViewWithFrame:(CGRect)frame;
 
-// Mark all the maskViews available.
-- (void)recycleMaskViews;
+// Insert the `maskView` into the pool.
+- (void)insertViewToPoolIfNeeded:(FlutterClippingMaskView*)maskView;
 
 @end
 
@@ -291,27 +291,20 @@ class FlutterPlatformViewsController {
   int CountClips(const MutatorsStack& mutators_stack);
 
   void ClipViewSetMaskView(UIView* clipView);
+
   // Applies the mutators in the mutators_stack to the UIView chain that was constructed by
   // `ReconstructClipViewsChain`
   //
-  // Clips are applied to the super view with a CALayer mask. Transforms are applied to the
-  // current view that's at the head of the chain. For example the following mutators stack [T_1,
-  // C_2, T_3, T_4, C_5, T_6] where T denotes a transform and C denotes a clip, will result in the
-  // following UIView tree:
-  //
-  // C_2 -> C_5 -> PLATFORM_VIEW
-  // (PLATFORM_VIEW is a subview of C_5 which is a subview of C_2)
-  //
-  // T_1 is applied to C_2, T_3 and T_4 are applied to C_5, and T_6 is applied to PLATFORM_VIEW.
-  //
-  // After each clip operation, we update the head to the super view of the current head.
+  // Clips are applied to the `embedded_view`'s super view(|ChildClippingView|) using a
+  // |FlutterClippingMaskView|. Transforms are applied to `embedded_view`
   //
   // The `bounding_rect` is the final bounding rect of the PlatformView
   // (EmbeddedViewParams::finalBoundingRect). If a clip mutator's rect contains the final bounding
   // rect of the PlatformView, the clip mutator is not applied for performance optimization.
   void ApplyMutators(const MutatorsStack& mutators_stack,
                      UIView* embedded_view,
                      const SkRect& bounding_rect);
+
   void CompositeWithParams(int64_t view_id, const EmbeddedViewParams& params);
 
   // Allocates a new FlutterPlatformViewLayer if needed, draws the pixels within the rect from

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews_Internal.mm

@@ -9,6 +9,7 @@
 #import "flutter/shell/platform/darwin/ios/ios_surface.h"
 
 static int kMaxPointsInVerb = 4;
+static const NSUInteger kFlutterClippingMaskViewPoolCapacity = 5;
 
 namespace flutter {
 
@@ -26,7 +27,10 @@
 
 FlutterPlatformViewsController::FlutterPlatformViewsController()
     : layer_pool_(std::make_unique<FlutterPlatformViewLayerPool>()),
-      weak_factory_(std::make_unique<fml::WeakPtrFactory<FlutterPlatformViewsController>>(this)){};
+      weak_factory_(std::make_unique<fml::WeakPtrFactory<FlutterPlatformViewsController>>(this)) {
+  mask_view_pool_.reset(
+      [[FlutterClippingMaskViewPool alloc] initWithCapacity:kFlutterClippingMaskViewPoolCapacity]);
+};
 
 FlutterPlatformViewsController::~FlutterPlatformViewsController() = default;
 
@@ -458,58 +462,51 @@ @interface FlutterClippingMaskViewPool ()
 // The maximum number of `FlutterClippingMaskView` the pool can contain.
 // This prevents the pool to grow infinately and limits the maximum memory a pool can use.
 @property(assign, nonatomic) NSUInteger capacity;
-@property(retain, nonatomic) NSMutableArray<FlutterClippingMaskView*>* pool;
-// The index points to the first available FlutterClippingMaskView in the `pool`.
-@property(assign, nonatomic) NSUInteger availableIndex;
+
+// The pool contains the views that are available to use.
+// The number of items in the pool must not excceds `capacity`.
+@property(retain, nonatomic) NSMutableSet<FlutterClippingMaskView*>* pool;
 
 @end
 
 @implementation FlutterClippingMaskViewPool : NSObject
 
 - (instancetype)initWithCapacity:(NSInteger)capacity {
   if (self = [super init]) {
-    _pool = [[NSMutableArray alloc] initWithCapacity:capacity];
+    // Most of cases, there are only one PlatformView in the scene.
+    // Thus init with the capacity of 1.
+    _pool = [[NSMutableSet alloc] initWithCapacity:1];
     _capacity = capacity;
-    _availableIndex = 0;
   }
   return self;
 }
 
 - (FlutterClippingMaskView*)getMaskViewWithFrame:(CGRect)frame {
-  FML_DCHECK(self.availableIndex <= self.capacity);
-  FlutterClippingMaskView* maskView;
-  if (self.availableIndex == self.capacity) {
-    // The pool is full, alloc a new one.
-    maskView =
+  FML_DCHECK(self.pool.count <= self.capacity);
+  if (self.pool.count == 0) {
+    // The pool is empty, alloc a new one.
+    return
         [[[FlutterClippingMaskView alloc] initWithFrame:frame
                                             screenScale:[UIScreen mainScreen].scale] autorelease];
-    return maskView;
   }
-
-  if (self.availableIndex >= self.pool.count) {
-    // The pool doesn't have enough maskViews, alloc a new one and add to the pool.
-    maskView =
-        [[[FlutterClippingMaskView alloc] initWithFrame:frame
-                                            screenScale:[UIScreen mainScreen].scale] autorelease];
-    [self.pool addObject:maskView];
-    FML_DCHECK(self.pool.count <= self.capacity);
-  } else {
-    // Reuse a maskView from the pool.
-    maskView = [self.pool objectAtIndex:self.availableIndex];
-    maskView.frame = frame;
-    [maskView reset];
-  }
-  self.availableIndex++;
+  FlutterClippingMaskView* maskView = [[[self.pool anyObject] retain] autorelease];
+  maskView.frame = frame;
+  [maskView reset];
+  [self.pool removeObject:maskView];
   return maskView;
 }
 
-- (void)recycleMaskViews {
-  self.availableIndex = 0;
+- (void)insertViewToPoolIfNeeded:(FlutterClippingMaskView*)maskView {
+  FML_DCHECK(![self.pool containsObject:maskView]);
+  FML_DCHECK(self.pool.count <= self.capacity);
+  if (self.pool.count == self.capacity) {
+    return;
+  }
+  [self.pool addObject:maskView];
 }
 
 - (void)dealloc {
   [_pool release];
-  _pool = nil;
 
   [super dealloc];
 }