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

Navigation Memory Leak #934

Open
mhmd-azeez opened this issue Jun 23, 2019 · 25 comments
Open

Navigation Memory Leak #934

mhmd-azeez opened this issue Jun 23, 2019 · 25 comments
Assignees
Labels
area-Navigation Frame, Page bug Something isn't working needs-winui-3 Indicates that feature can only be done in WinUI 3.0 or beyond. (needs winui 3) team-Controls Issue for the Controls team

Comments

@mhmd-azeez
Copy link

Describe the bug
We have noticed that our UWP apps have memory leaks. I have investigated it and found out that when navigating to new pages, the memory gets higher and doesn't seem to go down by much even when GC runs. The pages do get garbage collected (Their finalizer is run), but it seems like some kind of unmanaged memory is not properly cleaned up.

Steps to reproduce the bug

I have put together a small repro that consists of two pages:

  1. MainPage
<Page>
    <Grid>
        <Grid.RowDefinitions>
            <RowDefinition Height="70" />
            <RowDefinition  />
        </Grid.RowDefinitions>

        <Button x:Name="navigateButton" Content="Navigate" HorizontalAlignment="Center"
                    Click="NavigateButton_Click" />

        <Frame x:Name="mainFrame" IsNavigationStackEnabled="False"
               Padding="10" Grid.Row="1" />
    </Grid>
</Page>
    public sealed partial class MainPage : Page
    {
        public MainPage()
        {
            this.InitializeComponent();
        }

        private void NavigateButton_Click(object sender, RoutedEventArgs e)
        {
            mainFrame.Navigate(typeof(Page1), null, new Windows.UI.Xaml.Media.Animation.DrillInNavigationTransitionInfo());
        }
    }
  1. Page1:
<Page>
    <VariableSizedWrapGrid ItemWidth="100" ItemHeight="60">
        <Button Padding="20, 10" Content="Hello!"/>
        <Button Padding="20, 10" Content="Hello!"/>
        <Button Padding="20, 10" Content="Hello!"/>
        <Button Padding="20, 10" Content="Hello!"/>
        <Button Padding="20, 10" Content="Hello!"/>

        <!-- And 25 more buttons here -->
    </VariableSizedWrapGrid>
</Page>
    public sealed partial class Page1 : Page
    {
        public Page1()
        {
            this.InitializeComponent();
        }

        ~Page1()
        {
            Debug.WriteLine("Page dead :(");
        }
    }

The full source code is available on GitHub.

I have also tried out setting Frame.IsNavigationStackEnabled to false, it doesn't help.

What am I doing wrong here?

Expected behavior
Memory is reclaimed after Garbage collector runs

Screenshots
An image of memory use after clicking on the Navigate button for a while

And you can see a video of the repro.

Version Info
NuGet package version:
N/A (Not using WinUI)

Windows 10 version Saw the problem?
Insider Build (xxxxx)
May 2019 Update (18362) Yes
October 2018 Update (17763) Yes
April 2018 Update (17134)
Fall Creators Update (16299)
Creators Update (15063)
Anniversary Update (14393)
Device form factor Saw the problem?
Desktop Yes
Mobile
Xbox
Surface Hub
IoT

Additional context

@jevansaks
Copy link
Member

This issue is also cross-posted to https://stackoverflow.com/questions/56675628/uwp-navigation-memory-leak. We can investigate as a platform bug and report back to stack overflow when we understand this.

@MikeHillberg can you debug this repro?

@msft-github-bot msft-github-bot added the needs-assignee-attention Assignee needs to follow-up or investigate this issue label Jun 24, 2019
@r7dev
Copy link

r7dev commented Jul 11, 2019

I also have this problem with the same settings cited.

@jevansaks jevansaks added bug Something isn't working needs-winui-3 Indicates that feature can only be done in WinUI 3.0 or beyond. (needs winui 3) labels Jul 11, 2019
@msft-github-bot msft-github-bot removed the needs-assignee-attention Assignee needs to follow-up or investigate this issue label Jul 11, 2019
@r7dev
Copy link

r7dev commented Jul 11, 2019

@Opiumtm explain it here, but it doesn't demonstrate the code.

@cheles
Copy link
Member

cheles commented Sep 20, 2019

I leave my comment here, although it's an old post, for others that might search for it.

We made huge improvements in memory handeling and GC calls with latest UWP 6.2.9 Nuget update while targeting >=RS4 . The complete release notes are here: https://github.com/microsoft/dotnet/blob/master/releases/UWP/net-native2.2/README.md

