[controls] fix memory leak in CollectionView

Fixes: #10578 Context: https://github.com/Vroomer/MAUI-navigation-memory-leak.git After testing the above sample, I found that adding a `CollectionView` to a `Page`, makes it and the entire page live forever. This is WIP, as it is still failing on iOS/Catalyst. This fixes Android & Windows so far.
dotnet · Apr 10, 2023 · 6f74a43 · 6f74a43
1 parent 523653d
commit 6f74a43
Show file tree

Hide file tree

Showing 6 changed files with 46 additions and 12 deletions.
diff --git a/src/Controls/src/Core/Handlers/Items/Android/MauiRecyclerView.cs b/src/Controls/src/Core/Handlers/Items/Android/MauiRecyclerView.cs
@@ -42,6 +42,10 @@ public class MauiRecyclerView<TItemsView, TAdapter, TItemsViewSource> : Recycler
 
 		ItemTouchHelper _itemTouchHelper;
 		SimpleItemTouchHelperCallback _itemTouchHelperCallback;
+		WeakNotifyPropertyChangedProxy _layoutPropertyChangedProxy;
+		PropertyChangedEventHandler _layoutPropertyChanged;
+
+		~MauiRecyclerView() => _layoutPropertyChangedProxy?.Unsubscribe();
 
 		public MauiRecyclerView(Context context, Func<IItemsLayout> getItemsLayout, Func<TAdapter> getAdapter) : base(context)
 		{
@@ -58,9 +62,10 @@ public MauiRecyclerView(Context context, Func<IItemsLayout> getItemsLayout, Func
 		public virtual void TearDownOldElement(TItemsView oldElement)
 		{
 			// Stop listening for layout property changes
-			if (ItemsLayout != null)
+			if (_layoutPropertyChangedProxy is not null)
 			{
-				ItemsLayout.PropertyChanged -= LayoutPropertyChanged;
+				_layoutPropertyChangedProxy.Unsubscribe();
+				_layoutPropertyChanged = null;
 			}
 
 			// Stop listening for ScrollTo requests
@@ -283,14 +288,16 @@ public virtual void UpdateCanReorderItems()
 
 		public virtual void UpdateLayoutManager()
 		{
-			if (ItemsLayout != null)
-				ItemsLayout.PropertyChanged -= LayoutPropertyChanged;
+			_layoutPropertyChangedProxy?.Unsubscribe();
 
 			ItemsLayout = _getItemsLayout();
 
 			// Keep track of the ItemsLayout's property changes
 			if (ItemsLayout != null)
-				ItemsLayout.PropertyChanged += LayoutPropertyChanged;
+			{
+				_layoutPropertyChanged ??= LayoutPropertyChanged;
+				_layoutPropertyChangedProxy = new WeakNotifyPropertyChangedProxy(ItemsLayout, _layoutPropertyChanged);
+			}
 
 			SetLayoutManager(SelectLayoutManager(ItemsLayout));
 

diff --git a/src/Controls/src/Core/Handlers/Items/StructuredItemsViewHandler.Windows.cs b/src/Controls/src/Core/Handlers/Items/StructuredItemsViewHandler.Windows.cs
@@ -16,6 +16,10 @@ public partial class StructuredItemsViewHandler<TItemsView> : ItemsViewHandler<T
 	{
 		View _currentHeader;
 		View _currentFooter;
+		WeakNotifyPropertyChangedProxy _layoutPropertyChangedProxy;
+		PropertyChangedEventHandler _layoutPropertyChanged;
+
+		~StructuredItemsViewHandler() => _layoutPropertyChangedProxy?.Unsubscribe();
 
 		protected override IItemsLayout Layout { get => ItemsView?.ItemsLayout; }
 
@@ -24,15 +28,26 @@ protected override void ConnectHandler(ListViewBase platformView)
 			base.ConnectHandler(platformView);
 
 			if (Layout is not null)
-				Layout.PropertyChanged += LayoutPropertyChanged;
+			{
+				_layoutPropertyChanged ??= LayoutPropertyChanged;
+				_layoutPropertyChangedProxy = new WeakNotifyPropertyChangedProxy(Layout, _layoutPropertyChanged);
+			}
+			else if (_layoutPropertyChangedProxy is not null)
+			{
+				_layoutPropertyChangedProxy.Unsubscribe();
+				_layoutPropertyChangedProxy = null;
+			}
 		}
 
 		protected override void DisconnectHandler(ListViewBase platformView)
 		{
 			base.DisconnectHandler(platformView);
 
-			if (Layout is not null)
-				Layout.PropertyChanged -= LayoutPropertyChanged;
+			if (_layoutPropertyChangedProxy is not null)
+			{
+				_layoutPropertyChangedProxy.Unsubscribe();
+				_layoutPropertyChangedProxy = null;
+			}
 		}
 
 		void LayoutPropertyChanged(object sender, PropertyChangedEventArgs e)

diff --git a/src/Controls/src/Core/PublicAPI/net-android/PublicAPI.Unshipped.txt b/src/Controls/src/Core/PublicAPI/net-android/PublicAPI.Unshipped.txt
@@ -3,6 +3,7 @@
 Microsoft.Maui.Controls.Border.~Border() -> void
 Microsoft.Maui.Controls.Handlers.Compatibility.FrameRenderer.FrameRenderer(Android.Content.Context! context, Microsoft.Maui.IPropertyMapper! mapper) -> void
 Microsoft.Maui.Controls.Handlers.Compatibility.FrameRenderer.FrameRenderer(Android.Content.Context! context, Microsoft.Maui.IPropertyMapper! mapper, Microsoft.Maui.CommandMapper! commandMapper) -> void
+Microsoft.Maui.Controls.Handlers.Items.MauiRecyclerView<TItemsView, TAdapter, TItemsViewSource>.~MauiRecyclerView() -> void
 Microsoft.Maui.Controls.LayoutOptions.Equals(Microsoft.Maui.Controls.LayoutOptions other) -> bool
 Microsoft.Maui.Controls.Region.Equals(Microsoft.Maui.Controls.Region other) -> bool
 Microsoft.Maui.Controls.Shapes.Matrix.Equals(Microsoft.Maui.Controls.Shapes.Matrix other) -> bool

diff --git a/src/Controls/src/Core/PublicAPI/net-windows/PublicAPI.Unshipped.txt b/src/Controls/src/Core/PublicAPI/net-windows/PublicAPI.Unshipped.txt
@@ -1,6 +1,7 @@
 #nullable enable
 *REMOVED*override Microsoft.Maui.Controls.RefreshView.MeasureOverride(double widthConstraint, double heightConstraint) -> Microsoft.Maui.Graphics.Size
 Microsoft.Maui.Controls.Border.~Border() -> void
+Microsoft.Maui.Controls.Handlers.Items.StructuredItemsViewHandler<TItemsView>.~StructuredItemsViewHandler() -> void
 override Microsoft.Maui.Controls.Handlers.BoxViewHandler.NeedsContainer.get -> bool
 Microsoft.Maui.Controls.Handlers.BoxViewHandler
 Microsoft.Maui.Controls.Handlers.BoxViewHandler.BoxViewHandler() -> void

diff --git a/src/Controls/tests/DeviceTests/Elements/NavigationPage/NavigationPageTests.cs b/src/Controls/tests/DeviceTests/Elements/NavigationPage/NavigationPageTests.cs
@@ -5,6 +5,7 @@
 using System.Threading.Tasks;
 using Microsoft.Maui.Controls;
 using Microsoft.Maui.Controls.Handlers.Compatibility;
+using Microsoft.Maui.Controls.Handlers.Items;
 using Microsoft.Maui.DeviceTests.Stubs;
 using Microsoft.Maui.Handlers;
 using Microsoft.Maui.Hosting;
@@ -35,6 +36,7 @@ void SetupBuilder()
 					handlers.AddHandler<Frame, FrameRenderer>();
 					handlers.AddHandler<Label, LabelHandler>();
 					handlers.AddHandler<Button, ButtonHandler>();
+					handlers.AddHandler<CollectionView, CollectionViewHandler>();
 				});
 			});
 		}
