Shapes: Fix TransformGroup child subscription leaks

src/Controls/src/Core/Shapes/TransformGroup.cs

@@ -1,4 +1,5 @@
 #nullable disable
+using System.Collections.Generic;
 using System.Collections.Specialized;
 using System.ComponentModel;
 
@@ -10,10 +11,11 @@ namespace Microsoft.Maui.Controls.Shapes
 	[ContentProperty("Children")]
 	public sealed class TransformGroup : Transform
 	{
+		readonly Dictionary<INotifyPropertyChanged, int> _subscribedTransforms = new();
+
 		/// <summary>Bindable property for <see cref="Children"/>.</summary>
 		public static readonly BindableProperty ChildrenProperty =
-			BindableProperty.Create(nameof(Children), typeof(TransformCollection), typeof(TransformGroup), null,
-				propertyChanged: OnTransformGroupChanged);
+			BindableProperty.Create(nameof(Children), typeof(TransformCollection), typeof(TransformGroup), null, propertyChanged: OnChildrenChanged);
 
 		/// <summary>
 		/// Initializes a new instance of the <see cref="TransformGroup"/> class.
@@ -32,38 +34,125 @@ public TransformCollection Children
 			get { return (TransformCollection)GetValue(ChildrenProperty); }
 		}
 
-		static void OnTransformGroupChanged(BindableObject bindable, object oldValue, object newValue)
+		static void OnChildrenChanged(BindableObject bindable, object oldValue, object newValue)
+		{
+			var transformGroup = (TransformGroup)bindable;
+			transformGroup.UpdateChildren(
+			 oldValue as TransformCollection,
+			 newValue as TransformCollection);
+		}
+
+		void UpdateChildren(TransformCollection oldCollection, TransformCollection newCollection)
 		{
-			if (oldValue != null)
+			DetachCollection(oldCollection);
+			AttachCollection(newCollection);
+
+			UpdateTransformMatrix();
+		}
+
+		void AttachCollection(TransformCollection collection)
+		{
+			if (collection is null)
+			{
+				return;
+			}
+
+			collection.CollectionChanged += OnChildrenCollectionChanged;
+
+			foreach (var transform in collection)
 			{
-				(oldValue as TransformCollection).CollectionChanged -= (bindable as TransformGroup).OnChildrenCollectionChanged;
+				SubscribeToTransformPropertyChanged(transform);
 			}
+		}
 
-			if (newValue != null)
+		void DetachCollection(TransformCollection collection)
+		{
+			if (collection is null)
 			{
-				(newValue as TransformCollection).CollectionChanged += (bindable as TransformGroup).OnChildrenCollectionChanged;
+				return;
 			}
 
-			(bindable as TransformGroup).UpdateTransformMatrix();
+			collection.CollectionChanged -= OnChildrenCollectionChanged;
+
+			ClearAllTransformSubscriptions();
 		}
 
 		void OnChildrenCollectionChanged(object sender, NotifyCollectionChangedEventArgs args)
 		{
-			if (args.NewItems != null)
-				foreach (INotifyPropertyChanged item in args.NewItems)
+			if (args.Action == NotifyCollectionChangedAction.Reset)
+			{
+				ClearAllTransformSubscriptions();
+
+				if (sender is TransformCollection collection)
+				{
+					foreach (INotifyPropertyChanged item in collection)
+					{
+						SubscribeToTransformPropertyChanged(item);
+					}
+				}
+			}
+			else
+			{
+				if (args.OldItems is not null)
 				{
-					item.PropertyChanged += OnTransformPropertyChanged;
+					foreach (INotifyPropertyChanged item in args.OldItems)
+					{
+						UnsubscribeFromTransformPropertyChanged(item);
+					}
 				}
 
-			if (args.OldItems != null)
-				foreach (INotifyPropertyChanged item in args.OldItems)
+				if (args.NewItems is not null)
 				{
-					item.PropertyChanged -= OnTransformPropertyChanged;
+					foreach (INotifyPropertyChanged item in args.NewItems)
+					{
+						SubscribeToTransformPropertyChanged(item);
+					}
 				}
+			}
 
 			UpdateTransformMatrix();
 		}
 
