dotnet / maui

.NET MAUI is the .NET Multi-platform App UI, a framework for building native device applications spanning mobile, tablet, and desktop.
https://dot.net/maui
MIT License
22.22k stars 1.75k forks source link

Memory Leak issue in ListView #16450

Closed Tamilarasan-Paranthaman closed 1 year ago

Tamilarasan-Paranthaman commented 1 year ago

Description

I have a CheckBox in the ListView DataTemplate and am switching the ListView's ItemsSource at runtime. Due to this, the app size gradually increases and eventually leads to a crash of the application.

https://github.com/dotnet/maui/assets/93904422/9a793432-fa67-488a-a07d-5ccb28fa655b

Steps to Reproduce

  1. Run the given sample.
  2. Continuously switch the ItemsSource.

Link to public reproduction project repository

https://github.com/Tamilarasan-Paranthaman/Maui-Itemssource-Switching-Issue

Version with bug

7.0.86

Last version that worked well

Unknown/Other

Affected platforms

Windows, I was not able test on other platforms

Affected platform versions

Windows

Did you find any workaround?

No response

Relevant log output

No response

MartyIX commented 1 year ago

(@jonathanpeppers works on https://github.com/dotnet/maui/pull/16346 (iOS memory leaks). CheckBox is mentioned there as well. Not sure if by any chance a fix there can fix it for Windows as well.)

jonathanpeppers commented 1 year ago

@Tamilarasan-Paranthaman does this problem happen on .NET 8? The test I added for CheckBox was already passing on Windows.

There are various other fixes that are not in .NET 7.

Tamilarasan-Paranthaman commented 1 year ago

@jonathanpeppers I have tested the provided sample with ".NET 8" on the "Windows" platform and encountered the same issue.

image

jonathanpeppers commented 1 year ago

So, it looks like even if you have an empty <ViewCell/> your sample app leaks:

image

Notice the overall heap size at the end got up to 3,964 kb.

I looked here:

image

The problem is every Cell subscribes:

https://github.com/dotnet/maui/blob/9fcccd57b5a3d664e4788b3c7fc3edc10010beaf/src/Controls/src/Core/Cells/Cell.cs#L197-L201

And so the one ListView keeps all these cells alive.

Now I do think everything would go away if you navigated away from this page. Or a workaround would be to create two ListView and hide/show them instead.

jonathanpeppers commented 1 year ago

I can reproduce it in a test here:

https://github.com/dotnet/maui/compare/main...jonathanpeppers:ListViewOhNo

This line appears to also be problematic, commented out along with RealParent.PropertyChanged & the test passes:

https://github.com/dotnet/maui/blob/9fcccd57b5a3d664e4788b3c7fc3edc10010beaf/src/Controls/src/Core/Element/Element.cs#L322

ghost commented 1 year ago

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.

XamlTest commented 1 year ago

Verified this on Visual Studio Enterprise 17.8.0 Preview 1.0. Repro on Windows 11 .NET 8 with below Project: 16450.zip

jonathanpeppers commented 1 year ago

I added some logging to track each data-bound item:

List<WeakReference> references = new();

public MainPage()
{
    InitializeComponent();

    listView.ItemTemplate = new DataTemplate(() =>
    {
        var cell = new ViewCell();
        references.Add(new(cell));
        return cell;
    });
}

void Log()
{
    Debug.WriteLine($"Cells alive: {references.Count(l => l.IsAlive)}");
}

It looks like this issue only happens with ListView:

Cells alive: 207
Cells alive: 236
Cells alive: 266
Cells alive: 295
Cells alive: 325
Cells alive: 354

Where if I change to a CollectionView and Label in each row:

Labels alive: 30
Labels alive: 30
Labels alive: 20
Labels alive: 21
Labels alive: 23
Labels alive: 20
Labels alive: 21
Labels alive: 23
Labels alive: 21
Labels alive: 30
Labels alive: 23
Labels alive: 20
Labels alive: 21
Labels alive: 20
Labels alive: 20

So, a second workaround is to use CollectionView instead, while we investigate the issue with ListView.

Tamilarasan-Paranthaman commented 1 year ago

@jonathanpeppers,

I have checked the same test case using the CollectionView. The issue has been reproduced in that particular sample as well. Upon switching between item sources, there's a noticeable increase in the application size, which ultimately leads to a crash. In the attached sample, I simply replaced ListView with CollectionView while keeping the CheckBox within the CollectionView DataTemplate.

<?xml version="1.0" encoding="utf-8" ?>
<ContentPage xmlns="http://schemas.microsoft.com/dotnet/2021/maui"
             xmlns:x="http://schemas.microsoft.com/winfx/2009/xaml"
             xmlns:local="clr-namespace:MauiApp1"
             x:Class="MauiApp1.MainPage">

    <ContentPage.BindingContext>
        <local:EmployeeInfoRepository x:Name="employeeInfoRepository"/>
    </ContentPage.BindingContext>

    <Grid RowDefinitions="Auto,*">
        <HorizontalStackLayout Grid.Row="0">
            <Button Text="ItemSource 1" Clicked="ItemSource_One_Button_Clicked"/>
            <Button Text="ItemSource 2" Clicked="ItemSource_Two_Button_Clicked"/>
        </HorizontalStackLayout>
        <CollectionView  Grid.Row="1" x:Name="listView" ItemsSource="{Binding EmployeesInfo1}">
            <CollectionView.ItemTemplate>
                <DataTemplate>
                    <CheckBox x:Name="checkBoxCell" Margin="10,0,0,0" IsChecked="{Binding IsChecked}"
                          VerticalOptions="Center">
                    </CheckBox>
                </DataTemplate>
            </CollectionView.ItemTemplate>
        </CollectionView>
    </Grid>

</ContentPage>

When I replaced the CheckBox with a label in the DataTemplate, the crash did not occur, and the application size did not increase significantly.

It seems that the presence of a CheckBox in the DataTemplate is a recurring issue for both the CollectionView and ListView.

https://github.com/dotnet/maui/assets/93904422/fa5d9a8f-c014-470e-919f-7ba0260f4b85

jonathanpeppers commented 1 year ago

Let me fix the underlying issue with all Cell's first, and investigate CheckBox second.

The control itself doesn't leak in isolation, as we have a test for it:

https://github.com/dotnet/maui/blob/0ebb60bc6db12e2a33d943991fd335da843954aa/src/Controls/tests/DeviceTests/Memory/MemoryTests.cs#L36-L51

jonathanpeppers commented 1 year ago

So, I'm testing a local build from PR #16762 (basically .NET 8).

CheckBox looks OK, if I click quickly the number goes up but then back down just fine:

CheckBox's alive: 30
CheckBox's alive: 59
CheckBox's alive: 59
CheckBox's alive: 29
CheckBox's alive: 35
CheckBox's alive: 59
CheckBox's alive: 59
CheckBox's alive: 30
CheckBox's alive: 30
CheckBox's alive: 30
CheckBox's alive: 30

My changes to the app are here: https://github.com/jonathanpeppers/Maui-Itemssource-Switching-Issue/commit/43af9ec6e1698f2fe87499c803dd8251f849a833

deviprasad987 commented 1 year ago

@MartyIX @jonathanpeppers @PureWeen @jsuarezruiz @samhouts In which VS version this memory leak issue will be fixed?

MartyIX commented 1 year ago

It should be in https://github.com/dotnet/maui/releases/tag/8.0.0-rc.1.9171. It mentions:

[windows] fix memory leak in ListView by @jonathanpeppers in https://github.com/dotnet/maui/pull/16762

(But literaly above your post one can see fixed-in-8.0.0-rc.1.9171.)

deviprasad987 commented 1 year ago

Is it available in VS preview#17.8.0-pre.1.0

Tamilarasan-Paranthaman commented 1 year ago

@jonathanpeppers, @samhouts,

I am still encountering the same issue in version 8.0.0-rc.1.9171, although it was mentioned as fixed. The app size gradually increases and leads to a crash of the application. I have attached my updated sample (using version 8.0.0-rc.1.9171) along with a reference video demonstrating the issue.

https://github.com/dotnet/maui/assets/93904422/18a15e0f-1311-4eb1-997c-abe30338f372

Sample Link: Application.zip

Additionally, while reviewing your application, I noticed that you added a Label without assigning any text within the ViewCell, instead of using a CheckBox. Is there a specific reason for this choice?

jonathanpeppers commented 1 year ago

https://github.com/dotnet/maui/pull/16762 fixed a general problem with all ViewCell's no matter what the contents were.

I will retest your app above, thanks.

jonathanpeppers commented 1 year ago

@Tamilarasan-Paranthaman in your video is XAML hot reload disabled? (I see the toolbar in your video, so I guess it might not be?)

image

XAML hot reload is known to cause diagnostic/profiling tools to show incorrect results, they display a warning on this screen:

image

This is mentioned here:

Tamilarasan-Paranthaman commented 1 year ago

@jonathanpeppers, the XAML hot reload is not disabled in my Visual Studio.

image

However, I'm still facing the same issue even after disabling it.

jonathanpeppers commented 1 year ago

@Tamilarasan-Paranthaman for testing this, do you also have some GC.Collect() calls in the app like I did here.

This is mentioned as a troubleshooting step that makes the GC at least slightly more deterministic:

(remove these when your testing is done)

I tested your app again on Friday with GC.Collect() calls, and I did not observe a leak. Memory went up and down for me -- it did not consistently grow.

ghost commented 1 year ago

Hi @Tamilarasan-Paranthaman. We have added the "s/needs-info" label to this issue, which indicates that we have an open question for you before we can take further action. This issue will be closed automatically in 7 days if we do not hear back from you by then - please feel free to re-open it if you come back to this issue after that time.

Tamilarasan-Paranthaman commented 1 year ago

@jonathanpeppers, I have tested the application with GC.Collect() calls, and the issue did not reproduce when XAML hot reload was disabled.

However, why does the issue reproduce when binding the Checkbox in the XAML page? This issue occurred in both XAML hot reload enabled and disabled cases. There is an issue when binding the Checkbox on the XMAL page?

jonathanpeppers commented 1 year ago

issue did not reproduce when XAML hot reload was disabled.

However, why does the issue reproduce when binding the Checkbox in the XAML page? This issue occurred in both XAML hot reload enabled and disabled cases. There is an issue when binding the Checkbox on the XMAL page?

So, you still see an issue? Or not?

If you remove x:Name in the <DataTemplate/> does that change anything for you? Maybe you can share an updated project/video, and I will try it again in the meantime.

Tamilarasan-Paranthaman commented 1 year ago

@jonathanpeppers, I'm still encountering the issue even after disabling XAML hot reload in my application, which leads to a crash. I have already attached my updated sample to this query https://github.com/dotnet/maui/issues/16450#issuecomment-1729249569.

As per your suggestion, I also test it by adding a checkbox along with GC.Collect() calls in the code-behind. In this scenario, the issue did not reproduce when XAML hot reload was disabled.

Samples XAML hot reload disabled XAML hot reload enabled
With this sample https://github.com/dotnet/maui/issues/16450#issuecomment-1729249569 Issue reproduced Issue reproduced
Sample with GC.Collect() calls like you did here. Issue not reproduced Issue reproduced

The difference between the two samples lies in the addition of a Checkbox, with one being added in XAML and the other in the code behind.

https://github.com/dotnet/maui/assets/93904422/b08d9185-9fbe-4776-809e-27eba16ec955

jonathanpeppers commented 1 year ago

Did you try this suggestion?

If you remove x:Name in the <DataTemplate/> does that change anything for you? Maybe you can share an updated project/video, and I will try it again in the meantime.

In the video why isn't there a GC.Collect() call on line 19 for testing?

To verify, you are using .NET 8 RC 1 and the project has net8.0 TargetFrameworks? If any say net7.0, then you are still using .NET 7 stable versions of MAUI.

jonathanpeppers commented 1 year ago

I tested with the .NET 8 RC 2 release today, only change to your sample was added GC.Collect() calls: Application.zip

I don't feel like I'm seeing a leak at all, when taking a snapshot between each button press:

image

image

@Tamilarasan-Paranthaman I would like to solve a memory issue here if there is one, but I'm not currently able to see it. If you can give more details or info, we can investigate further. Thanks!