diff --git a/src/Compatibility/Core/src/Tizen/Extensions/GeometryExtensions.cs b/src/Compatibility/Core/src/Tizen/Extensions/GeometryExtensions.cs index 072d623e9399..81fc0f7727f1 100644 --- a/src/Compatibility/Core/src/Tizen/Extensions/GeometryExtensions.cs +++ b/src/Compatibility/Core/src/Tizen/Extensions/GeometryExtensions.cs @@ -68,8 +68,15 @@ static SKPath MakePath(GeometryGroup geometryGroup) var path = new SKPath(); path.FillType = geometryGroup.FillRule == FillRule.Nonzero ? SKPathFillType.Winding : SKPathFillType.EvenOdd; - foreach (Geometry child in geometryGroup.Children) + var children = geometryGroup.Children; + if (children is null) + return path; + + foreach (Geometry child in children) { + if (child is null) + continue; + #pragma warning disable IL2026 SKPath childPath = MakePath((dynamic)child); #pragma warning disable IL2026 diff --git a/src/Compatibility/Core/src/iOS/Extensions/GeometryExtensions.cs b/src/Compatibility/Core/src/iOS/Extensions/GeometryExtensions.cs index d55aae0b5566..b3ec077ef425 100644 --- a/src/Compatibility/Core/src/iOS/Extensions/GeometryExtensions.cs +++ b/src/Compatibility/Core/src/iOS/Extensions/GeometryExtensions.cs @@ -57,8 +57,15 @@ public static PathData ToCGPath(this Geometry geometry, Transform renderTransfor pathData.IsNonzeroFillRule = geometryGroup.FillRule == FillRule.Nonzero; - foreach (Geometry child in geometryGroup.Children) + var children = geometryGroup.Children; + if (children is null) + return pathData; + + foreach (Geometry child in children) { + if (child is null) + continue; + PathData pathChild = child.ToCGPath(renderTransform); pathData.Data.AddPath(pathChild.Data); } diff --git a/src/Controls/src/Core/Compatibility/Android/Extensions/GeometryExtensions.cs b/src/Controls/src/Core/Compatibility/Android/Extensions/GeometryExtensions.cs index 8a8e9721bdcc..a42d8a6be3a6 100644 --- a/src/Controls/src/Core/Compatibility/Android/Extensions/GeometryExtensions.cs +++ b/src/Controls/src/Core/Compatibility/Android/Extensions/GeometryExtensions.cs @@ -58,8 +58,15 @@ public static APath ToAPath(this Geometry geometry, Context context) path.SetFillType(geometryGroup.FillRule == FillRule.Nonzero ? APath.FillType.Winding : APath.FillType.EvenOdd); - foreach (Geometry child in geometryGroup.Children) + var children = geometryGroup.Children; + if (children is null) + return path; + + foreach (Geometry child in children) { + if (child is null) + continue; + APath childPath = child.ToAPath(context); path.AddPath(childPath); } diff --git a/src/Controls/src/Core/Platform/Windows/Extensions/GeometryExtensions.cs b/src/Controls/src/Core/Platform/Windows/Extensions/GeometryExtensions.cs index d15785ff306c..4e485b5ec727 100644 --- a/src/Controls/src/Core/Platform/Windows/Extensions/GeometryExtensions.cs +++ b/src/Controls/src/Core/Platform/Windows/Extensions/GeometryExtensions.cs @@ -47,9 +47,16 @@ public static WMedia.Geometry ToPlatform(this Geometry geometry) FillRule = ConvertFillRule(geometryGroup.FillRule) }; - foreach (Geometry children in geometryGroup.Children) + var children = geometryGroup.Children; + if (children is null) + return wGeometry; + + foreach (Geometry child in children) { - WMedia.Geometry winChild = children.ToPlatform(); + if (child is null) + continue; + + WMedia.Geometry winChild = child.ToPlatform(); (wGeometry as WMedia.GeometryGroup).Children.Add(winChild); } } diff --git a/src/Controls/src/Core/Shapes/GeometryGroup.cs b/src/Controls/src/Core/Shapes/GeometryGroup.cs index bcfaa8469004..26863ca50679 100644 --- a/src/Controls/src/Core/Shapes/GeometryGroup.cs +++ b/src/Controls/src/Core/Shapes/GeometryGroup.cs @@ -12,8 +12,6 @@ namespace Microsoft.Maui.Controls.Shapes [ContentProperty("Children")] public class GeometryGroup : Geometry { - readonly Dictionary _subscriptionRefCounts = new(); - /// Bindable property for . public static readonly BindableProperty ChildrenProperty = BindableProperty.Create(nameof(Children), typeof(GeometryCollection), typeof(GeometryGroup), null, @@ -56,148 +54,116 @@ public FillRule FillRule public event EventHandler InvalidateGeometryRequested; - void UpdateChildren(GeometryCollection oldCollection, GeometryCollection newCollection) - { - DetachCollection(oldCollection); - AttachCollection(newCollection); + ChildrenSubscriptions _childrenSubscriptions; + NotifyCollectionChangedEventHandler _childrenCollectionChanged; + PropertyChangedEventHandler _childrenPropertyChanged; - Invalidate(); - } - - void AttachCollection(GeometryCollection collection) + void UpdateChildren(GeometryCollection oldCollection, GeometryCollection newCollection) { - if (collection == null) - return; - - collection.CollectionChanged += OnChildrenCollectionChanged; - - foreach (var geometry in collection) + if (oldCollection != null) { - SubscribeToGeometry(geometry); + _childrenSubscriptions?.UnsubscribeAll(); } - } - void DetachCollection(GeometryCollection collection) - { - if (collection == null) + if (newCollection == null) return; - collection.CollectionChanged -= OnChildrenCollectionChanged; + _childrenCollectionChanged ??= OnChildrenCollectionChanged; + _childrenPropertyChanged ??= OnChildrenPropertyChanged; + + var subscriptions = _childrenSubscriptions ??= new ChildrenSubscriptions(); + subscriptions.Subscribe(newCollection, _childrenCollectionChanged); - foreach (var geometry in collection) + foreach (var newChildren in newCollection) { - UnsubscribeFromGeometry(geometry); + if (newChildren is not null) + { + subscriptions.Add(newChildren, _childrenPropertyChanged); + } } } void OnChildrenCollectionChanged(object sender, NotifyCollectionChangedEventArgs e) { - switch (e.Action) + if (e.Action == NotifyCollectionChangedAction.Reset) { - case NotifyCollectionChangedAction.Add: - if (e.NewItems != null) - { - foreach (Geometry geometry in e.NewItems) - { - SubscribeToGeometry(geometry); - } - } - break; - - case NotifyCollectionChangedAction.Remove: - if (e.OldItems != null) - { - foreach (Geometry geometry in e.OldItems) - { - UnsubscribeFromGeometry(geometry); - } - } - break; - - case NotifyCollectionChangedAction.Replace: - if (e.OldItems != null) - { - foreach (Geometry geometry in e.OldItems) - { - UnsubscribeFromGeometry(geometry); - } - } + // GeometryCollection is sealed, so Reset follows Clear after the collection is empty. + _childrenSubscriptions?.ResetChildren(); + } + else if (e.OldItems != null) + { + foreach (var oldItem in e.OldItems) + { + if (!(oldItem is Geometry oldGeometry)) + continue; - if (e.NewItems != null) - { - foreach (Geometry geometry in e.NewItems) - { - SubscribeToGeometry(geometry); - } - } - break; + _childrenSubscriptions?.Remove(oldGeometry); + } + } - case NotifyCollectionChangedAction.Move: - // No subscription changes required. - break; + if (e.NewItems != null) + { + foreach (var newItem in e.NewItems) + { + if (!(newItem is Geometry newGeometry)) + continue; - case NotifyCollectionChangedAction.Reset: - ResubscribeCollection(sender as GeometryCollection); - break; + _childrenSubscriptions?.Add(newGeometry, _childrenPropertyChanged); + } } Invalidate(); } - void ResubscribeCollection(GeometryCollection collection) + sealed class ChildrenSubscriptions { - UnsubscribeFromAllChildren(); + readonly WeakNotifyCollectionChangedProxy _collectionProxy = new(); + readonly List _childProxies = new(); - if (collection == null) - return; + ~ChildrenSubscriptions() => UnsubscribeAll(); - foreach (var geometry in collection) + public void Subscribe(GeometryCollection source, NotifyCollectionChangedEventHandler handler) { - SubscribeToGeometry(geometry); + _collectionProxy.Subscribe(source, handler); } - } - void SubscribeToGeometry(Geometry geometry) - { - if (geometry == null) - return; - - if (_subscriptionRefCounts.TryGetValue(geometry, out var count)) + public void Add(Geometry source, PropertyChangedEventHandler handler) { - _subscriptionRefCounts[geometry] = count + 1; - return; + _childProxies.Add(new WeakNotifyPropertyChangedProxy(source, handler)); } - _subscriptionRefCounts[geometry] = 1; - geometry.PropertyChanged += OnChildrenPropertyChanged; - } - - void UnsubscribeFromGeometry(Geometry geometry) - { - if (geometry == null) - return; - - if (!_subscriptionRefCounts.TryGetValue(geometry, out var count)) - return; - - if (count > 1) + public void Remove(Geometry source) { - _subscriptionRefCounts[geometry] = count - 1; - return; + 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; + } + } } - _subscriptionRefCounts.Remove(geometry); - geometry.PropertyChanged -= OnChildrenPropertyChanged; - } + public void ResetChildren() + { + UnsubscribeChildren(); + } - void UnsubscribeFromAllChildren() - { - foreach (var geometry in _subscriptionRefCounts.Keys) + public void UnsubscribeAll() { - geometry.PropertyChanged -= OnChildrenPropertyChanged; + _collectionProxy.Unsubscribe(); + UnsubscribeChildren(); } - _subscriptionRefCounts.Clear(); + void UnsubscribeChildren() + { + for (int i = 0; i < _childProxies.Count; i++) + _childProxies[i].Unsubscribe(); + + _childProxies.Clear(); + } } void OnChildrenPropertyChanged(object sender, PropertyChangedEventArgs e) @@ -212,9 +178,13 @@ void Invalidate() public override void AppendPath(Graphics.PathF path) { - foreach (var c in Children) + var children = Children; + if (children is null) + return; + + foreach (var c in children) { - c.AppendPath(path); + c?.AppendPath(path); } } } diff --git a/src/Controls/src/Core/Shapes/GeometryHelper.cs b/src/Controls/src/Core/Shapes/GeometryHelper.cs index 0d0e64ef0906..907091f52f51 100644 --- a/src/Controls/src/Core/Shapes/GeometryHelper.cs +++ b/src/Controls/src/Core/Shapes/GeometryHelper.cs @@ -41,11 +41,16 @@ public static void FlattenGeometry(PathGeometry pathGeoDst, Geometry geoSrc, dou Matrix matx = matxPrevious; - if (geoSrc is GeometryGroup) + if (geoSrc is GeometryGroup geometryGroup) { - foreach (Geometry geoChild in (geoSrc as GeometryGroup).Children) + var children = geometryGroup.Children; + if (children is null) + return; + + foreach (Geometry geoChild in children) { - FlattenGeometry(pathGeoDst, geoChild, tolerance, matx); + if (geoChild is not null) + FlattenGeometry(pathGeoDst, geoChild, tolerance, matx); } } else if (geoSrc is LineGeometry) diff --git a/src/Controls/tests/Core.UnitTests/GeometryGroupMemoryTests.cs b/src/Controls/tests/Core.UnitTests/GeometryGroupMemoryTests.cs new file mode 100644 index 000000000000..b2e4a869eaa0 --- /dev/null +++ b/src/Controls/tests/Core.UnitTests/GeometryGroupMemoryTests.cs @@ -0,0 +1,330 @@ +using System; +using System.Runtime.CompilerServices; +using System.Threading.Tasks; +using Microsoft.Maui.Controls.Shapes; +using Microsoft.Maui.Graphics; +using Xunit; + +namespace Microsoft.Maui.Controls.Core.UnitTests +{ + public class GeometryGroupMemoryTests : BaseTestFixture + { + [MethodImpl(MethodImplOptions.NoInlining)] + static WeakReference AssignSharedChildrenAndDrop(GeometryCollection sharedChildren) + { + var group = new GeometryGroup { Children = sharedChildren }; + return new WeakReference(group); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static WeakReference AssignSharedChildAndDrop(Geometry sharedChild) + { + var group = new GeometryGroup + { + Children = new GeometryCollection { sharedChild } + }; + + return new WeakReference(group); + } + + [Fact] + public async Task GeometryGroupDoesNotLeakWhenSharingChildren() + { + // A long-lived/shared GeometryCollection, exactly as the issue describes. + var sharedChildren = new GeometryCollection + { + new RectangleGeometry() + }; + var weakGroup = AssignSharedChildrenAndDrop(sharedChildren); + + Assert.False(await weakGroup.WaitForCollect(), "GeometryGroup should not be alive!"); + GC.KeepAlive(sharedChildren); + } + + [Fact] + public async Task GeometryGroupDoesNotLeakWhenSharingChild() + { + var sharedChild = new RectangleGeometry(); + var weakGroup = AssignSharedChildAndDrop(sharedChild); + + Assert.False(await weakGroup.WaitForCollect(), "GeometryGroup should not be alive!"); + GC.KeepAlive(sharedChild); + } + + [Fact] + public async Task ChildGeometryChangesStillInvalidateAfterGc() + { + var child = new RectangleGeometry { Rect = new Rect(0, 0, 10, 10) }; + var group = new GeometryGroup + { + Children = new GeometryCollection { child } + }; + bool invalidated = false; + group.InvalidateGeometryRequested += (_, __) => invalidated = true; + + await TestHelpers.Collect(); + + child.Rect = new Rect(0, 0, 20, 20); + + Assert.True(invalidated); + GC.KeepAlive(group); + } + + [Fact] + public void AssigningChildrenWithNullEntrySubscribesValidChildren() + { + var child = new RectangleGeometry { Rect = new Rect(0, 0, 10, 10) }; + var group = new GeometryGroup + { + Children = new GeometryCollection { null, child } + }; + bool invalidated = false; + group.InvalidateGeometryRequested += (_, __) => invalidated = true; + + child.Rect = new Rect(0, 0, 20, 20); + + Assert.True(invalidated); + } + + [Fact] + public void AppendPathSkipsNullChildren() + { + var group = new GeometryGroup + { + Children = new GeometryCollection + { + null, + new RectangleGeometry { Rect = new Rect(0, 0, 10, 10) } + } + }; + var path = new PathF(); + + group.AppendPath(path); + + Assert.Equal(1, path.SubPathCount); + } + + [Fact] + public void AppendPathAllowsNullChildrenCollection() + { + var group = new GeometryGroup { Children = null }; + var path = new PathF(); + + group.AppendPath(path); + + Assert.Equal(0, path.SubPathCount); + } + + [Fact] + public void FlattenGeometrySkipsNullChildren() + { + var group = new GeometryGroup + { + Children = new GeometryCollection + { + null, + new RectangleGeometry { Rect = new Rect(0, 0, 10, 10) } + } + }; + + var flattened = GeometryHelper.FlattenGeometry(group, tolerance: 0.1); + + Assert.Single(flattened.Figures); + } + + [Fact] + public void FlattenGeometryAllowsNullChildrenCollection() + { + var group = new GeometryGroup { Children = null }; + + var flattened = GeometryHelper.FlattenGeometry(group, tolerance: 0.1); + + Assert.Empty(flattened.Figures); + } + + [Fact] + public async Task SharedChildrenInvalidateEachLiveGroupAfterGc() + { + var child = new RectangleGeometry { Rect = new Rect(0, 0, 10, 10) }; + var sharedChildren = new GeometryCollection { child }; + var firstGroup = new GeometryGroup { Children = sharedChildren }; + var secondGroup = new GeometryGroup { Children = sharedChildren }; + int firstInvalidationCount = 0; + int secondInvalidationCount = 0; + firstGroup.InvalidateGeometryRequested += (_, __) => firstInvalidationCount++; + secondGroup.InvalidateGeometryRequested += (_, __) => secondInvalidationCount++; + + await TestHelpers.Collect(); + + child.Rect = new Rect(0, 0, 20, 20); + + Assert.Equal(1, firstInvalidationCount); + Assert.Equal(1, secondInvalidationCount); + GC.KeepAlive(firstGroup); + GC.KeepAlive(secondGroup); + } + + [Fact] + public void RemovingAndReplacingChildrenMovesSubscriptions() + { + var removedChild = new RectangleGeometry { Rect = new Rect(0, 0, 10, 10) }; + var retainedChild = new RectangleGeometry { Rect = new Rect(0, 0, 10, 10) }; + var children = new GeometryCollection { removedChild, retainedChild }; + var group = new GeometryGroup { Children = children }; + int invalidationCount = 0; + group.InvalidateGeometryRequested += (_, __) => invalidationCount++; + + children.Remove(removedChild); + invalidationCount = 0; + + removedChild.Rect = new Rect(0, 0, 20, 20); + Assert.Equal(0, invalidationCount); + + retainedChild.Rect = new Rect(0, 0, 20, 20); + Assert.Equal(1, invalidationCount); + + var replacementChild = new RectangleGeometry { Rect = new Rect(0, 0, 10, 10) }; + children[0] = replacementChild; + invalidationCount = 0; + + retainedChild.Rect = new Rect(0, 0, 30, 30); + Assert.Equal(0, invalidationCount); + + replacementChild.Rect = new Rect(0, 0, 20, 20); + Assert.Equal(1, invalidationCount); + } + + [Fact] + public void ReplacingChildrenCollectionMovesSubscriptions() + { + var oldChild = new RectangleGeometry { Rect = new Rect(0, 0, 10, 10) }; + var oldChildren = new GeometryCollection { oldChild }; + var newChild = new RectangleGeometry { Rect = new Rect(0, 0, 10, 10) }; + var newChildren = new GeometryCollection { newChild }; + var group = new GeometryGroup { Children = oldChildren }; + int invalidationCount = 0; + group.InvalidateGeometryRequested += (_, __) => invalidationCount++; + + group.Children = newChildren; + + oldChild.Rect = new Rect(0, 0, 20, 20); + var addedOldChild = new RectangleGeometry(); + oldChildren.Add(addedOldChild); + addedOldChild.Rect = new Rect(0, 0, 20, 20); + + Assert.Equal(0, invalidationCount); + + newChild.Rect = new Rect(0, 0, 20, 20); + Assert.Equal(1, invalidationCount); + + var addedNewChild = new RectangleGeometry(); + newChildren.Add(addedNewChild); + invalidationCount = 0; + addedNewChild.Rect = new Rect(0, 0, 20, 20); + + Assert.Equal(1, invalidationCount); + } + + [Fact] + public void ReplacingChildrenThroughNullMovesSubscriptions() + { + var oldChild = new RectangleGeometry { Rect = new Rect(0, 0, 10, 10) }; + var oldChildren = new GeometryCollection { oldChild }; + var newChild = new RectangleGeometry { Rect = new Rect(0, 0, 10, 10) }; + var newChildren = new GeometryCollection { newChild }; + var group = new GeometryGroup { Children = oldChildren }; + int invalidationCount = 0; + group.InvalidateGeometryRequested += (_, __) => invalidationCount++; + + group.Children = null; + + oldChild.Rect = new Rect(0, 0, 20, 20); + var addedOldChild = new RectangleGeometry(); + oldChildren.Add(addedOldChild); + addedOldChild.Rect = new Rect(0, 0, 20, 20); + + Assert.Equal(0, invalidationCount); + + group.Children = newChildren; + newChild.Rect = new Rect(0, 0, 20, 20); + + Assert.Equal(1, invalidationCount); + + var addedNewChild = new RectangleGeometry(); + newChildren.Add(addedNewChild); + invalidationCount = 0; + addedNewChild.Rect = new Rect(0, 0, 20, 20); + + Assert.Equal(1, invalidationCount); + } + + [Fact] + public void MovingChildrenPreservesSubscriptions() + { + var firstChild = new RectangleGeometry { Rect = new Rect(0, 0, 10, 10) }; + var secondChild = new RectangleGeometry { Rect = new Rect(0, 0, 10, 10) }; + var children = new GeometryCollection { firstChild, secondChild }; + var group = new GeometryGroup { Children = children }; + int invalidationCount = 0; + group.InvalidateGeometryRequested += (_, __) => invalidationCount++; + + children.Move(0, 1); + invalidationCount = 0; + + firstChild.Rect = new Rect(0, 0, 20, 20); + Assert.Equal(1, invalidationCount); + + secondChild.Rect = new Rect(0, 0, 20, 20); + Assert.Equal(2, invalidationCount); + } + + [Fact] + public void DuplicateChildrenPreserveOccurrenceSubscriptions() + { + var child = new RectangleGeometry { Rect = new Rect(0, 0, 10, 10) }; + var children = new GeometryCollection { child, child }; + var group = new GeometryGroup { Children = children }; + int invalidationCount = 0; + group.InvalidateGeometryRequested += (_, __) => invalidationCount++; + + child.Rect = new Rect(0, 0, 20, 20); + + Assert.Equal(2, invalidationCount); + + children.Remove(child); + invalidationCount = 0; + child.Rect = new Rect(0, 0, 30, 30); + + Assert.Equal(1, invalidationCount); + + children.Remove(child); + invalidationCount = 0; + child.Rect = new Rect(0, 0, 40, 40); + + Assert.Equal(0, invalidationCount); + } + + [Fact] + public void ResetChildrenResubscribesLaterAdditions() + { + var oldChild = new RectangleGeometry { Rect = new Rect(0, 0, 10, 10) }; + var newChild = new RectangleGeometry { Rect = new Rect(0, 0, 10, 10) }; + var group = new GeometryGroup + { + Children = new GeometryCollection { oldChild } + }; + bool invalidated = false; + group.InvalidateGeometryRequested += (_, __) => invalidated = true; + + group.Children.Clear(); + group.Children.Add(newChild); + invalidated = false; + + oldChild.Rect = new Rect(0, 0, 20, 20); + Assert.False(invalidated); + + newChild.Rect = new Rect(0, 0, 20, 20); + Assert.True(invalidated); + } + } +}