@@ -306,16 +308,19 @@ await CreateHandlerAndAddToWindow<WindowHandlerStub>(new Window(navPage), async
 					{
 						new Label(),
 						new Button(),
+						new CollectionView(),
 					}
 				};
 				pageReference = new WeakReference(page);
 				await navPage.Navigation.PushAsync(page);
 				await navPage.Navigation.PopAsync();
 			});
 
-			// 3 GCs were required in Android API 23, 2 worked otherwise
-			for (int i = 0; i < 3; i++)
+			// As we add more controls to this test, more GCs will be required
+			for (int i = 0; i < 16; i++)
 			{
+				if (!pageReference.IsAlive)
+					break;
 				await Task.Yield();
 				GC.Collect();
 				GC.WaitForPendingFinalizers();

diff --git a/src/Controls/tests/DeviceTests/Elements/Shell/ShellTests.cs b/src/Controls/tests/DeviceTests/Elements/Shell/ShellTests.cs
@@ -8,6 +8,7 @@
 using Microsoft.Maui.Controls;
 using Microsoft.Maui.Controls.Handlers;
 using Microsoft.Maui.Controls.Handlers.Compatibility;
+using Microsoft.Maui.Controls.Handlers.Items;
 using Microsoft.Maui.Devices;
 using Microsoft.Maui.DeviceTests.Stubs;
 using Microsoft.Maui.Handlers;
@@ -34,6 +35,7 @@ void SetupBuilder()
 					SetupShellHandlers(handlers);
 					handlers.AddHandler(typeof(NavigationPage), typeof(NavigationViewHandler));
 					handlers.AddHandler(typeof(Button), typeof(ButtonHandler));
+					handlers.AddHandler(typeof(CollectionView), typeof(CollectionViewHandler));
 				});
 			});
 		}
@@ -975,6 +977,7 @@ await CreateHandlerAndAddToWindow<ShellHandler>(shell, async (handler) =>
 					{
 						new Label(),
 						new Button(),
+						new CollectionView(),
 					}
 				};
 				pageReference = new WeakReference(page);
@@ -983,9 +986,11 @@ await CreateHandlerAndAddToWindow<ShellHandler>(shell, async (handler) =>
 				await shell.Navigation.PopAsync();
 			});
 
-			// 3 GCs were required in Android API 23, 2 worked otherwise
-			for (int i = 0; i < 3; i++)
+			// As we add more controls to this test, more GCs will be required
+			for (int i = 0; i < 16; i++)
 			{
+				if (!pageReference.IsAlive)
+					break;
 				await Task.Yield();
 				GC.Collect();
 				GC.WaitForPendingFinalizers();