From 73a1e37c9bbbdf7ed91df1b3e845fd5043f61864 Mon Sep 17 00:00:00 2001 From: Jonathan Peppers Date: Fri, 24 Feb 2023 16:15:03 -0600 Subject: [PATCH] [controls] fix memory leak in `BindableLayout` Context: https://github.com/dotnet/maui/issues/12130 Context: https://github.com/angelru/CvSlowJittering In reviewing the above sample, I found the following problem: * Setup a `BindableLayout` on a long-lived `BindableCollection` * Navigate away, or otherwise remove the `BindableLayout` from the screen. * A `BindableLayoutController` object lives forever -- or as long as the `BindableCollection`. Note that nothing ever removes/clears `BindableLayout.SetItemsSource()`. This gets really bad if you have a `BindableLayout` inside a `CollectionView`. In the sample above, it has a `` using `BindableLayout` inside. On Windows, I saw it creating infinite `BindableLayoutController` objects while scrolling. I could reproduce this issue in a unit test. I applied the same tricks from 0372df7 to solve the problem. #12130 is not fully solved as it still seems slow to me and still has a memory issue? Still thinking if there is a "comprehensive" way to solve all of these types of issues across the repo... --- src/Controls/src/Core/BindableLayout.cs | 53 +++++++++++++++++-- .../Core.UnitTests/BindableLayoutTests.cs | 37 +++++++++++++ 2 files changed, 85 insertions(+), 5 deletions(-) diff --git a/src/Controls/src/Core/BindableLayout.cs b/src/Controls/src/Core/BindableLayout.cs index f5a45800bd3d..91b74728c770 100644 --- a/src/Controls/src/Core/BindableLayout.cs +++ b/src/Controls/src/Core/BindableLayout.cs @@ -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); } @@ -201,6 +201,7 @@ internal static void Clear(this IBindableLayout layout) class BindableLayoutController { readonly WeakReference _layoutWeakReference; + readonly WeakCollectionChangedProxy _collectionChangedProxy = new(); IEnumerable _itemsSource; DataTemplate _itemTemplate; DataTemplateSelector _itemTemplateSelector; @@ -221,6 +222,8 @@ public BindableLayoutController(IBindableLayout layout) _layoutWeakReference = new WeakReference(layout); } + ~BindableLayoutController() => _collectionChangedProxy.Unsubscribe(); + internal void StartBatchUpdate() { _isBatchUpdate = true; @@ -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) @@ -392,5 +395,45 @@ void ItemsSourceCollectionChanged(object sender, NotifyCollectionChangedEventArg if (e.Action != NotifyCollectionChangedAction.Reset) UpdateEmptyView(layout); } + + class WeakCollectionChangedProxy + { + WeakReference _handler; + WeakReference _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(source); + _handler = new WeakReference(handler); + source.CollectionChanged += OnCollectionChanged; + } + + public void Unsubscribe() + { + if (_source is not null && _source.TryGetTarget(out var s)) + { + s.CollectionChanged -= OnCollectionChanged; + } + _source = null; + _handler = null; + } + } } } \ No newline at end of file diff --git a/src/Controls/tests/Core.UnitTests/BindableLayoutTests.cs b/src/Controls/tests/Core.UnitTests/BindableLayoutTests.cs index 261b1c1c50d2..18950dcd24d0 100644 --- a/src/Controls/tests/Core.UnitTests/BindableLayoutTests.cs +++ b/src/Controls/tests/Core.UnitTests/BindableLayoutTests.cs @@ -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 @@ -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 { "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) {