[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 · Mar 31, 2023 · f871ff3 · f871ff3
1 parent fe4d514
commit f871ff3
Show file tree

Hide file tree

Showing 6 changed files with 38 additions and 10 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;
@@ -34,6 +35,7 @@ void SetupBuilder()
 					handlers.AddHandler<Window, WindowHandlerStub>();
 					handlers.AddHandler<Frame, FrameRenderer>();
 					handlers.AddHandler<Label, LabelHandler>();
+					handlers.AddHandler<CollectionView, CollectionViewHandler>();
 				});
 			});
 		}
@@ -298,7 +300,7 @@ public async Task DoesNotLeak()
 
 			await CreateHandlerAndAddToWindow<WindowHandlerStub>(new Window(navPage), async (handler) =>
 			{
-				var page = new ContentPage { Title = "Page 2", Content = new VerticalStackLayout { new Label() } };
+				var page = new ContentPage { Title = "Page 2", Content = new VerticalStackLayout { new Label(), new CollectionView() } };
 				pageReference = new WeakReference(page);
 				await navPage.Navigation.PushAsync(page);
 				await navPage.Navigation.PopAsync();

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));
 				});
 			});
 		}
@@ -968,7 +970,7 @@ await CreateHandlerAndAddToWindow<ShellHandler>(shell, async (handler) =>
 			{
 				await OnLoadedAsync(shell.CurrentPage);
 
-				var page = new ContentPage { Title = "Page 2", Content = new VerticalStackLayout { new Label() } };
+				var page = new ContentPage { Title = "Page 2", Content = new VerticalStackLayout { new Label(), new CollectionView() } };
 				pageReference = new WeakReference(page);
 
 				await shell.Navigation.PushAsync(page);