Skip to content

Commit

Permalink
[ios] fix memory leak in CollectionView cells
Browse files Browse the repository at this point in the history
Context: dotnet#14664
Context: https://github.com/nacompllo/MemoryLeakEverywhere/tree/bugfix/memoryLeakItemsSource

In reviewing our latest changes in main with the above customer sample,
I found that there appeared to be leaks related to MAUI's
`UITableViewCell` subclasses when using `CollectionView`.

I was able to reproduce the issue in a test, such as:

    // DataTemplate saves WeakReference to the View in a list
    collectionView.ItemTemplate = new DataTemplate(() =>
    {
        var label = new Label();
        labels.Add(new(label));
        return label;
    });

    // Create a cell and bind it to the template:
    cell = new VerticalCell(CGRect.Empty);
    cell.Bind(collectionView.ItemTemplate, bindingContext, collectionView);

    // Check we have no leaks
    foreach (var reference in labels)
    {
        Assert.False(reference.IsAlive, "View should not be alive!");
    }

After isolating the issue, I found the issue was the
`TemplatedCell.PlatformHandler` property:

    internal IPlatformViewHandler PlatformHandler { get; private set; }

This stores a copy of the `LabelHandler` in our test/example.

The problem with `UITableViewCell` is that UIKit holds onto these and
reuses them. This means that UIKit may keep the `LabelHandler` alive
longer than needed.

It also appears to be a somewhat complex circular reference:

* `CollectionView` -> handlers / etc. -> `TemplatedCell` -> `LabelHandler` -> `Label` -> `CollectionView`

I made the `PlatformHandler` use a `WeakReference` as its backing
field and the problem goes away!

I will retest dotnet#14664 to verify if it is fully solved.
  • Loading branch information
jonathanpeppers committed Jun 23, 2023
1 parent 4de67ed commit 669c542
Show file tree
Hide file tree
Showing 2 changed files with 50 additions and 2 deletions.
8 changes: 7 additions & 1 deletion src/Controls/src/Core/Handlers/Items/iOS/TemplatedCell.cs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,13 @@ protected TemplatedCell(CGRect frame) : base(frame)
{
}

internal IPlatformViewHandler PlatformHandler { get; private set; }
WeakReference<IPlatformViewHandler> _handler;

internal IPlatformViewHandler PlatformHandler
{
get => _handler is not null && _handler.TryGetTarget(out var h) ? h : null;
set => _handler = value == null ? null : new(value);
}

public override void ConstrainTo(CGSize constraint)
{
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using System.Collections;
using System;
using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.Reflection;
Expand Down Expand Up @@ -90,5 +90,47 @@ await CreateHandlerAndAddToWindow<CollectionViewHandler>(collectionView, async h
Assert.Equal(margin, absPoint.X);
});
}

[Fact("Cells Do Not Leak")]
public async Task CellsDoNotLeak()
{
SetupBuilder();

var labels = new List<WeakReference>();
VerticalCell cell = null;

{
var bindingContext = "foo";
var collectionView = new CollectionView
{
ItemTemplate = new DataTemplate(() =>
{
var label = new Label();
labels.Add(new(label));
return label;
}),
};

var handler = await CreateHandlerAsync(collectionView);

await InvokeOnMainThreadAsync(() =>
{
cell = new VerticalCell(CGRect.Empty);
cell.Bind(collectionView.ItemTemplate, bindingContext, collectionView);
});

Assert.NotNull(cell);
Assert.NotEmpty(labels);
}

await Task.Yield();
GC.Collect();
GC.WaitForPendingFinalizers();

foreach (var reference in labels)
{
Assert.False(reference.IsAlive, "View should not be alive!");
}
}
}
}

0 comments on commit 669c542

Please sign in to comment.