From 6da3e40f3920dd7810a5b8d6774b689a35672685 Mon Sep 17 00:00:00 2001 From: Copilot <223556219+Copilot@users.noreply.github.com> Date: Thu, 16 Jul 2026 17:49:34 +0200 Subject: [PATCH] [leak-fix] Fix TransformGroup.Children memory leak (Fixes #36367) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rebased onto inflight/current, which already contains #36150's child-subscription fix using strong CollectionChanged/PropertyChanged subscriptions — the exact mechanism #36367 reports (a shared/long-lived TransformCollection roots the TransformGroup). Converts those subscriptions to WeakNotifyCollectionChangedProxy / WeakNotifyPropertyChangedProxy, preserving #36150's Clear()/Reset teardown (guarded by Shapes/TransformGroupTests). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e27685d0-fe80-460a-aa05-83d2ab9bf032 --- .../src/Core/Shapes/TransformGroup.cs | 183 ++++++------ .../TransformGroupMemoryTests.cs | 264 ++++++++++++++++++ 2 files changed, 364 insertions(+), 83 deletions(-) create mode 100644 src/Controls/tests/Core.UnitTests/TransformGroupMemoryTests.cs diff --git a/src/Controls/src/Core/Shapes/TransformGroup.cs b/src/Controls/src/Core/Shapes/TransformGroup.cs index 1d348ece6eaf..6ef5e7e96186 100644 --- a/src/Controls/src/Core/Shapes/TransformGroup.cs +++ b/src/Controls/src/Core/Shapes/TransformGroup.cs @@ -11,11 +11,10 @@ namespace Microsoft.Maui.Controls.Shapes [ContentProperty("Children")] public sealed class TransformGroup : Transform { - readonly Dictionary _subscribedTransforms = new(); - /// Bindable property for . public static readonly BindableProperty ChildrenProperty = - BindableProperty.Create(nameof(Children), typeof(TransformCollection), typeof(TransformGroup), null, propertyChanged: OnChildrenChanged); + BindableProperty.Create(nameof(Children), typeof(TransformCollection), typeof(TransformGroup), null, + propertyChanged: OnTransformGroupChanged); /// /// Initializes a new instance of the class. @@ -34,78 +33,64 @@ public TransformCollection Children get { return (TransformCollection)GetValue(ChildrenProperty); } } - static void OnChildrenChanged(BindableObject bindable, object oldValue, object newValue) + static void OnTransformGroupChanged(BindableObject bindable, object oldValue, object newValue) { - var transformGroup = (TransformGroup)bindable; - transformGroup.UpdateChildren( - oldValue as TransformCollection, - newValue as TransformCollection); + (bindable as TransformGroup)?.UpdateChildren(oldValue as TransformCollection, newValue as TransformCollection); } - void UpdateChildren(TransformCollection oldCollection, TransformCollection newCollection) - { - DetachCollection(oldCollection); - AttachCollection(newCollection); - - UpdateTransformMatrix(); - } + ChildrenSubscriptions _childrenSubscriptions; + NotifyCollectionChangedEventHandler _childrenCollectionChanged; + PropertyChangedEventHandler _childPropertyChanged; - void AttachCollection(TransformCollection collection) + void UpdateChildren(TransformCollection oldCollection, TransformCollection newCollection) { - if (collection is null) + if (oldCollection != null) { - return; + _childrenSubscriptions?.UnsubscribeAll(); + // Keep the empty helper for reuse; UnsubscribeAll releases every source and child proxy. } - collection.CollectionChanged += OnChildrenCollectionChanged; - - foreach (var transform in collection) + if (newCollection != null) { - SubscribeToTransformPropertyChanged(transform); - } - } + _childrenCollectionChanged ??= OnChildrenCollectionChanged; + _childPropertyChanged ??= OnTransformPropertyChanged; - void DetachCollection(TransformCollection collection) - { - if (collection is null) - { - return; + var subscriptions = _childrenSubscriptions ??= new ChildrenSubscriptions(); + subscriptions.Subscribe(newCollection, _childrenCollectionChanged, _childPropertyChanged); } - collection.CollectionChanged -= OnChildrenCollectionChanged; - - ClearAllTransformSubscriptions(); + UpdateTransformMatrix(); } void OnChildrenCollectionChanged(object sender, NotifyCollectionChangedEventArgs args) { if (args.Action == NotifyCollectionChangedAction.Reset) { - ClearAllTransformSubscriptions(); - - if (sender is TransformCollection collection) - { - foreach (INotifyPropertyChanged item in collection) - { - SubscribeToTransformPropertyChanged(item); - } - } + _childrenSubscriptions?.ResetChildren(); } - else + else if (args.Action != NotifyCollectionChangedAction.Move) { - if (args.OldItems is not null) + if (args.OldItems != null) { - foreach (INotifyPropertyChanged item in args.OldItems) + foreach (var oldItem in args.OldItems) { - UnsubscribeFromTransformPropertyChanged(item); + if (oldItem is Transform oldTransform) + { + _childrenSubscriptions?.Remove(oldTransform); + } } } - if (args.NewItems is not null) + if (args.NewItems != null) { - foreach (INotifyPropertyChanged item in args.NewItems) + _childPropertyChanged ??= OnTransformPropertyChanged; + + foreach (var newItem in args.NewItems) { - SubscribeToTransformPropertyChanged(item); + if (newItem is Transform newTransform) + { + _childrenSubscriptions?.Add(newTransform, _childPropertyChanged); + } } } } @@ -113,59 +98,91 @@ void OnChildrenCollectionChanged(object sender, NotifyCollectionChangedEventArgs UpdateTransformMatrix(); } - void SubscribeToTransformPropertyChanged(INotifyPropertyChanged item) + void OnTransformPropertyChanged(object sender, PropertyChangedEventArgs args) { - if (_subscribedTransforms.TryGetValue(item, out int count)) + UpdateTransformMatrix(); + } + + void UpdateTransformMatrix() + { + var matrix = new Matrix(); + + if (Children is not null) { - _subscribedTransforms[item] = count + 1; - return; + foreach (var child in Children) + { + if (child is not null) + matrix = Matrix.Multiply(matrix, child.Value); + } } - item.PropertyChanged += OnTransformPropertyChanged; - _subscribedTransforms[item] = 1; + Value = matrix; } - void UnsubscribeFromTransformPropertyChanged(INotifyPropertyChanged item) + // Keeps the CollectionChanged and per-child PropertyChanged subscriptions weak so a shared + // or long-lived TransformCollection cannot root the TransformGroup. The finalizer tears the + // subscriptions down, mirroring the pattern used by other WeakEventProxy owners. + sealed class ChildrenSubscriptions { - if (!_subscribedTransforms.TryGetValue(item, out int count)) + readonly WeakNotifyCollectionChangedProxy _collectionProxy = new(); + readonly List _childProxies = new(); + + ~ChildrenSubscriptions() => UnsubscribeAll(); + + public void Subscribe( + TransformCollection source, + NotifyCollectionChangedEventHandler collectionChangedHandler, + PropertyChangedEventHandler childPropertyChangedHandler) { - return; + _collectionProxy.Subscribe(source, collectionChangedHandler); + + foreach (var child in source) + { + if (child is not null) + { + Add(child, childPropertyChangedHandler); + } + } } - if (count > 1) + public void Add(Transform source, PropertyChangedEventHandler handler) { - _subscribedTransforms[item] = count - 1; - return; + _childProxies.Add(new WeakNotifyPropertyChangedProxy(source, handler)); } - item.PropertyChanged -= OnTransformPropertyChanged; - _subscribedTransforms.Remove(item); - } - - // Unsubscribes all tracked transforms from PropertyChanged and clears the dictionary. - void ClearAllTransformSubscriptions() - { - foreach (var item in _subscribedTransforms) + public void Remove(Transform source) { - item.Key.PropertyChanged -= OnTransformPropertyChanged; + for (int i = _childProxies.Count - 1; i >= 0; i--) + { + var proxy = _childProxies[i]; + if (proxy.TryGetSource(out var proxySource) && ReferenceEquals(proxySource, source)) + { + proxy.Unsubscribe(); + _childProxies.RemoveAt(i); + break; + } + } } - _subscribedTransforms.Clear(); - } - - void OnTransformPropertyChanged(object sender, PropertyChangedEventArgs args) - { - UpdateTransformMatrix(); - } + public void ResetChildren() + { + // TransformCollection is sealed, so Reset means the current children were cleared. + UnsubscribeChildren(); + } - void UpdateTransformMatrix() - { - var matrix = new Matrix(); + public void UnsubscribeAll() + { + _collectionProxy.Unsubscribe(); + UnsubscribeChildren(); + } - foreach (Transform child in Children) - matrix = Matrix.Multiply(matrix, child.Value); + void UnsubscribeChildren() + { + for (int i = 0; i < _childProxies.Count; i++) + _childProxies[i].Unsubscribe(); - Value = matrix; + _childProxies.Clear(); + } } } -} \ No newline at end of file +} diff --git a/src/Controls/tests/Core.UnitTests/TransformGroupMemoryTests.cs b/src/Controls/tests/Core.UnitTests/TransformGroupMemoryTests.cs new file mode 100644 index 000000000000..fd21493f25ac --- /dev/null +++ b/src/Controls/tests/Core.UnitTests/TransformGroupMemoryTests.cs @@ -0,0 +1,264 @@ +using System; +using System.Collections; +using System.Reflection; +using System.Runtime.CompilerServices; +using System.Threading.Tasks; +using Microsoft.Maui.Controls.Shapes; +using Xunit; + +namespace Microsoft.Maui.Controls.Core.UnitTests +{ + public class TransformGroupMemoryTests : BaseTestFixture + { + [MethodImpl(MethodImplOptions.NoInlining)] + static WeakReference GetChildProxyReference(TransformGroup group, int index) + { + return new WeakReference(GetChildProxies(group)[index]); + } + + static IList GetChildProxies(TransformGroup group) + { + var flags = BindingFlags.NonPublic | BindingFlags.Instance; + var subscriptionsField = typeof(TransformGroup).GetField("_childrenSubscriptions", flags); + Assert.NotNull(subscriptionsField); + + var subscriptions = subscriptionsField.GetValue(group); + Assert.NotNull(subscriptions); + + var childProxiesField = subscriptions.GetType().GetField("_childProxies", flags); + Assert.NotNull(childProxiesField); + + return Assert.IsAssignableFrom(childProxiesField.GetValue(subscriptions)); + } + + [Fact, Category(TestCategory.Memory)] + public async Task TransformGroupDoesNotLeakWhenSharingChildren() + { + // A long-lived/shared TransformCollection, exactly as the issue describes. + var sharedChildren = new TransformCollection + { + new ScaleTransform(2, 2) + }; + + WeakReference weakGroup; + { + var group = new TransformGroup(); + group.Children = sharedChildren; + weakGroup = new WeakReference(group); + } + + Assert.False(await weakGroup.WaitForCollect(), "TransformGroup should not be alive!"); + GC.KeepAlive(sharedChildren); + } + + [Fact, Category(TestCategory.Memory)] + public async Task ChildTransformChangesStillInvalidateAfterGc() + { + var child = new ScaleTransform(1, 1); + var group = new TransformGroup(); + group.Children.Add(child); + + await TestHelpers.Collect(); + + var before = group.Value; + child.ScaleX = 3; + var after = group.Value; + + Assert.NotEqual(before, after); + GC.KeepAlive(group); + } + + [Fact, Category(TestCategory.Memory)] + public async Task ExistingChildTransformChangesStillInvalidateAfterGc() + { + var child = new ScaleTransform(1, 1); + var group = new TransformGroup + { + Children = new TransformCollection { child } + }; + + await TestHelpers.Collect(); + + var before = group.Value; + child.ScaleX = 3; + var after = group.Value; + + Assert.NotEqual(before, after); + GC.KeepAlive(group); + } + + [Fact] + public void ReassigningChildrenMovesChangeSubscriptions() + { + var oldChild = new ScaleTransform(2, 2); + var oldChildren = new TransformCollection { oldChild }; + var replacementChild = new ScaleTransform(3, 3); + var replacementChildren = new TransformCollection { replacementChild }; + var group = new TransformGroup { Children = oldChildren }; + + group.Children = replacementChildren; + var replacementValue = group.Value; + + int valueChangeCount = 0; + group.PropertyChanged += (_, e) => + { + if (e.PropertyName == Transform.ValueProperty.PropertyName) + valueChangeCount++; + }; + + var sentinel = new Matrix(7, 0, 0, 11, 13, 17); + group.Value = sentinel; + valueChangeCount = 0; + + oldChild.ScaleX = 4; + Assert.Equal(sentinel, group.Value); + Assert.Equal(0, valueChangeCount); + + oldChildren.Add(new TranslateTransform(10, 20)); + Assert.Equal(sentinel, group.Value); + Assert.Equal(0, valueChangeCount); + + group.Value = replacementValue; + valueChangeCount = 0; + + replacementChild.ScaleX = 5; + replacementChildren.Add(new TranslateTransform(30, 40)); + + Assert.Equal(2, valueChangeCount); + } + + [Fact, Category(TestCategory.Memory)] + public async Task RemovingChildTransformReleasesSubscription() + { + var removed = new ScaleTransform(1, 1); + var retained = new ScaleTransform(1, 1); + var group = new TransformGroup + { + Children = new TransformCollection { removed, retained } + }; + var removedProxy = GetChildProxyReference(group, 0); + + group.Children.Remove(removed); + + Assert.False(await removedProxy.WaitForCollect(), "Removed child proxy should not be alive!"); + + var before = group.Value; + retained.ScaleX = 3; + var after = group.Value; + + Assert.NotEqual(before, after); + GC.KeepAlive(removed); + GC.KeepAlive(group); + } + + [Fact, Category(TestCategory.Memory)] + public async Task ReplacingChildTransformReleasesOldSubscription() + { + var replaced = new ScaleTransform(1, 1); + var replacement = new ScaleTransform(1, 1); + var group = new TransformGroup + { + Children = new TransformCollection { replaced } + }; + var replacedProxy = GetChildProxyReference(group, 0); + + group.Children[0] = replacement; + + Assert.False(await replacedProxy.WaitForCollect(), "Replaced child proxy should not be alive!"); + + var before = group.Value; + replacement.ScaleX = 3; + var after = group.Value; + + Assert.NotEqual(before, after); + GC.KeepAlive(replaced); + GC.KeepAlive(group); + } + + [Fact, Category(TestCategory.Memory)] + public async Task MovingChildTransformsReusesSubscriptions() + { + var scale = new ScaleTransform(2, 2); + var translate = new TranslateTransform(10, 20); + var group = new TransformGroup + { + Children = new TransformCollection { scale, translate } + }; + var proxies = GetChildProxies(group); + var scaleProxy = proxies[0]; + var translateProxy = proxies[1]; + var beforeMove = group.Value; + + group.Children.Move(0, 1); + + var afterMove = group.Value; + var movedProxies = GetChildProxies(group); + Assert.NotEqual(beforeMove, afterMove); + Assert.True(ReferenceEquals(scaleProxy, movedProxies[0]) || ReferenceEquals(scaleProxy, movedProxies[1])); + Assert.True(ReferenceEquals(translateProxy, movedProxies[0]) || ReferenceEquals(translateProxy, movedProxies[1])); + + await TestHelpers.Collect(); + + var beforeChange = group.Value; + scale.ScaleX = 3; + var afterChange = group.Value; + + Assert.NotEqual(beforeChange, afterChange); + GC.KeepAlive(group); + } + + [Fact, Category(TestCategory.Memory)] + public async Task ClearingChildrenReleasesSubscriptionsAndAllowsReuse() + { + var first = new ScaleTransform(1, 1); + var second = new ScaleTransform(1, 1); + var group = new TransformGroup + { + Children = new TransformCollection { first, second } + }; + var firstProxy = GetChildProxyReference(group, 0); + var secondProxy = GetChildProxyReference(group, 1); + + group.Children.Clear(); + + Assert.False(await firstProxy.WaitForCollect(), "Cleared child proxy should not be alive!"); + Assert.False(await secondProxy.WaitForCollect(), "Cleared child proxy should not be alive!"); + + var added = new ScaleTransform(1, 1); + group.Children.Add(added); + await TestHelpers.Collect(); + + var before = group.Value; + added.ScaleX = 3; + var after = group.Value; + + Assert.NotEqual(before, after); + GC.KeepAlive(first); + GC.KeepAlive(second); + GC.KeepAlive(group); + } + + [Fact] + public void NullChildrenUseIdentityMatrix() + { + var group = new TransformGroup + { + Children = null + }; + + Assert.Equal(new Matrix(), group.Value); + } + + [Fact] + public void NullChildIsIgnoredWhenUpdatingMatrix() + { + var transform = new TranslateTransform(10, 20); + var group = new TransformGroup + { + Children = new TransformCollection { null, transform } + }; + + Assert.Equal(transform.Value, group.Value); + } + } +}