maui icon indicating copy to clipboard operation
maui copied to clipboard

CollectionView is not currently being Garbage Collected

Open marco-skizza opened this issue 1 year ago • 14 comments

Description

Hi

With the following Xaml on a page, the page doesn't get garbage collected when navigating back:

        <CollectionView ItemsSource="{Binding Items}">
            <CollectionView.ItemTemplate >
                <DataTemplate>
                    <Grid>

                        <Label Text="Test" />

                    </Grid>
                </DataTemplate>
            </CollectionView.ItemTemplate>
        </CollectionView>

Steps to Reproduce

  • Run the project
  • Click Button and go back again multiple times
  • Observe that ~MyPage() never gets called

Link to public reproduction project repository

https://github.com/marco-skizza/MauiCollectionView

Version with bug

8.0.7 SR2

Is this a regression from previous behavior?

Not sure, did not test other versions

Last version that worked well

Unknown/Other

Affected platforms

iOS, I was not able test on other platforms

Affected platform versions

iPadOS 17.3.1 on iPad mini 6

Did you find any workaround?

No

Relevant log output

No response

marco-skizza avatar Feb 19 '24 18:02 marco-skizza

We've added this issue to our backlog, and we will work to address it as time and resources allow. If you have any additional information or questions about this issue, please leave a comment. For additional info about issue management, please read our Triage Process.

ghost avatar Feb 20 '24 19:02 ghost

As I observed, the CollectionView also leaks when setting the ItemsSource in the code behind directly. The CollectionView seems to leak as long as the ItemTemplate is not "empty", e.g. as long as it contains some "real" elements like the Label in my sample.

marco-skizza avatar Feb 24 '24 15:02 marco-skizza

@marco-skizza can you add some GC.Collect() calls to test this? See:

  • https://github.com/dotnet/maui/wiki/Memory-Leaks#determining-if-there-is-a-leak-or-not

jonathanpeppers avatar Mar 14 '24 21:03 jonathanpeppers

Hi @jonathanpeppers

The GC.Collect() is in the MainPage.xaml.cs after navigating back again from the page with the CollectionView:

https://github.com/marco-skizza/MauiCollectionView/blob/756c7e874465aee645a7ebe59bffa3555ee68bfa/MauiCollectionView/MainPage.xaml.cs#L15-L21

But tell me if I should add some more of those calls to the OnAppearing() to make it more deterministic...

P.S.: With the current code I have to navigate forth and back several times to see the effect of GC.Collect().

P.P.S.: I tested this with the Debug build. But so far, this approach was good enough for me.

marco-skizza avatar Mar 15 '24 06:03 marco-skizza

It's possible there is an issue, CollectionView isn't on this list where we test each control for leaks:

https://github.com/dotnet/maui/blob/23069ff420f98c25545905e730f2d70f0927f2e4/src/Controls/tests/DeviceTests/Memory/MemoryTests.cs#L58-L85

jonathanpeppers avatar Mar 15 '24 15:03 jonathanpeppers

Hi @marco-skizza I just added the tests and was trying to debug same example but seems at least on main we don't have this issue, the 1st run the destructor isn't called, but if you navigate more it gets logged.

2024-03-15 17:19:53.763501+0000 Maui.Controls.Sample.Sandbox[75517:3267940] Memory: 6206824
2024-03-15 17:23:26.327649+0000 Maui.Controls.Sample.Sandbox[75517:3267940] Memory: 6643256
2024-03-15 17:23:32.283356+0000 Maui.Controls.Sample.Sandbox[75517:3268037] ~TestPage() called
2024-03-15 17:23:32.284604+0000 Maui.Controls.Sample.Sandbox[75517:3268037] ~MyViewModel() called
2024-03-15 17:23:32.292096+0000 Maui.Controls.Sample.Sandbox[75517:3267940] Memory: 6729256
2024-03-15 17:23:42.736095+0000 Maui.Controls.Sample.Sandbox[75517:3268037] ~TestPage() called
2024-03-15 17:23:42.736485+0000 Maui.Controls.Sample.Sandbox[75517:3268037] ~MyViewModel() called
2024-03-15 17:23:42.742458+0000 Maui.Controls.Sample.Sandbox[75517:3267940] Memory: 6735248

here's my sample coe, you can add on Sandbox app on our repo ..

