Skip to content

[ios] fix memory leak in CollectionView cells#15831

Merged
rmarinho merged 1 commit intodotnet:mainfrom
jonathanpeppers:FixUITableViewCellLeaks
Jun 26, 2023
Merged

[ios] fix memory leak in CollectionView cells#15831
rmarinho merged 1 commit intodotnet:mainfrom
jonathanpeppers:FixUITableViewCellLeaks

Conversation

@jonathanpeppers
Copy link
Copy Markdown
Member

Context: #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 #14664 to verify if it is fully solved.

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.
@jonathanpeppers jonathanpeppers added platform/ios legacy-area-perf Startup / Runtime performance labels Jun 23, 2023
@rmarinho rmarinho requested review from hartez and rmarinho June 23, 2023 16:29
@jonathanpeppers jonathanpeppers marked this pull request as ready for review June 25, 2023 00:52
@rmarinho rmarinho closed this Jun 26, 2023
@rmarinho rmarinho reopened this Jun 26, 2023
@rmarinho rmarinho enabled auto-merge (squash) June 26, 2023 16:42
@rmarinho rmarinho merged commit 05697a6 into dotnet:main Jun 26, 2023
@jonathanpeppers jonathanpeppers deleted the FixUITableViewCellLeaks branch June 27, 2023 00:36
@jonathanpeppers jonathanpeppers added perf/memory-leak 💦 Memory usage grows / objects live forever (sub: perf) and removed legacy-area-perf Startup / Runtime performance labels Jul 12, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 9, 2023
@samhouts samhouts added the fixed-in-8.0.0-preview.6.8686 Look for this fix in 8.0.0-preview.6.8686! label Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

fixed-in-8.0.0-preview.6.8686 Look for this fix in 8.0.0-preview.6.8686! perf/memory-leak 💦 Memory usage grows / objects live forever (sub: perf) platform/ios

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants