diff --git a/src/Controls/src/Core/FormattedString.cs b/src/Controls/src/Core/FormattedString.cs index 396005f6cf6b..621ea05fcfa3 100644 --- a/src/Controls/src/Core/FormattedString.cs +++ b/src/Controls/src/Core/FormattedString.cs @@ -23,8 +23,17 @@ internal event NotifyCollectionChangedEventHandler SpansCollectionChanged remove => _weakEventManager.RemoveEventHandler(value, nameof(SpansCollectionChanged)); } + // Subscribe to each Span's PropertyChanging/PropertyChanged via per-occurrence weak + // subscription tokens so that a shared or long-lived Span (e.g. one held by a view-model or + // App.Resources) does not keep this FormattedString alive through the event subscriptions. + readonly SpanSubscriptions _spanSubscriptions; + /// Initializes a new instance of the FormattedString class. - public FormattedString() => _spans.CollectionChanged += OnCollectionChanged; + public FormattedString() + { + _spanSubscriptions = new SpanSubscriptions(this); + _spans.CollectionChanged += OnCollectionChanged; + } protected override void OnBindingContextChanged() { @@ -53,8 +62,7 @@ void OnCollectionChanged(object sender, NotifyCollectionChangedEventArgs e) if (bo != null) { bo.Parent?.RemoveLogicalChild(bo); - bo.PropertyChanging -= OnItemPropertyChanging; - bo.PropertyChanged -= OnItemPropertyChanged; + _spanSubscriptions.Remove(bo); } } @@ -68,8 +76,7 @@ void OnCollectionChanged(object sender, NotifyCollectionChangedEventArgs e) if (bo != null) { this.AddLogicalChild(bo); - bo.PropertyChanging += OnItemPropertyChanging; - bo.PropertyChanged += OnItemPropertyChanged; + _spanSubscriptions.Add(bo); } } @@ -83,6 +90,98 @@ void OnCollectionChanged(object sender, NotifyCollectionChangedEventArgs e) void OnItemPropertyChanging(object sender, PropertyChangingEventArgs e) => OnPropertyChanging(nameof(Spans)); + sealed class SpanSubscriptions + { + readonly WeakReference _owner; + readonly List _subscriptions = new(); + + public SpanSubscriptions(FormattedString owner) => _owner = new(owner); + + ~SpanSubscriptions() => Clear(); + + public void Add(Span span) => _subscriptions.Add(new SpanSubscription(_owner, span)); + + public void Remove(Span span) + { + for (int i = 0; i < _subscriptions.Count; i++) + { + if (_subscriptions[i].Span == span) + { + _subscriptions[i].Unsubscribe(); + _subscriptions.RemoveAt(i); + return; + } + } + } + + void Clear() + { + foreach (var subscription in _subscriptions) + { + subscription.Unsubscribe(); + } + + _subscriptions.Clear(); + } + } + + sealed class SpanSubscription + { + // The Span's event delegate holds a strong reference to this SpanSubscription, so the + // instance is kept alive until either the Span fires an event (which triggers self-cleanup + // via the weak-owner check in OnPropertyChanged/OnPropertyChanging) or the SpanSubscriptions + // finalizer runs. This is an accepted trade-off of weak-event cleanup: only these small + // tokens may linger briefly, while the owning FormattedString is free to be collected. + readonly WeakReference _owner; + Span _span; + + public SpanSubscription(WeakReference owner, Span span) + { + _owner = owner; + _span = span; + _span.PropertyChanging += OnPropertyChanging; + _span.PropertyChanged += OnPropertyChanged; + } + + public Span Span => _span; + + public void Unsubscribe() + { + if (_span is null) + { + return; + } + + _span.PropertyChanging -= OnPropertyChanging; + _span.PropertyChanged -= OnPropertyChanged; + _span = null; + } + + void OnPropertyChanged(object sender, PropertyChangedEventArgs e) + { + if (_owner.TryGetTarget(out var owner)) + { + owner.OnItemPropertyChanged(sender, e); + } + else + { + Unsubscribe(); + } + } + + void OnPropertyChanging(object sender, PropertyChangingEventArgs e) + { + if (_owner.TryGetTarget(out var owner)) + { + owner.OnItemPropertyChanging(sender, e); + } + else + { + Unsubscribe(); + } + } + } + class SpanCollection : ObservableCollection { protected override void InsertItem(int index, Span item) => base.InsertItem(index, item ?? throw new ArgumentNullException(nameof(item))); diff --git a/src/Controls/tests/Core.UnitTests/FormattedStringTests.cs b/src/Controls/tests/Core.UnitTests/FormattedStringTests.cs index 83157fc43e3d..433eda4880a4 100644 --- a/src/Controls/tests/Core.UnitTests/FormattedStringTests.cs +++ b/src/Controls/tests/Core.UnitTests/FormattedStringTests.cs @@ -1,5 +1,6 @@ using System; using System.Collections.ObjectModel; +using System.Threading.Tasks; using Xunit; namespace Microsoft.Maui.Controls.Core.UnitTests @@ -61,6 +62,88 @@ public void SpanChangesUnsubscribes() Assert.False(spansChanged); } + [Fact] + public void DuplicateSpanChangesUnsubscribes() + { + var span = new Span(); + var fs = new FormattedString(); + fs.Spans.Add(span); + fs.Spans.Add(span); + fs.Spans.Remove(span); + fs.Spans.Remove(span); + + bool spansChanged = false; + fs.PropertyChanged += (s, e) => + { + if (e.PropertyName == "Spans") + spansChanged = true; + }; + + span.Text = "New text"; + + Assert.False(spansChanged); + } + + [Fact] + public void DuplicateSpanKeepsOneSubscriptionAfterSingleRemove() + { + var span = new Span(); + var fs = new FormattedString(); + fs.Spans.Add(span); + fs.Spans.Add(span); + fs.Spans.Remove(span); // removes one occurrence only + + bool spansChanged = false; + fs.PropertyChanged += (s, e) => + { + if (e.PropertyName == "Spans") + spansChanged = true; + }; + + span.Text = "New text"; + + Assert.True(spansChanged); // second subscription still active + } + + [Fact] + public void SpanChangingUnsubscribesAfterRemoval() + { + var span = new Span { Text = "Original" }; + var fs = new FormattedString(); + fs.Spans.Add(span); + fs.Spans.Remove(span); + + bool spansChanging = false; + fs.PropertyChanging += (s, e) => + { + if (e.PropertyName == "Spans") + spansChanging = true; + }; + + span.Text = "New text"; + + Assert.False(spansChanging); + } + + [Fact] + public void SpanChangingTriggersSpansPropertyChanging() + { + var span = new Span { Text = "Original text" }; + var fs = new FormattedString(); + fs.Spans.Add(span); + + bool spansChanging = false; + fs.PropertyChanging += (s, e) => + { + if (e.PropertyName == "Spans") + spansChanging = true; + }; + + span.Text = "New text"; + + Assert.True(spansChanging); + } + [Fact] public void AddingSpanTriggersSpansPropertyChange() { @@ -100,5 +183,32 @@ public void ImplicitStringConversionNull() Assert.NotNull(fs.Spans[0]); Assert.Equal(fs.Spans[0].Text, original); } + + [Fact, Category(TestCategory.Memory)] + public async Task FormattedStringDoesNotLeak() + { + // Long-lived span, like one shared from a view-model or App.Resources. + // Adding it to a FormattedString subscribes FormattedString to the + // span's PropertyChanged/PropertyChanging events and makes FormattedString + // the span's logical parent. If those references aren't weak, the shared + // span keeps every FormattedString it was added to alive. + var span = new Span { Text = "Hello" }; + + WeakReference CreateReference() + { + var fs = new FormattedString(); + fs.Spans.Add(span); + return new(fs); + } + + WeakReference reference = CreateReference(); + + await TestHelpers.Collect(); + + Assert.False(await reference.WaitForCollect(), "FormattedString should not be alive!"); + + // Ensure the shared Span isn't collected during the test + GC.KeepAlive(span); + } } } \ No newline at end of file