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.

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

[ios] fix memory leak in CollectionView cells #15831

Merged
merged 1 commit into from
Jun 26, 2023

Conversation

jonathanpeppers
Copy link
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 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! 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