+		void SubscribeToTransformPropertyChanged(INotifyPropertyChanged item)
+		{
+			if (_subscribedTransforms.TryGetValue(item, out int count))
+			{
+				_subscribedTransforms[item] = count + 1;
+				return;
+			}
+
+			item.PropertyChanged += OnTransformPropertyChanged;
+			_subscribedTransforms[item] = 1;
+		}
+
+		void UnsubscribeFromTransformPropertyChanged(INotifyPropertyChanged item)
+		{
+			if (!_subscribedTransforms.TryGetValue(item, out int count))
+			{
+				return;
+			}
+
+			if (count > 1)
+			{
+				_subscribedTransforms[item] = count - 1;
+				return;
+			}
+
+			item.PropertyChanged -= OnTransformPropertyChanged;
+			_subscribedTransforms.Remove(item);
+		}
+
+		// Unsubscribes all tracked transforms from PropertyChanged and clears the dictionary.
+		void ClearAllTransformSubscriptions()
+		{
+			foreach (var item in _subscribedTransforms)
+			{
+				item.Key.PropertyChanged -= OnTransformPropertyChanged;
+			}
+
+			_subscribedTransforms.Clear();
+		}
+
 		void OnTransformPropertyChanged(object sender, PropertyChangedEventArgs args)
 		{
 			UpdateTransformMatrix();

src/Controls/tests/Core.UnitTests/Shapes/TransformGroupTests.cs

@@ -0,0 +1,87 @@
+using System;
+using System.ComponentModel;
+using System.Threading.Tasks;
+using Microsoft.Maui.Controls.Shapes;
+using Xunit;
+
+namespace Microsoft.Maui.Controls.Core.UnitTests.Shapes
+{
+	public class TransformGroupTests : BaseTestFixture
+	{
+		[Fact]
+		public async Task ReplacingChildrenUnsubscribesFromOldChildPropertyChanged()
+		{
+			var sharedTransform = new ScaleTransform { ScaleX = 1.0, ScaleY = 1.0 };
+			WeakReference weakGroup;
+
+			{
+				var group = new TransformGroup();
+				group.Children.Add(sharedTransform);
+				group.Children = new TransformCollection();
+				weakGroup = new WeakReference(group);
+			}
+
+			Assert.False(await weakGroup.WaitForCollect(),
+				"TransformGroup should be collected after Children replacement. " +
+				"Shared child transform is keeping it alive via stale PropertyChanged subscription.");
+		}
+
+		[Fact]
+		public void ReplacingChildrenSubscribesToNewChildPropertyChanged()
+		{
+			var group = new TransformGroup();
+			var newCollection = new TransformCollection();
+			var childTransform = new ScaleTransform { ScaleX = 1.0, ScaleY = 1.0 };
+			newCollection.Add(childTransform);
+
+			group.Children = newCollection;
+
+			var matrixBefore = group.Value;
+			childTransform.ScaleX = 2.0;
+			var matrixAfter = group.Value;
+
+			Assert.NotEqual(matrixBefore, matrixAfter);
+		}
+
+		[Fact]
+		public async Task SharedTransformDoesNotRetainMultipleGroups()
+		{
+			var sharedTransform = new ScaleTransform { ScaleX = 1.0, ScaleY = 1.0 };
+			var weakRefs = new WeakReference[10];
+
+			{
+				for (int i = 0; i < 10; i++)
+				{
+					var group = new TransformGroup();
+					group.Children.Add(sharedTransform);
+					group.Children = new TransformCollection();
+					weakRefs[i] = new WeakReference(group);
+				}
+			}
+
+			for (int i = 0; i < weakRefs.Length; i++)
+			{
+				Assert.False(await weakRefs[i].WaitForCollect(),
+					$"TransformGroup #{i} should be collected. Shared transform is retaining it.");
+			}
+		}
+
+		[Fact]
+		public async Task ClearingChildrenUnsubscribesAllTransforms()
+		{
+			var sharedTransform = new ScaleTransform { ScaleX = 1.0, ScaleY = 1.0 };
+			WeakReference weakGroup;
+
+			{
+				var group = new TransformGroup();
+				group.Children.Add(sharedTransform);
+				group.Children.Clear();
+				weakGroup = new WeakReference(group);
+			}
+
+			Assert.False(await weakGroup.WaitForCollect(),
+				"TransformGroup should be collected after Children.Clear(). " +
+				"Shared child transform is keeping it alive via stale PropertyChanged subscription.");
+		}
+	}
+}