From 0f421bf78f5ee39646ea2ca70ab1cc30b7ad21cc Mon Sep 17 00:00:00 2001
From: Jonathan Peppers <jonathan.peppers@microsoft.com>
Date: Wed, 5 Jun 2024 13:43:55 -0500
Subject: [PATCH] [ios/catalyst] fix memory leak in CollectionView cells

Fixes: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/2064274

In debugging a sample with a `<CollectionView/>` with large images
in each row, we found memory leaking only for certain objects:

* `Microsoft.Maui.Controls.Handlers.Items.MauiCollectionView`
* `Microsoft.Maui.Controls.Handlers.Items.VerticalCell`
* `Microsoft.Maui.Platform.MauiLabel`
* `Microsoft.Maui.Platform.MauiImageView`

It appears all of these types of objects were going away appropriately:

* `Microsoft.Maui.Controls.ContentPage`
* `Microsoft.Maui.Controls.CollectionView`
* `Microsoft.Maui.Controls.Label`
* `Microsoft.Maui.Controls.ImageView`

In #21850, #15831, and #14329 we fixed memory leaks in other various ways
related to `CollectionView`. But the above UIKit objects remain!

After a combination of using `dotnet-gcdump` and Instruments, we found:

* `ReorderableItemsViewController` referenced by
  * `EventHandler<EventArgs>` referenced by
    * `VerticalCell`

And so every `ReorderableItemsViewController` subscribes to:

    cell.ContentSizeChanged += CellContentSizeChanged;
    cell.LayoutAttributesChanged += CellLayoutAttributesChanged;

Creating a cycle:

    ReorderableItemsViewController -> VerticalCell -> ReorderableItemsViewController

This makes every `VerticalCell` never get collected, and so any
child platform views are also never collected. However, any MAUI-views
were already being collected successfully.

After some trial and error, I was able to reproduce this problem in
an existing device test.

The fix was to make both of the `ContentSizeChanged` and `LayoutAttributesChanged`
events backed by `WeakEventManager`.

I tried overriding `WillMoveToParentViewController` to unsubscribe, but
I didn't have a reference to the original cell.
---
 .../Handlers/Items/iOS/ItemsViewController.cs |  4 +-
 .../Core/Handlers/Items/iOS/TemplatedCell.cs  | 19 ++++-
 .../CollectionView/CollectionViewTests.cs     | 85 +++++++++++++------
 3 files changed, 74 insertions(+), 34 deletions(-)

diff --git a/src/Controls/src/Core/Handlers/Items/iOS/ItemsViewController.cs b/src/Controls/src/Core/Handlers/Items/iOS/ItemsViewController.cs
index f1b8a02df27b..452ae5d07d2d 100644
--- a/src/Controls/src/Core/Handlers/Items/iOS/ItemsViewController.cs
+++ b/src/Controls/src/Core/Handlers/Items/iOS/ItemsViewController.cs
@@ -377,7 +377,7 @@ protected object GetItemAtIndex(NSIndexPath index)
 			return ItemsSource[index];
 		}
 
-		[UnconditionalSuppressMessage("Memory", "MEM0003", Justification = "Proven safe in test: MemoryTests.HandlerDoesNotLeak")]
+		[UnconditionalSuppressMessage("Memory", "MEM0003", Justification = "Proven safe in test: CollectionViewTests.ItemsSourceDoesNotLeak")]
 		void CellContentSizeChanged(object sender, EventArgs e)
 		{
 			if (_disposed)
@@ -400,7 +400,7 @@ void CellContentSizeChanged(object sender, EventArgs e)
 			}
 		}
 