@mhmd-azeez
Copy link
Author

@cheles I am not using WinUI currently, is there a way for me to use these latest changes?

@cheles
Copy link
Member

cheles commented Sep 23, 2019

@Encrypt0r Microsoft.NETCore.UniversalWindowsPlatform is the default package for any UWP application. aren't you using this package?

@mhmd-azeez
Copy link
Author

@cheles I actually do! Thanks, that's great to hear and I hope you continue these optimizations

@cheles
Copy link
Member

cheles commented Sep 23, 2019

cool. test it and let us know if we can close this one out.

@mhmd-azeez
Copy link
Author

@cheles unfortunately, it doesn't seem to help with the repro. I have disabled NavigationStack on the frame and yet, no memory is reclaimed after 3 garbage collections.

image

@jevansaks jevansaks added the area-Navigation Frame, Page label Sep 23, 2019
@jevansaks jevansaks added the team-Controls Issue for the Controls team label Nov 7, 2019
@florian-alexandre
Copy link

Hi !
Any news about this problem please ?

@cheles
Copy link
Member

cheles commented Jan 22, 2020

@Encrypt0r are you debugging a Debug build or Release? GC is implemented differently and, in general, it's an expensive. In Release you will see GC being called more rarely than Debug.

93 MB out of, probably 3/4GB or RAM is not dangerously enough to cause GC to clean gen 0/1/2 objects. I've tried your sample in Debug and I've barely got to 80 MB after 80+ clicks.
Try and see if you manage to reach at least 100MB+ or create an UI test with [WinAppDriver ] (https://github.com/microsoft/WinAppDriver/tree/master/Tests/UWPControls) to reach a higher memory consumption and observe GC calls

@kmgallahan
Copy link
Contributor

kmgallahan commented Jan 22, 2020

Along the lines of what @cheles mentioned, I'd recommend you give this article a read. In particular item 4 of "What Causes Memory Leaks?".

@cheles
Copy link
Member

cheles commented Jan 22, 2020

Also, if you want a more in-depth way to inspect memory consumption & GC calls, use Performance Monitor (Win10 built-in) https://devblogs.microsoft.com/dotnet/gc-performance-counters/

@CodeForCSharp
Copy link

@cheles This problem has existed for a long time and has not been solved, even with the updated Microsoft.NETCore.UniversalWindowsPlatform to lastest.
I've seen a lot of people mention things like this. such as dotnet/runtime#12255.
Look at the last few replies,from the snapshot,ConditionalWeakTable leak memory.
In other case,it is not necessary to switch pages. Frequent remove and add interface elements can also cause this problem.
We don't know what's going on inside, but that's how it shows.

@cheles
Copy link
Member

cheles commented May 25, 2020

@CodeForCSharp I haven't said that UWP team fixed all memory issues in UWP. I just said that UWP 6.2.9 brought some memory consumption improvements per this link

Adjust garbage collection timing during interop to eliminate memory increase in UWP app when navigating between pages

dotnet/runtime#12255 seems to have been marked for Future release but I don't see any due date to it.

@CodeForCSharp
Copy link

@cheles Thank you for your work.I know it's getting better.
Many questions about UWP are usually unanswered, which is very disturbing.
UWP Framework also have many memory issues.Hope to fix it as soon as possible.

@abk90007
Copy link

@cheles, I am using latest version 6.2.10, and still facing this memory leak issue, which leads to app memory to 1 GB.
Is there any fixed date for the resolution of this issue?

@cxllxn
Copy link

cxllxn commented Feb 22, 2022

Why doesn't Microsoft test it and fix it?

@Blinue
Copy link

Blinue commented Jul 29, 2022

This problem also exists when using C++/WinRT. The destructor does get called when switching pages, but the memory continues to grow.

It's disappointing that this bug has not been fixed after three years.

@JoverQian
Copy link

I have the same problem. When I press the ‘X’ to close the app, the last Page in Frame was not released, the destructor of last Page not called.

@github-actions
Copy link

This issue is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@garrettpauls
Copy link

This is still an issue in WinUI3 using Microsoft.WindowsAppSDK version 1.5.240802000 (the latest version as of 2024-08-14). I've attached a sample reproduction against WinUI3 (please let me know if I should recreate this as a separate issue, since this one was originally for UWP).

The example has three parts:

  • SmallPage - a page with just a few basic controls
  • LargePage - a page with a ListView of 100 items, each of which has several controls in its data template. This is to replicate the number of controls on a data-heavy page of our line of business app we've written in WinUI3.
  • MainWindow - the main window, which hosts a Frame that exhibits the problem, a textblock to show information about the frame and current page, and a button to trigger GC.Collect()

I have set IsNavigationStackEnabled to False for the frame, and both pages are using the default NavigationCacheMode of Disabled, so WinUI3/WinRT should not be attempting to cache any data.

If we watch the memory usage in Visual Studio we can see that the memory jumps up by a few MB every time SmallPage navigates to LargePage, and does not go back down when LargePage is navigated away from or when GC.Collect is explicitly called. Visual Studio doesn't show it well, but I've also profiled this same issue with dotMemory in our main application (unpackaged) and you can see that the memory that isn't being released is native memory, not managed, which indicates it's WinRT causing the leak.

Please give us a status update on this, our application is expected to be run by our customers for most of their day without having to restart it, and they run in a shared environment. Large memory leaks like this are a problem.

FrameMemoryLeakDemo
NavigationMemoryLeak.zip

For those curious, this is what our application does when it runs into this problem (app is opened, the data-heavy page is opened, the page is closed, the page is opened again, etc):
image

@r7dev
Copy link

r7dev commented Aug 15, 2024

Hello @garrettpauls I hope you are well.
ItemsSource inside the ListView is not being cleaned up.
Fortunately, you can clean up before navigating to the next page.

<ListView Grid.Row="2" ItemsSource="{x:Bind Items}" x:Name="lstView"

lstView.ItemsSource = null;
lstView.Items.Clear();

You need to apply the architecture correctly so you don't have memory leaks. Take this example of architecture. https://github.com/microsoft/InventorySample

@garrettpauls
Copy link

@r7dev where is that documented? Coming from a WPF background unsetting ItemsSource has never been necessary before, and I don't see any indication that should be necessary now according to the documentation for that ItemsControl.ItemsSource or the documentation for ListView (a sample is not documentation)...

That said, I did give it a try by manually setting and unsetting ItemsSource, and it does make a difference, however there is still a leak somewhere. Are there other places we're also expected to be manually unsetting values? Is there a way to know what properties need to be manually unset when a page is navigated away from that I can reference?

protected override void OnNavigatedTo(NavigationEventArgs e)
{
	base.OnNavigatedTo(e);
	listView.ItemsSource = Items;
}

protected override void OnNavigatedFrom(NavigationEventArgs e)
{
	base.OnNavigatedFrom(e);
	listView.ItemsSource = null;
}

Thank you for pointing this out, it's frustrating that it doesn't seem documented anywhere, so I appreciate you mentioning it. I'll see if I can get our app to manually clean up all our ItemsSource bindings and hopefully that will improve the situation.

@garrettpauls
Copy link

In case anyone else needs it, I ended up writing a little helper class as a quick fix to this while we evaluate more direct options. I'm calling CleanUp.FrameworkElement on our pages when they're navigated away from (and not being cached), and it has significantly cut down our app's constant memory growth on page navigation.

Here's a simplified version that should be fine for common use:

public static class CleanUp
{
	public static void FrameworkElement( FrameworkElement element )
	{
		var count = VisualTreeHelper.GetChildrenCount( element );
		for( var index = 0; index < count; index++ )
		{
			var child = VisualTreeHelper.GetChild( element, index );
			if( child is FrameworkElement childElement )
			{
				FrameworkElement( childElement );
			}
		}

		switch( element )
		{
			case ItemsControl itemsControl:
				itemsControl.ItemsSource = null;
				break;
			case ItemsRepeater itemsRepeater:
				itemsRepeater.ItemsSource = null;
				break;
			case TabView tabView:
				tabView.TabItemsSource = null;
				break;
		}
	}
}

then in our shared PageBase class:

protected override void OnNavigatedFrom( NavigationEventArgs args )
{
	base.OnNavigatedFrom( args );

	if( NavigationCacheMode == NavigationCacheMode.Disabled )
	{
		CleanUp.FrameworkElement( this );
	}
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Navigation Frame, Page bug Something isn't working needs-winui-3 Indicates that feature can only be done in WinUI 3.0 or beyond. (needs winui 3) team-Controls Issue for the Controls team
Projects
None yet
Development

No branches or pull requests