[controls] fix memory leak with Grid Row/ColumnDefinitions (#16145)

jonathanpeppers · web-flow · commit 443d3a028b77 · 2023-07-17T20:37:07.000-05:00
Context: #15860 In a customer's app, they have the setup:  <Style TargetType="Grid" x:Key="GridStyleWithColumnDefinitions"> <Setter Property="ColumnDefinitions" Value="18,*"/> </Style> Then a page with: <Grid Style="{StaticResource GridStyleWithColumnDefinitions}" /> Navigating forward & back from this page would show that the entire `Page` would live forever! What is interesting, is simply removing the Grid's `Style` solved the problem?!? I narrowed this down to a unit test: [Fact] public async Task ColumnDefinitionDoesNotLeak() { // Long-lived column, like from a Style in App.Resources var column = new ColumnDefinition(); WeakReference reference; { var grid = new Grid(); grid.ColumnDefinitions.Add(column); reference = new(grid); } await Task.Yield(); GC.Collect(); GC.WaitForPendingFinalizers(); Assert.False(reference.IsAlive, "Grid should not be alive!"); } https://github.com/dotnet/maui/blob/cda3eb3381cfb686567ed05e3eb8e8f26c02d785/src/Controls/src/Core/Layout/Grid.cs#L20 `Grid` subscribes to `DefinitionCollection<T>.ItemSizeChanged`, and in this case the `DefinitionCollection<T>` lived indefinitely in an application-wide `ResourceDictionary`. The simplest solution is to make the `ItemSizeChanged` event use `WeakEventManager` as was done for other events like `Application.RequestedThemeChanged`: https://github.com/dotnet/maui/blob/cda3eb3381cfb686567ed05e3eb8e8f26c02d785/src/Controls/src/Core/Application/Application.cs#L221-L225 Since MAUI owns the property, we can use the simple solution here. This is one issue solved in the customer app, but I will need to retest the app in #15860 to see if there are further issues.
diff --git a/src/Controls/src/Core/DefinitionCollection.cs b/src/Controls/src/Core/DefinitionCollection.cs
@@ -7,6 +7,7 @@ namespace Microsoft.Maui.Controls
 {
 	public class DefinitionCollection<T> : IList<T>, ICollection<T> where T : IDefinition
 	{
+		readonly WeakEventManager _weakEventManager = new WeakEventManager();
 		readonly List<T> _internalList;
 
 		internal DefinitionCollection() => _internalList = new List<T>();
@@ -101,13 +102,15 @@ public void RemoveAt(int index)
 			OnItemSizeChanged(this, EventArgs.Empty);
 		}
 
-		public event EventHandler ItemSizeChanged;
+		public event EventHandler ItemSizeChanged
+		{
+			add => _weakEventManager.AddEventHandler(value);
+			remove => _weakEventManager.RemoveEventHandler(value);
+		}
 
 		void OnItemSizeChanged(object sender, EventArgs e)
 		{
-			EventHandler eh = ItemSizeChanged;
-			if (eh != null)
-				eh(this, EventArgs.Empty);
+			_weakEventManager.HandleEvent(this, e, nameof(ItemSizeChanged));
 		}
 	}
 }
diff --git a/src/Controls/tests/Core.UnitTests/Layouts/GridLayoutTests.cs b/src/Controls/tests/Core.UnitTests/Layouts/GridLayoutTests.cs
@@ -1,4 +1,6 @@
-﻿using System.Collections.Generic;
+﻿using System;
+using System.Collections.Generic;
+using System.Threading.Tasks;
 using NSubstitute;
 using Xunit;
 
@@ -184,5 +186,45 @@ public void ColumnDefinitionsGetBindingContext()
 
 			Assert.Equal(def2.BindingContext, context);
 		}
+
+		[Fact]
+		public async Task ColumnDefinitionDoesNotLeak()
+		{
+			// Long-lived column, like from a Style in App.Resources
+			var column = new ColumnDefinition();
+			WeakReference reference;
+
+			{
+				var grid = new Grid();
+				grid.ColumnDefinitions.Add(column);
+				reference = new(grid);
+			}
+
+			await Task.Yield();
+			GC.Collect();
+			GC.WaitForPendingFinalizers();
+
+			Assert.False(reference.IsAlive, "Grid should not be alive!");
+		}
+
+		[Fact]
+		public async Task RowDefinitionDoesNotLeak()
+		{
+			// Long-lived row, like from a Style in App.Resources
+			var row = new RowDefinition();
+			WeakReference reference;
+
+			{
+				var grid = new Grid();
+				grid.RowDefinitions.Add(row);
+				reference = new(grid);
+			}
+
+			await Task.Yield();
+			GC.Collect();
+			GC.WaitForPendingFinalizers();
+
+			Assert.False(reference.IsAlive, "Grid should not be alive!");
+		}
 	}
 }

Original file line number	Diff line number	Diff line change
`@@ -7,6 +7,7 @@ namespace Microsoft.Maui.Controls`
`7`	`7`	`{`
`8`	`8`	`public class DefinitionCollection<T> : IList<T>, ICollection<T> where T : IDefinition`
`9`	`9`	`{`
	`10`	`+ readonly WeakEventManager _weakEventManager = new WeakEventManager();`
`10`	`11`	`readonly List<T> _internalList;`
`11`	`12`
`12`	`13`	`internal DefinitionCollection() => _internalList = new List<T>();`
`@@ -101,13 +102,15 @@ public void RemoveAt(int index)`
`101`	`102`	`OnItemSizeChanged(this, EventArgs.Empty);`
`102`	`103`	`}`
`103`	`104`
`104`		`- public event EventHandler ItemSizeChanged;`
	`105`	`+ public event EventHandler ItemSizeChanged`
	`106`	`+ {`
	`107`	`+ add => _weakEventManager.AddEventHandler(value);`
	`108`	`+ remove => _weakEventManager.RemoveEventHandler(value);`
	`109`	`+ }`
`105`	`110`
`106`	`111`	`void OnItemSizeChanged(object sender, EventArgs e)`
`107`	`112`	`{`
`108`		`- EventHandler eh = ItemSizeChanged;`
`109`		`- if (eh != null)`
`110`		`- eh(this, EventArgs.Empty);`
	`113`	`+ _weakEventManager.HandleEvent(this, e, nameof(ItemSizeChanged));`
`111`	`114`	`}`
`112`	`115`	`}`
`113`	`116`	`}`