-		[UnconditionalSuppressMessage("Memory", "MEM0003", Justification = "Proven safe in test: MemoryTests.HandlerDoesNotLeak")]
+		[UnconditionalSuppressMessage("Memory", "MEM0003", Justification = "Proven safe in test: CollectionViewTests.ItemsSourceDoesNotLeak")]
 		void CellLayoutAttributesChanged(object sender, LayoutAttributesChangedEventArgs args)
 		{
 			CacheCellAttributes(args.NewAttributes.IndexPath, args.NewAttributes.Size);
diff --git a/src/Controls/src/Core/Handlers/Items/iOS/TemplatedCell.cs b/src/Controls/src/Core/Handlers/Items/iOS/TemplatedCell.cs
index e461b4b9c396..17baa2f40c1e 100644
--- a/src/Controls/src/Core/Handlers/Items/iOS/TemplatedCell.cs
+++ b/src/Controls/src/Core/Handlers/Items/iOS/TemplatedCell.cs
@@ -11,8 +11,19 @@ namespace Microsoft.Maui.Controls.Handlers.Items
 {
 	public abstract class TemplatedCell : ItemsViewCell
 	{
-		public event EventHandler<EventArgs> ContentSizeChanged;
-		public event EventHandler<LayoutAttributesChangedEventArgs> LayoutAttributesChanged;
+		readonly WeakEventManager _weakEventManager = new();
+
+		public event EventHandler<EventArgs> ContentSizeChanged
+		{
+			add => _weakEventManager.AddEventHandler(value);
+			remove => _weakEventManager.RemoveEventHandler(value);
+		}
+
+		public event EventHandler<LayoutAttributesChangedEventArgs> LayoutAttributesChanged
+		{
+			add => _weakEventManager.AddEventHandler(value);
+			remove => _weakEventManager.RemoveEventHandler(value);
+		}
 
 		protected CGSize ConstrainedSize;
 
@@ -287,12 +298,12 @@ void MeasureInvalidated(object sender, EventArgs args)
 
 		protected void OnContentSizeChanged()
 		{
-			ContentSizeChanged?.Invoke(this, EventArgs.Empty);
+			_weakEventManager.HandleEvent(this, EventArgs.Empty, nameof(ContentSizeChanged));
 		}
 
 		protected void OnLayoutAttributesChanged(UICollectionViewLayoutAttributes newAttributes)
 		{
-			LayoutAttributesChanged?.Invoke(this, new LayoutAttributesChangedEventArgs(newAttributes));
+			_weakEventManager.HandleEvent(this, new LayoutAttributesChangedEventArgs(newAttributes), nameof(LayoutAttributesChanged));
 		}
 
 		protected abstract bool AttributesConsistentWithConstrainedDimension(UICollectionViewLayoutAttributes attributes);
diff --git a/src/Controls/tests/DeviceTests/Elements/CollectionView/CollectionViewTests.cs b/src/Controls/tests/DeviceTests/Elements/CollectionView/CollectionViewTests.cs
index 36ad00303c9d..d48ec956e48d 100644
--- a/src/Controls/tests/DeviceTests/Elements/CollectionView/CollectionViewTests.cs
+++ b/src/Controls/tests/DeviceTests/Elements/CollectionView/CollectionViewTests.cs
@@ -6,6 +6,7 @@
 using System.Reflection;
 using System.Threading.Tasks;
 using Microsoft.Maui.Controls;
+using Microsoft.Maui.Controls.Handlers.Compatibility;
 using Microsoft.Maui.Controls.Handlers.Items;
 using Microsoft.Maui.Controls.Platform;
 using Microsoft.Maui.DeviceTests.Stubs;
@@ -41,7 +42,11 @@ void SetupBuilder()
 					handlers.AddHandler<Button, ButtonHandler>();
 					handlers.AddHandler<SwipeView, SwipeViewHandler>();
 					handlers.AddHandler<SwipeItem, SwipeItemMenuItemHandler>();
-
+#if IOS || MACCATALYST
+					handlers.AddHandler(typeof(NavigationPage), typeof(NavigationRenderer));
+#else
+					handlers.AddHandler(typeof(NavigationPage), typeof(NavigationViewHandler));
+#endif
 #if IOS && !MACCATALYST
 					handlers.AddHandler<CacheTestCollectionView, CacheTestCollectionViewHandler>();
 #endif
@@ -102,40 +107,64 @@ public async Task ItemsSourceDoesNotLeak()
 		{
 			SetupBuilder();
 
-			IList logicalChildren = null;
-			WeakReference weakReference = null;
-			var collectionView = new CollectionView
-			{
-				Header = new Label { Text = "Header" },
-				Footer = new Label { Text = "Footer" },
-				ItemTemplate = new DataTemplate(() => new Label())
-			};
+			var weakReferences = new List<WeakReference>();
 
-			await CreateHandlerAndAddToWindow<CollectionViewHandler>(collectionView, async handler =>
 			{
-				var data = new ObservableCollection<string>()
+				var labels = new List<Label>();
+				IList logicalChildren = null;
+				var collectionView = new CollectionView
 				{
-					"Item 1",
-					"Item 2",
-					"Item 3"
+					Header = new Label { Text = "Header" },
+					Footer = new Label { Text = "Footer" },
+					ItemTemplate = new DataTemplate(() => 
+					{
+						var label = new Label();
+						labels.Add(label);
+						return label;
+					}),
 				};
-				weakReference = new WeakReference(data);
-				collectionView.ItemsSource = data;
-				await Task.Delay(100);
 
-				// Get ItemsView._logicalChildren
-				var flags = BindingFlags.NonPublic | BindingFlags.Instance;
-				logicalChildren = typeof(Element).GetField("_internalChildren", flags).GetValue(collectionView) as IList;
-				Assert.NotNull(logicalChildren);
+				var navPage = new NavigationPage(new ContentPage { Title = "Page 1" });
 
-				// Replace with cloned collection
-				collectionView.ItemsSource = new ObservableCollection<string>(data);
-				await Task.Delay(100);
-			});
+				await CreateHandlerAndAddToWindow<WindowHandlerStub>(new Window(navPage), async handler =>
+				{
+					await navPage.PushAsync(new ContentPage { Content = collectionView });
+
+					var data = new ObservableCollection<string>()
+					{
+						"Item 1",
+						"Item 2",
+						"Item 3"
+					};
+					weakReferences.Add(new(data));
+					collectionView.ItemsSource = data;
+					await Task.Delay(100);
+
+					Assert.NotEmpty(labels);
+					foreach (var label in labels)
+					{
+						weakReferences.Add(new(label));
+						weakReferences.Add(new(label.Handler));
+						weakReferences.Add(new(label.Handler.PlatformView));
+					}
+
+					// Get ItemsView._logicalChildren
+					var flags = BindingFlags.NonPublic | BindingFlags.Instance;
+					logicalChildren = typeof(Element).GetField("_internalChildren", flags).GetValue(collectionView) as IList;
+					Assert.NotNull(logicalChildren);
+
+					// Replace with cloned collection
+					collectionView.ItemsSource = new ObservableCollection<string>(data);
+					await Task.Delay(100);
+					await navPage.PopAsync();
+				});
+
+				
+				Assert.NotNull(logicalChildren);
+				Assert.True(logicalChildren.Count <= 5, "_logicalChildren should not grow in size!");
+			}
 
-			await AssertionExtensions.WaitForGC(weakReference);
-			Assert.NotNull(logicalChildren);
-			Assert.True(logicalChildren.Count <= 5, "_logicalChildren should not grow in size!");
+			await AssertionExtensions.WaitForGC([.. weakReferences]);
 		}
 
 		[Theory]