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.

Sign up for GitHub

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

Revert "[ios] fix memory leak in CollectionView cells (#15831)" #24310

Closed
wants to merge 3 commits into from
Closed

Revert "[ios] fix memory leak in CollectionView cells (#15831)" #24310

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
79a5e38
Revert "[ios] fix memory leak in CollectionView cells (#15831)"
filipnavara Aug 19, 2024
4e123a3
Update CollectionViewTests.iOS.cs
jonathanpeppers Aug 19, 2024
e523d52
Update CellsDoNotLeak test to test whether the collection view itself…
filipnavara Aug 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 1 addition & 7 deletions src/Controls/src/Core/Handlers/Items/iOS/TemplatedCell.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,13 +49,7 @@ protected TemplatedCell(CGRect frame) : base(frame)
{
}

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);
}
internal IPlatformViewHandler PlatformHandler { get; private set; }

public override void ConstrainTo(CGSize constraint)
{
Expand Down
5 changes: 4 additions & 1 deletion src/Controls/tests/DeviceTests/Elements/CollectionView/CollectionViewTests.iOS.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ public async Task CellsDoNotLeak()

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

{
var bindingContext = "foo";
Expand All @@ -116,12 +117,14 @@ await InvokeOnMainThreadAsync(() =>
});

Assert.NotNull(cell);
collectionViewReference = new WeakReference(collectionView);
collectionView = null;
}

// HACK: test passes running individually, but fails when running entire suite.
// Skip the assertion on Catalyst for now.
#if !MACCATALYST
await AssertionExtensions.WaitForGC(labels.ToArray());
await AssertionExtensions.WaitForGC(collectionViewReference);
Comment on lines -124 to +127
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After these changes, when would the labels go away? Should we update this to check all the VerticalCell instances go away instead?

Copy link
Member Author

@filipnavara filipnavara Aug 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the VerticalCell was not created synthetically and held in local variable then they should be gone with the collection view.

That would essentially mean rewriting the test to do what ClearingItemsSourceClearsBindingContext does and at the end checking that dropping reference to collection view also allows collecting the cells (and then the attached labels).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(The ClearingItemsSourceClearsBindingContext test was moved/renamed - it was originally added in https://github.com/dotnet/maui/pull/14619/files#diff-a1aed29ff55ec6fe5da9c27f12285c91406be414338738b0bcc3106ec4aa675f)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's already tested by this:

maui/src/Controls/tests/DeviceTests/Elements/CollectionView/CollectionViewTests.cs

Lines 106 to 169 in 639fcb0

[Fact]
public async Task ItemsSourceDoesNotLeak()
{
SetupBuilder();
var weakReferences = new List<WeakReference>();
{
var labels = new List<Label>();
IList logicalChildren = null;
var collectionView = new CollectionView
{
Header = new Label { Text = "Header" },
Footer = new Label { Text = "Footer" },
ItemTemplate = new DataTemplate(() =>
{
var label = new Label();
labels.Add(label);
return label;
}),
};
var navPage = new NavigationPage(new ContentPage { Title = "Page 1" });
await CreateHandlerAndAddToWindow<WindowHandlerStub>(new Window(navPage), async handler =>
{
await navPage.PushAsync(new ContentPage { Content = collectionView });
var data = new ObservableCollection<string>()
{
"Item 1",
"Item 2",
"Item 3"
};
weakReferences.Add(new(data));
collectionView.ItemsSource = data;
await Task.Delay(100);
Assert.NotEmpty(labels);
foreach (var label in labels)
{
weakReferences.Add(new(label));
weakReferences.Add(new(label.Handler));
weakReferences.Add(new(label.Handler.PlatformView));
}
// Get ItemsView._logicalChildren
var flags = BindingFlags.NonPublic | BindingFlags.Instance;
logicalChildren = typeof(Element).GetField("_internalChildren", flags).GetValue(collectionView) as IList;
Assert.NotNull(logicalChildren);
// Replace with cloned collection
collectionView.ItemsSource = new ObservableCollection<string>(data);
await Task.Delay(100);
await navPage.PopAsync();
});
Assert.NotNull(logicalChildren);
Assert.True(logicalChildren.Count <= 5, "_logicalChildren should not grow in size!");
}
await AssertionExtensions.WaitForGC([.. weakReferences]);
}

#endif
}

Expand Down
Loading