Skip to content

Commit

Permalink
[controls] fix memory leak in CollectionView (#14329)
Browse files Browse the repository at this point in the history
Fixes: #10578
Context: https://github.com/Vroomer/MAUI-navigation-memory-leak.git

After testing the above sample, I found that adding a `CollectionView`
to a `Page`, makes it and the entire page live forever.

Android & Windows:

* `MauiRecyclerView` and `StructuredItemsViewHandler` respectively
  subscribed to `ItemsLayout.PropertyChanged`. This kept the
  `CollectionView` alive -> all the way up to the `Page`.

* Switched to using `WeakNotifyPropertyChangedProxy` solved the issue
  for these two platforms.

iOS:

* Generally had a small "nest" of circular references. Initially, I saw
  `CollectionView`, `UICollectionView`, and various helper classes that
  would live forever.

* `ItemsViewController : UICollectionViewController` ->
  * `ObservableItemsSource` ->
    * `UICollectionViewController` and `UICollectionView`

* `UICollectionView` ->
  * `ItemsViewLayout : UICollectionViewFlowLayout` ->
    * `Func<UICollectionViewCell>` ->
      * `ItemsViewController : UICollectionViewController` ->
        * `UICollectionView`

I switched to using `WeakReference<T>` to break the circular references.
This required several null checks, where `null` references were not
possible before.

After these changes my tests pass, yay!
  • Loading branch information
jonathanpeppers authored Apr 13, 2023
1 parent e8039c9 commit 84128e6
Show file tree
Hide file tree
Showing 14 changed files with 114 additions and 46 deletions.
17 changes: 12 additions & 5 deletions src/Controls/src/Core/Handlers/Items/Android/MauiRecyclerView.cs
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,10 @@ public class MauiRecyclerView<TItemsView, TAdapter, TItemsViewSource> : Recycler

ItemTouchHelper _itemTouchHelper;
SimpleItemTouchHelperCallback _itemTouchHelperCallback;
WeakNotifyPropertyChangedProxy _layoutPropertyChangedProxy;
PropertyChangedEventHandler _layoutPropertyChanged;

~MauiRecyclerView() => _layoutPropertyChangedProxy?.Unsubscribe();

public MauiRecyclerView(Context context, Func<IItemsLayout> getItemsLayout, Func<TAdapter> getAdapter) : base(context)
{
Expand All @@ -58,9 +62,10 @@ public MauiRecyclerView(Context context, Func<IItemsLayout> getItemsLayout, Func
public virtual void TearDownOldElement(TItemsView oldElement)
{
// Stop listening for layout property changes
if (ItemsLayout != null)
if (_layoutPropertyChangedProxy is not null)
{
ItemsLayout.PropertyChanged -= LayoutPropertyChanged;
_layoutPropertyChangedProxy.Unsubscribe();
_layoutPropertyChanged = null;
}

// Stop listening for ScrollTo requests
Expand Down Expand Up @@ -283,14 +288,16 @@ public virtual void UpdateCanReorderItems()

public virtual void UpdateLayoutManager()
{
if (ItemsLayout != null)
ItemsLayout.PropertyChanged -= LayoutPropertyChanged;
_layoutPropertyChangedProxy?.Unsubscribe();

ItemsLayout = _getItemsLayout();

// Keep track of the ItemsLayout's property changes
if (ItemsLayout != null)
ItemsLayout.PropertyChanged += LayoutPropertyChanged;
{
_layoutPropertyChanged ??= LayoutPropertyChanged;
_layoutPropertyChangedProxy = new WeakNotifyPropertyChangedProxy(ItemsLayout, _layoutPropertyChanged);
}

SetLayoutManager(SelectLayoutManager(ItemsLayout));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@ public partial class StructuredItemsViewHandler<TItemsView> : ItemsViewHandler<T
{
View _currentHeader;
View _currentFooter;
WeakNotifyPropertyChangedProxy _layoutPropertyChangedProxy;
PropertyChangedEventHandler _layoutPropertyChanged;

~StructuredItemsViewHandler() => _layoutPropertyChangedProxy?.Unsubscribe();

protected override IItemsLayout Layout { get => ItemsView?.ItemsLayout; }

Expand All @@ -24,15 +28,26 @@ protected override void ConnectHandler(ListViewBase platformView)
base.ConnectHandler(platformView);

if (Layout is not null)
Layout.PropertyChanged += LayoutPropertyChanged;
{
_layoutPropertyChanged ??= LayoutPropertyChanged;
_layoutPropertyChangedProxy = new WeakNotifyPropertyChangedProxy(Layout, _layoutPropertyChanged);
}
else if (_layoutPropertyChangedProxy is not null)
{
_layoutPropertyChangedProxy.Unsubscribe();
_layoutPropertyChangedProxy = null;
}
}

protected override void DisconnectHandler(ListViewBase platformView)
{
base.DisconnectHandler(platformView);

if (Layout is not null)
Layout.PropertyChanged -= LayoutPropertyChanged;
if (_layoutPropertyChangedProxy is not null)
{
_layoutPropertyChangedProxy.Unsubscribe();
_layoutPropertyChangedProxy = null;
}
}

void LayoutPropertyChanged(object sender, PropertyChangedEventArgs e)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,11 @@ protected override (bool VisibleItems, int First, int Center, int Last) GetVisib
{
var (VisibleItems, First, Center, Last) = GetVisibleItemsIndexPath();
int firstVisibleItemIndex = -1, centerItemIndex = -1, lastVisibleItemIndex = -1;
if (VisibleItems)
if (VisibleItems && ViewController is CarouselViewController vc)
{
firstVisibleItemIndex = ViewController.GetIndexFromIndexPath(First);
centerItemIndex = ViewController.GetIndexFromIndexPath(Center);
lastVisibleItemIndex = ViewController.GetIndexFromIndexPath(Last);
firstVisibleItemIndex = vc.GetIndexFromIndexPath(First);
centerItemIndex = vc.GetIndexFromIndexPath(Center);
lastVisibleItemIndex = vc.GetIndexFromIndexPath(Last);
}
return (VisibleItems, firstVisibleItemIndex, centerItemIndex, lastVisibleItemIndex);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,12 @@ public GroupableItemsViewDelegator(ItemsViewLayout itemsViewLayout, TViewControl

public override CGSize GetReferenceSizeForHeader(UICollectionView collectionView, UICollectionViewLayout layout, nint section)
{
return ViewController.GetReferenceSizeForHeader(collectionView, layout, section);
return ViewController?.GetReferenceSizeForHeader(collectionView, layout, section) ?? CGSize.Empty;
}

public override CGSize GetReferenceSizeForFooter(UICollectionView collectionView, UICollectionViewLayout layout, nint section)
{
return ViewController.GetReferenceSizeForFooter(collectionView, layout, section);
return ViewController?.GetReferenceSizeForFooter(collectionView, layout, section) ?? CGSize.Empty;
}

public override void ScrollAnimationEnded(UIScrollView scrollView)
Expand All @@ -37,7 +37,7 @@ public override UIEdgeInsets GetInsetForSection(UICollectionView collectionView,
return default;
}

return ViewController.GetInsetForSection(ItemsViewLayout, collectionView, section);
return ViewController?.GetInsetForSection(ItemsViewLayout, collectionView, section) ?? UIEdgeInsets.Zero;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ public abstract class ItemsViewController<TItemsView> : UICollectionViewControll
bool _emptyViewDisplayed;
bool _disposed;

Func<UICollectionViewCell> _getPrototype;
UIView _emptyUIView;
VisualElement _emptyViewFormsElement;
Dictionary<object, TemplatedCell> _measurementCells = new Dictionary<object, TemplatedCell>();
Expand Down Expand Up @@ -200,7 +201,8 @@ void EnsureLayoutInitialized()

_initialized = true;

ItemsViewLayout.GetPrototype = GetPrototype;
_getPrototype ??= GetPrototype;
ItemsViewLayout.GetPrototype = _getPrototype;

Delegator = CreateDelegator();
CollectionView.Delegate = Delegator;
Expand Down
24 changes: 17 additions & 7 deletions src/Controls/src/Core/Handlers/Items/iOS/ItemsViewDelegator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,17 @@ public class ItemsViewDelegator<TItemsView, TViewController> : UICollectionViewD
where TItemsView : ItemsView
where TViewController : ItemsViewController<TItemsView>
{
readonly WeakReference<TViewController> _viewController;

public ItemsViewLayout ItemsViewLayout { get; }
public TViewController ViewController { get; }
public TViewController ViewController => _viewController.TryGetTarget(out var vc) ? vc : null;

protected float PreviousHorizontalOffset, PreviousVerticalOffset;

public ItemsViewDelegator(ItemsViewLayout itemsViewLayout, TViewController itemsViewController)
{
ItemsViewLayout = itemsViewLayout;
ViewController = itemsViewController;
_viewController = new(itemsViewController);
}

public override void Scrolled(UIScrollView scrollView)
Expand All @@ -45,8 +47,12 @@ public override void Scrolled(UIScrollView scrollView)
LastVisibleItemIndex = lastVisibleItemIndex
};

var itemsView = ViewController.ItemsView;
var source = ViewController.ItemsSource;
var viewController = ViewController;
if (viewController is null)
return;

var itemsView = viewController.ItemsView;
var source = viewController.ItemsSource;
itemsView.SendScrolled(itemsViewScrolledEventArgs);

PreviousHorizontalOffset = (float)contentOffsetX;
Expand Down Expand Up @@ -119,15 +125,19 @@ public override void CellDisplayingEnded(UICollectionView collectionView, UIColl

protected virtual (bool VisibleItems, NSIndexPath First, NSIndexPath Center, NSIndexPath Last) GetVisibleItemsIndexPath()
{
var indexPathsForVisibleItems = ViewController.CollectionView.IndexPathsForVisibleItems.OrderBy(x => x.Row).ToList();
var collectionView = ViewController?.CollectionView;
if (collectionView is null)
return default;

var indexPathsForVisibleItems = collectionView.IndexPathsForVisibleItems.OrderBy(x => x.Row).ToList();

var visibleItems = indexPathsForVisibleItems.Count > 0;
NSIndexPath firstVisibleItemIndex = null, centerItemIndex = null, lastVisibleItemIndex = null;

if (visibleItems)
{
firstVisibleItemIndex = indexPathsForVisibleItems.First();
centerItemIndex = GetCenteredIndexPath(ViewController.CollectionView);
centerItemIndex = GetCenteredIndexPath(collectionView);
lastVisibleItemIndex = indexPathsForVisibleItems.Last();
}

Expand Down Expand Up @@ -166,7 +176,7 @@ static NSIndexPath GetCenteredIndexPath(UICollectionView collectionView)

public override CGSize GetSizeForItem(UICollectionView collectionView, UICollectionViewLayout layout, NSIndexPath indexPath)
{
return ViewController.GetSizeForItem(indexPath);
return ViewController?.GetSizeForItem(indexPath) ?? CGSize.Empty;
}
}
}
7 changes: 6 additions & 1 deletion src/Controls/src/Core/Handlers/Items/iOS/ItemsViewLayout.cs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ public abstract class ItemsViewLayout : UICollectionViewFlowLayout
CGSize _adjustmentSize0;
CGSize _adjustmentSize1;
CGSize _currentSize;
WeakReference<Func<UICollectionViewCell>> _getPrototype;

const double ConstraintSizeTolerance = 0.00001;

Expand All @@ -28,7 +29,11 @@ public abstract class ItemsViewLayout : UICollectionViewFlowLayout

public nfloat ConstrainedDimension { get; set; }

public Func<UICollectionViewCell> GetPrototype { get; set; }
public Func<UICollectionViewCell> GetPrototype
{
get => _getPrototype is not null && _getPrototype.TryGetTarget(out var func) ? func : null;
set => _getPrototype = new(value);
}

internal ItemSizingStrategy ItemSizingStrategy { get; private set; }

Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#nullable disable
using System;
using System.Collections;
using Foundation;
using ObjCRuntime;
Expand Down Expand Up @@ -26,8 +27,12 @@ protected override NSIndexPath[] CreateIndexesFrom(int startIndex, int count)
return base.CreateIndexesFrom(startIndex, count);
}

var collectionView = CollectionView;
if (collectionView is null)
return Array.Empty<NSIndexPath>();

return IndexPathHelpers.GenerateLoopedIndexPathRange(Section,
(int)CollectionView.NumberOfItemsInSection(Section), LoopBy, startIndex, count);
(int)collectionView.NumberOfItemsInSection(Section), LoopBy, startIndex, count);
}
}
}
44 changes: 28 additions & 16 deletions src/Controls/src/Core/Handlers/Items/iOS/ObservableItemsSource.cs
Original file line number Diff line number Diff line change
Expand Up @@ -10,17 +10,15 @@ namespace Microsoft.Maui.Controls.Handlers.Items
{
internal class ObservableItemsSource : IObservableItemsViewSource
{
readonly UICollectionViewController _collectionViewController;
protected readonly UICollectionView CollectionView;
readonly WeakReference<UICollectionViewController> _collectionViewController;
readonly bool _grouped;
readonly int _section;
readonly IEnumerable _itemsSource;
bool _disposed;

public ObservableItemsSource(IEnumerable itemSource, UICollectionViewController collectionViewController, int group = -1)
{
_collectionViewController = collectionViewController;
CollectionView = _collectionViewController.CollectionView;
_collectionViewController = new(collectionViewController);

_section = group < 0 ? 0 : group;
_grouped = group >= 0;
Expand All @@ -35,6 +33,8 @@ public ObservableItemsSource(IEnumerable itemSource, UICollectionViewController
internal event NotifyCollectionChangedEventHandler CollectionViewUpdating;
internal event NotifyCollectionChangedEventHandler CollectionViewUpdated;

internal UICollectionView CollectionView => _collectionViewController.TryGetTarget(out var controller) ? controller.CollectionView : null;

public int Count { get; private set; }

public int Section => _section;
Expand Down Expand Up @@ -125,9 +125,13 @@ void CollectionChanged(object sender, NotifyCollectionChangedEventArgs args)

void CollectionChanged(NotifyCollectionChangedEventArgs args)
{
if (!_collectionViewController.TryGetTarget(out var controller))
return;

// Force UICollectionView to get the internal accounting straight
if (!CollectionView.Hidden)
CollectionView.NumberOfItemsInSection(_section);
var collectionView = controller.CollectionView;
if (!collectionView.Hidden)
collectionView.NumberOfItemsInSection(_section);

switch (args.Action)
{
Expand All @@ -153,14 +157,18 @@ void CollectionChanged(NotifyCollectionChangedEventArgs args)

void Reload()
{
if (!_collectionViewController.TryGetTarget(out var controller))
return;

var args = new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Reset);

Count = ItemsCount();

OnCollectionViewUpdating(args);

CollectionView.ReloadData();
CollectionView.CollectionViewLayout.InvalidateLayout();
var collectionView = controller.CollectionView;
collectionView.ReloadData();
collectionView.CollectionViewLayout.InvalidateLayout();

OnCollectionViewUpdated(args);
}
Expand All @@ -177,7 +185,7 @@ void Add(NotifyCollectionChangedEventArgs args)
var startIndex = args.NewStartingIndex > -1 ? args.NewStartingIndex : IndexOf(args.NewItems[0]);

// Queue up the updates to the UICollectionView
Update(() => CollectionView.InsertItems(CreateIndexesFrom(startIndex, count)), args);
Update(c => c.InsertItems(CreateIndexesFrom(startIndex, count)), args);
}

void Remove(NotifyCollectionChangedEventArgs args)
Expand All @@ -196,7 +204,7 @@ void Remove(NotifyCollectionChangedEventArgs args)
var count = args.OldItems.Count;
Count -= count;

Update(() => CollectionView.DeleteItems(CreateIndexesFrom(startIndex, count)), args);
Update(c => c.DeleteItems(CreateIndexesFrom(startIndex, count)), args);
}

void Replace(NotifyCollectionChangedEventArgs args)
Expand All @@ -209,7 +217,7 @@ void Replace(NotifyCollectionChangedEventArgs args)

// We are replacing one set of items with a set of equal size; we can do a simple item range update

Update(() => CollectionView.ReloadItems(CreateIndexesFrom(startIndex, newCount)), args);
Update(c => c.ReloadItems(CreateIndexesFrom(startIndex, newCount)), args);
return;
}

Expand All @@ -228,14 +236,14 @@ void Move(NotifyCollectionChangedEventArgs args)
var oldPath = NSIndexPath.Create(_section, args.OldStartingIndex);
var newPath = NSIndexPath.Create(_section, args.NewStartingIndex);

Update(() => CollectionView.MoveItem(oldPath, newPath), args);
Update(c => c.MoveItem(oldPath, newPath), args);
return;
}

var start = Math.Min(args.OldStartingIndex, args.NewStartingIndex);
var end = Math.Max(args.OldStartingIndex, args.NewStartingIndex) + count;

Update(() => CollectionView.ReloadItems(CreateIndexesFrom(start, end)), args);
Update(c => c.ReloadItems(CreateIndexesFrom(start, end)), args);
}

internal int ItemsCount()
Expand Down Expand Up @@ -281,15 +289,19 @@ internal int IndexOf(object item)
return -1;
}

void Update(Action update, NotifyCollectionChangedEventArgs args)
void Update(Action<UICollectionView> update, NotifyCollectionChangedEventArgs args)
{
if (CollectionView.Hidden)
if (!_collectionViewController.TryGetTarget(out var controller))
return;

var collectionView = controller.CollectionView;
if (collectionView.Hidden)
{
return;
}

OnCollectionViewUpdating(args);
update();
update(collectionView);
OnCollectionViewUpdated(args);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ public override NSIndexPath GetTargetIndexPathForMove(UICollectionView collectio
{
NSIndexPath targetIndexPath;

var itemsView = ViewController.ItemsView;
var itemsView = ViewController?.ItemsView;
if (itemsView?.IsGrouped == true)
{
if (originalIndexPath.Section == proposedIndexPath.Section || itemsView.CanMixGroups)
Expand Down
Loading

0 comments on commit 84128e6

Please sign in to comment.