Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 104 additions & 5 deletions src/Controls/src/Core/FormattedString.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/// <summary>Initializes a new instance of the FormattedString class.</summary>
public FormattedString() => _spans.CollectionChanged += OnCollectionChanged;
public FormattedString()
{
_spanSubscriptions = new SpanSubscriptions(this);
_spans.CollectionChanged += OnCollectionChanged;
}

protected override void OnBindingContextChanged()
{
Expand Down Expand Up @@ -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);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[major] Logic and Correctness — This now removes one event subscription per removed Span, but the logical-child cleanup in the same branch still removes through bo.Parent. For duplicate occurrences, the first removal clears span.Parent, so the second removal reaches this subscription cleanup while leaving the remaining logical child in this FormattedString; for a Span shared by two FormattedString instances, removing it from the first removes the logical child from the second instead. Please remove the occurrence from this FormattedString's logical children (and add a regression assertion) so subscription cleanup and logical-tree cleanup stay symmetric.

}

}
Expand All @@ -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);
}

}
Expand All @@ -83,6 +90,98 @@ void OnCollectionChanged(object sender, NotifyCollectionChangedEventArgs e)

void OnItemPropertyChanging(object sender, PropertyChangingEventArgs e) => OnPropertyChanging(nameof(Spans));

sealed class SpanSubscriptions
{
readonly WeakReference<FormattedString> _owner;
readonly List<SpanSubscription> _subscriptions = new();

public SpanSubscriptions(FormattedString owner) => _owner = new(owner);

~SpanSubscriptions() => Clear();

public void Add(Span span) => _subscriptions.Add(new SpanSubscription(_owner, span));
Comment on lines +93 to +102

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<FormattedString> _owner;
Span _span;

public SpanSubscription(WeakReference<FormattedString> 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<Span>
{
protected override void InsertItem(int index, Span item) => base.InsertItem(index, item ?? throw new ArgumentNullException(nameof(item)));
Expand Down
110 changes: 110 additions & 0 deletions src/Controls/tests/Core.UnitTests/FormattedStringTests.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using System;
using System.Collections.ObjectModel;
using System.Threading.Tasks;
using Xunit;

namespace Microsoft.Maui.Controls.Core.UnitTests
Expand Down Expand Up @@ -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()
{
Expand Down Expand Up @@ -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);
}
}
}
Loading