Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[controls] fix memory leak in BindableLayout #13550

Merged
merged 1 commit into from
Feb 28, 2023
Merged

[controls] fix memory leak in BindableLayout #13550

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
53 changes: 48 additions & 5 deletions src/Controls/src/Core/BindableLayout.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ public static void SetEmptyViewTemplate(BindableObject b, DataTemplate value)
b.SetValue(EmptyViewTemplateProperty, value);
}

static BindableLayoutController GetBindableLayoutController(BindableObject b)
internal static BindableLayoutController GetBindableLayoutController(BindableObject b)
{
return (BindableLayoutController)b.GetValue(BindableLayoutControllerProperty);
}
Expand Down Expand Up @@ -201,6 +201,7 @@ internal static void Clear(this IBindableLayout layout)
class BindableLayoutController
{
readonly WeakReference<IBindableLayout> _layoutWeakReference;
readonly WeakCollectionChangedProxy _collectionChangedProxy = new();
IEnumerable _itemsSource;
DataTemplate _itemTemplate;
DataTemplateSelector _itemTemplateSelector;
Expand All @@ -221,6 +222,8 @@ public BindableLayoutController(IBindableLayout layout)
_layoutWeakReference = new WeakReference<IBindableLayout>(layout);
}

~BindableLayoutController() => _collectionChangedProxy.Unsubscribe();

internal void StartBatchUpdate()
{
_isBatchUpdate = true;
Expand All @@ -234,16 +237,16 @@ internal void EndBatchUpdate()

void SetItemsSource(IEnumerable itemsSource)
{
if (_itemsSource is INotifyCollectionChanged c)
if (_itemsSource is INotifyCollectionChanged)
{
c.CollectionChanged -= ItemsSourceCollectionChanged;
_collectionChangedProxy.Unsubscribe();
}

_itemsSource = itemsSource;

if (_itemsSource is INotifyCollectionChanged c1)
if (_itemsSource is INotifyCollectionChanged c)
{
c1.CollectionChanged += ItemsSourceCollectionChanged;
_collectionChangedProxy.Subscribe(c, ItemsSourceCollectionChanged);
}

if (!_isBatchUpdate)
Expand Down Expand Up @@ -392,5 +395,45 @@ void ItemsSourceCollectionChanged(object sender, NotifyCollectionChangedEventArg
if (e.Action != NotifyCollectionChangedAction.Reset)
UpdateEmptyView(layout);
}

class WeakCollectionChangedProxy
{
WeakReference<NotifyCollectionChangedEventHandler> _handler;
WeakReference<INotifyCollectionChanged> _source;

void OnCollectionChanged(object sender, NotifyCollectionChangedEventArgs e)
{
if (_handler.TryGetTarget(out var handler))
{
handler(sender, e);
}
else
{
Unsubscribe();
}
}

public void Subscribe(INotifyCollectionChanged source, NotifyCollectionChangedEventHandler handler)
{
if (_source is not null && _source.TryGetTarget(out var s))
{
s.CollectionChanged -= OnCollectionChanged;
}

_source = new WeakReference<INotifyCollectionChanged>(source);
_handler = new WeakReference<NotifyCollectionChangedEventHandler>(handler);
source.CollectionChanged += OnCollectionChanged;
}

public void Unsubscribe()
{
if (_source is not null && _source.TryGetTarget(out var s))
{
s.CollectionChanged -= OnCollectionChanged;
}
_source = null;
_handler = null;
}
}
}
}
37 changes: 37 additions & 0 deletions src/Controls/tests/Core.UnitTests/BindableLayoutTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
using System.Collections.Specialized;
using System.ComponentModel;
using System.Linq;
using System.Reflection;
using System.Threading.Tasks;
using Xunit;

namespace Microsoft.Maui.Controls.Core.UnitTests
Expand Down Expand Up @@ -386,6 +388,41 @@ public void ValidateBindableProperties()
Assert.Equal(itemTemplateSelector, layout.GetValue(BindableLayout.ItemTemplateSelectorProperty));
}

[Fact]
public async Task DoesNotLeak()
{
WeakReference controllerRef, proxyRef;
var list = new ObservableCollection<string> { "Foo", "Bar", "Baz" };

// Scope for BindableLayout
{
var layout = new StackLayout { IsPlatformEnabled = true };
BindableLayout.SetItemTemplate(layout, new DataTemplate(() => new Label()));
BindableLayout.SetItemsSource(layout, list);

var controller = BindableLayout.GetBindableLayoutController(layout);
controllerRef = new WeakReference(controller);

// BindableLayoutController._collectionChangedProxy
var flags = BindingFlags.NonPublic | BindingFlags.Instance;
var proxy = controller.GetType().GetField("_collectionChangedProxy", flags).GetValue(controller);
Assert.NotNull(proxy);
proxyRef = new WeakReference(proxy);
}

// First GC
await Task.Yield();
GC.Collect();
GC.WaitForPendingFinalizers();
Assert.False(controllerRef.IsAlive, "BindableLayoutController should not be alive!");

// Second GC
await Task.Yield();
GC.Collect();
GC.WaitForPendingFinalizers();
Assert.False(proxyRef.IsAlive, "WeakCollectionChangedProxy should not be alive!");
}

// Checks if for every item in the items source there's a corresponding view
static bool IsLayoutWithItemsSource(IEnumerable itemsSource, Compatibility.Layout layout)
{
Expand Down