public partial class MainPage : ContentPage
	{
		public MainPage()
		{
			InitializeComponent();
		    Appearing += OnAppearing;
        }

        void OnAppearing(object? sender, EventArgs e)
        {
            GC.Collect();
            GC.WaitForPendingFinalizers();
            var totalMemory = GC.GetTotalMemory(true);
            Console.WriteLine($"Memory: {totalMemory}");
        }

        async void Button_OnClicked(object? sender, EventArgs e)
        {
            await Navigation.PushAsync(new TestPage());
        }
	}

	public class MyViewModel
    {
        public ObservableCollection<string> Items { get; } = ["1", "2", "3"];

        ~MyViewModel()
        {
            Console.WriteLine("~MyViewModel() called");
        }
    }

	public class TestPage : ContentPage
	{
		public TestPage()
		{
			var collectionView = new CollectionView();
			collectionView.SetBinding(ItemsView.ItemsSourceProperty, "Items");
			collectionView.ItemTemplate = new DataTemplate(() =>
			{
				var label = new Label();
				label.SetBinding(Label.TextProperty, ".");
				return label;
			});
			Content = collectionView;

			BindingContext = new MyViewModel();
		}

   		~TestPage()
    	{
        	Console.WriteLine("~TestPage() called");
    	}
}

rmarinho avatar Mar 15 '24 17:03 rmarinho

See https://github.com/dotnet/maui/issues/21249, I was able to reproduce it as well through their example. seeing as their example was with XAML, I think this issue should be reopened.

drasticactions avatar Mar 17 '24 06:03 drasticactions

Hi @rmarinho

Can you please remove the "needs-info" label from this issue. You find my feedback under: https://github.com/dotnet/maui/issues/21249 I have also updated my sample code...

Thanks!

marco-skizza avatar Mar 22 '24 05:03 marco-skizza

I've encountered this issue as well and can easily replicate it. The page does not collected when using a CollectionView and a DataTemplate that is defined in XAML, either directly or through the Resources of the Page.

The memory leak does not occur when setting a DataTemplate in the code-behind, nor when pointing to a template in the App.xaml resources. Hopefully that points the way to the underlying issue.

EGoverde avatar Mar 28 '24 13:03 EGoverde

In reference to the reload problem @marco-skizza mentioned: I also created a repro sample that might be identical or at least related. Issue: https://github.com/dotnet/maui/issues/18030

sjorsmiltenburg avatar Apr 08 '24 20:04 sjorsmiltenburg

I am also able to reproduce this on .NET MAUI 8.0.20 on iOS on a real device, as soon as I add collection view to my page, the page is no longer garbage collected by GC.Collect(). Works as expected on Android.

<CollectionView ItemsSource="{Binding TestData}">
    <CollectionView.ItemTemplate>
        <DataTemplate>
            <Label Text="test"/>
        </DataTemplate>
     </CollectionView.ItemTemplate>
</CollectionView>

brentpbc avatar Apr 10 '24 23:04 brentpbc

Hi @rmarinho

I hope this issue hasn't been forgotten, because it has already been closed before. As I noticed, in MAUI SDK Ongoing it's state is already on Done.

Please understand that this issue is really a blocker for me. I can't release my app, knowing it will crash eventually due to this memory leak. And I think a lot of developers use a CollectionView in their code, not even knowing they will face this problem...

marco-skizza avatar Apr 13 '24 06:04 marco-skizza

This one is pretty nasty...

Using dotnet-gcdump with a Release build of the app, I see:

image

You can cause the C# version to leak, if you make the () => an instance method instead:

  • https://github.com/jonathanpeppers/iOSMauiCollectionView/commit/3e5fb021fee3b512d8163c53224e17b76c3789c2

So I believe there is a "cycle" that causes the problem here:

  • Microsoft.Maui.Controls.Handlers.Items.VerticalCell (note this is a UICollectionViewCell) ->
  • DataTemplate ->
  • Func<object> ->
  • PageXamlLeak (or PageCsOk w/ my change) ->
  • CollectionView ->
  • Microsoft.Maui.Controls.Handlers.Items.VerticalCell

I'm not sure I've seen a leak with a cycle this complicated.

I need to test, but I think the change should go in VerticalCell at the iOS/Catalyst platform level. Likely we just make this DataTemplate a WeakReference?

https://github.com/dotnet/maui/blob/7f85d2a544f0cf6c60bccf2d746e2995208af6a7/src/Controls/src/Core/Handlers/Items/iOS/TemplatedCell.cs#L21

The events here also could be problematic:

https://github.com/dotnet/maui/blob/7f85d2a544f0cf6c60bccf2d746e2995208af6a7/src/Controls/src/Core/Handlers/Items/iOS/TemplatedCell.cs#L14-L15

jonathanpeppers avatar Apr 15 '24 21:04 jonathanpeppers

@jonathanpeppers Thanks for looking at this :)

brentpbc avatar Apr 15 '24 23:04 brentpbc