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

WinUI 3 Page objects are being leaked on navigate in a C#/WinRT app #413

Closed
llongley opened this issue Sep 16, 2020 · 35 comments · Fixed by #782
Closed

WinUI 3 Page objects are being leaked on navigate in a C#/WinRT app #413

llongley opened this issue Sep 16, 2020 · 35 comments · Fixed by #782
Assignees
Labels
bug Something isn't working fixed Issue has been fixed in an upcoming or existing release gc Related to garbage collection pri-0 Blocking/issue has no workaround
Milestone

Comments

@llongley
Copy link
Member

I've put together two sample apps to demonstrate the same scenario, one in C++/WinRT, the other in C#/WinRT:

C++/WinRT: DesktopWinUICppLeakTest.zip

C#/WinRT: DesktopWinUICSharpLeakTest.zip

In both cases, the app contains a ListView containing 1,000 items, plus a button that navigates to and from a second page also containing a ListView with 1,000 items. In the C++/WinRT case, the memory footprint of the app stays the same after successive navigations, whereas in the C#/WinRT case, the memory footprint steadily grows. A heap dump analysis indicates that the root Page object in native code is what's leaking, so it's likely the case that something in the C#/WinRT projection is holding onto the native object past when it should be.

@Scottj1s Scottj1s added this to the Release Candidate 2 milestone Sep 16, 2020
@Scottj1s Scottj1s added pri-0 Blocking/issue has no workaround bug Something isn't working labels Sep 16, 2020
@Scottj1s Scottj1s self-assigned this Sep 17, 2020
@BenJKuhn BenJKuhn self-assigned this Sep 21, 2020
@AdamBraden
Copy link
Contributor

@angelazhangmsft - relnote for RC2.

@BenJKuhn
Copy link
Member

Here's an update on the investigation. We've tightened up reference cleanup, but there is still one lingering cycle, and it's a challenging one. This object will always leak upon creation:

class TestPage : Page { }

In simplest terms, to create the XAML page, CSWinRT creates a CCW for TestPage, so that it can override methods on Page. So far so good. However, Page is serving double-duty in this instance. It's both the access to the base implementation for the derived object, and the lifetime management for clients in C# to access the combined object. This net effect is that there's a strong reference cycle between the XAML page & CCW of TestPage, and the CCW isn't aggregating it's lifetime with the XAML page (nor can it, it needs to be created first to create the XAML page.

Correcting this leak is going to require some careful thought about how the aggregate object is assembled.

@AdamBraden AdamBraden modified the milestones: NET5 GA, Release 1.1 Oct 20, 2020
@Scottj1s
Copy link
Member

@Scottj1s
Copy link
Member

@AaronRobinsonMSFT
Copy link
Member

Servicing PR for .NET 5 - dotnet/runtime#45622.

@huoyaoyuan
Copy link
Contributor

I'm running with .NET 5.0.2 with WinRT.Runtime built from master(copied into appx folder), and still getting memory leak.

The process memory grows about 4GB. Visual Studio diagnostic tool spends an hour to get ~75M roots and hadn't completed the snapshot.

Do you think this is another memory leak issue?

@huoyaoyuan
Copy link
Contributor

Well, there may be some issue with Visual Studio debugger. Sometimes it can only find KBs of managed heap when I know there should be some MB byte arrays alive. Sometimes it finds excessive amount of roots.

@angelazhangmsft angelazhangmsft added this to the Release 1.1.5 milestone Mar 17, 2021
@AdamBraden
Copy link
Contributor

fyi - we are and have been investigating the root cause for a while now. Its proving to be a hard to pin down challenge due to the reference tracking of strong and weak references between cswinrt, xaml and .net. This is impacting WinUI and Reunion so it is being treated with utmost urgency.

@manodasanW
Copy link
Member

The leaks related to the WinUI scenarios have been addressed in CsWinRT 1.2.0 (affects both generated projection and runtime) and .NET runtime 5.0.2. Once WinUI and Project Reunion builds and ships their C# projections using CsWinRT 1.2.0 which they will do in an upcoming update, the leaks should disappear.

@dotMorten
Copy link
Contributor

@manodasanW WOOHOO

@llongley
Copy link
Member Author

llongley commented Apr 2, 2021

Thanks so much for getting this done! We'll be sure to pull this into WinUI 3 and Project Reunion.

@Noemata
Copy link

Noemata commented Apr 6, 2021

If there are no objections, could we please use the following tool to evaluate WinUI/UWP performance and memory utilization issues associated with Navigation Frame and its interaction with other controls: https://github.com/Noemata/RosettaNavigation

The tool supports testing NavigationCacheMode = NavigationCacheMode.Enabled and NavigationCacheMode = NavigationCacheMode.Disabled scenarios.

@angelazhangmsft angelazhangmsft added the fixed Issue has been fixed in an upcoming or existing release label May 13, 2021
@marcpiechura
Copy link

marcpiechura commented Jan 19, 2022

Hi,

I'm developing a new windows desktop application with WinUI 3 1.0 and .Net 6.0 and it looks like I'm hitting the bug that is described here. When I navigate between my pages, the memory keeps growing. I was also able to reproduce it with the test app from the first comment, the memory keeps growing until the app crashes eventually.
I would like to think it's an issue on my end, but I'm running out of options and once I implement the most basic functionality, it starts leaking memory. I can't believe I'm the only one facing this issue, so any help is much appreciated.

Exception thrown at 0x751FEC52 (KernelBase.dll) in App1.exe: WinRT originate error - 0x8007000E : 'Not enough memory resources are available to complete this operation.'

devenv_PTrfuUOEpf.mp4

@curia-damiano
Copy link

Hi @marcpiechura, can you please share the source codes of this POC?

@marcpiechura
Copy link

Hi @curia-damiano,

sure thing, find attached two demo projects, the "UIOnly" folder contains the example from this issue comment, ported to the latest project template.
Additionally, the "MVVM" folder contains an example that is similar to what I try to build in my application. I had to click back and force for a long time and some memory gets freed sometimes, but the app will crash eventually.
image

Hope that helps and thanks for looking into it.

PoC.zip

@marwalsch
Copy link

@marcpiechura You are not alone. I witnessed similar behaviour working with WinUI 3.

The images show how each page instantiation allocates memory while idle instances' memory is not released timely and might pile up to the point of exhaustion.

Screenshot 2022-01-24 145620

This is what it looks like when invoking the GC explicitly when a page is left.

Screenshot 2022-01-24 145855

@bogdan-patraucean
Copy link

bogdan-patraucean commented Feb 20, 2022

@marcpiechura, @marwalsch have the same problem with .NET 6 and Windows App SDK 1.0. The memory is not released after I leave a page, and when I come back it's increasing with the amount needed for the page. If the memory needed for that page is 50 MBs, each time I navigate back and forth, it keeps increasing with 50 MBs. The page constructor is never hit.

  • Tried NavigationCacheMode=Disabled and IsNagivationStackEnabled=False, same problem
  • Tried to implement IDisposable, but not even that is hit when I leave the page, same problem
  • Tried Bindinds.StopTracking() on Page Unload event, same problem
  • Tried GC.Collect() on Page Unload event, same problem

@TheAvonian
Copy link

TheAvonian commented Jun 29, 2022

This bug doesn't seem to be resolved, I'm using WinUI 3 on SDK 1.1.1, using the navigation template supplied by the official extension provides the same memory leak issue adding a MB every new page load.
I did the poor workaround of calling GC Collect every navigation call which did seem to suppress the issue but it brings about a different issue of possible crashing if clicking between pages too fast.

This issue is also prevalent on the official WinUI 3 Gallery app.

Edit: Also using .NET 6 newest SDK

@manodasanW
Copy link
Member

@TheAvonian I created #1213 to further investigate this remaining leak. If you happen to have a repro that we can use to investigate, can you post it there. Thanks.

@marwalsch
Copy link

marwalsch commented Jul 1, 2022

@manodasanW Just to be sure, is the Windows App SDK 1.0.0 guaranteed to ship with CWinRT 1.2.0 or newer, or does this dependency lie elsewhere?

My previous observations seem inconsistent and not reliably reproducible across different scenarios, i.e. invoking the GC has little to no effect.
Can confirm bogdan-patraucean's observations .

@manodasanW
Copy link
Member

@marwalsch It is unclear at the moment where the issue is, whether it is in CsWinRT, WinAppSDK / WinUI, or .NET. I will know more as we investigate this issue and after which is when I will be able to comment on shipping vehicles.

@huoyaoyuan
Copy link
Contributor

Just to be sure, is the Windows App SDK 1.0.0 guaranteed to ship with CWinRT 1.2.0 or newer, or does this dependency lie elsewhere?

WinAppSDK is detecting CsWinRT version, and fails the compilation on older version.

@KWodarczyk
Copy link

KWodarczyk commented Oct 24, 2022

what is the update o this ? i still see this issue
wappsdk 1.2.220930.4-preview2
winui 3
windows sdk build tools 10.0.25227-preview

@Omenrevlis
Copy link

Omenrevlis commented Nov 8, 2023

Looks like this bug is still present in Windows SDK 10.1 using Winui 3 and the Template Studio.

Something wacky this way comes…

I’m having the same memory leak issue. First, let me describe the problem. I created a Navigation based WinUI 3 application via the Templates. With the wizard, I added the main, another blank page and a few other pages that don’t matter for this discussion.
The wizard created a full MVVM setup. And the important parts about it is, there is a ShellPage.xaml view that holds the navigation stuff and one of the blank views I put on a Mapsui map with very a simple implementation.
When I run it and navigate away from the page with the map, it keeps the mapPage class in memory. I test this by going into the map page and navigating to another page and back 14 times. Then I take a snapshot and look for the class of the mapPage. To my surprise, I found it has 14 instances in memory. The other blank page I was navigating to and from in the test has 1 instance in memory.

Of course, my first thought is it is a disposing issue. So, I implement IDisopsable and in the Dispose method calls dispose on the map control.

Then I went into the shellpage Xaml and found the Frame “NavigationFrame” and added a method for the Navigating event. In this method, I check the content to see if it is a Page and if it is IDisposable. If it is, I call the Dispose method.

private void NavigationFrame_Navigating(object sender, Microsoft.UI.Xaml.Navigation.NavigatingCancelEventArgs e)
{
    if (NavigationFrame.Content is Page page && page is IDisposable disposablePage)
    {
        disposablePage.Dispose();
    }
}

Straight forward stuff. And it calls dispose on the Mappage as expected.

First problem is when I got to the point of calling dispose on the mapsui control it throws an exception saying I cannot access a disposed item… Interesting.

So, I wrapped the call to dispose in a try, and ate the exception. Now the wacky part. I do the same test, leaving the map page 14 times and take a snapshot and... There are 7 mapPages objects in memory…

That was unexpected.

So, I put junk code after the call to dispose and in the error block like this:

public void Dispose()
{
    try
    {
        MyMap.Dispose();
        var x = “it disposed”;
    }
    catch (System.Exception ex) 
    {
        var b = “broken!” ;
    }
}

Then I put break points on the var x. I ran the same test and all 14 times and never got to the breakpoint. But there were still only 7 instances held in memory.
Then I moved my breakpoint to the var b and ran the tests. 14 times I had to hit continue. But when I took a snapshot there was… 1 map page in memory. Even though it was never able to successfully call dispose on the mapsui control.

While my approach seems to be a failure, it leads me to believe that there is a race condition. Perhaps it’s where the Frame kills the current object before loading the new page?

For my solution, I’m considering rewriting how the NavigationViewItems call to the navigation helper. Perhaps pointing them instead to a method that I can wait for the page to fully dispose.

@dotMorten
Copy link
Contributor

dotMorten commented Nov 9, 2023

@Omenrevlis Are you expecting pages that implements IDisposable that something automatically will call Dispose on them? (You want to look at the finalizers instead though)

Also garbage collection is lazy. Its not uncommon that objects gets left around for a while - especially if memory pressure is low

@Omenrevlis
Copy link

Omenrevlis commented Nov 9, 2023

finalizers

For your first question, I have multiple answers. Yes, I would expect a solution built from the template studio where you define the pages you want in the wizard and it sets up a full MVVM solution with all the navigation wiring setup for you, to take care of appropriately releasing managed resources. And it seems to do so for not overly complicated or memory intensive pages.

No, I didn't expect something to magically call the Dispose method I created. When implemented IDisposable in the MapPage, I then went to the ShellPage (built by the Template Studio) and found the Frame that loads the page. On that frame, I registered for the Navigating event. This happens when a request to navigate is sent. That's where I make sure the content is a Page and Implements IDisposable and, if so, I call the Page Dispose method.

I would not have commented if it was just a matter of waiting for the garbage collector. The instances I mentioned stick around. I left my computer for over an hour and came back and took a snapshot and they were still there and the memory was still be used.

Implementing a finalizer wouldn't help. Best practice is to use dispose when you can and finalizers as a last ditch effort incase someone forgot to call dispose and is used typically for unmanaged resources. Also best practice is to call dispose with your finalizer. In this case, it wouldn't matter, because .net thinks the control is already disposed. (thus proving that the navigation actually tried) While I could unwire from the Navigating event for the Frame on the ShellPage and just use a finalizer to call the dispose, it should have the same effect. It stinks as a pattern, but I could. For giggles, and because it only takes two seconds, I tried. Did the same test and 14 copies of the class are still in memory and the map control was already disposed when tried to dispose it.

Only other thing on the mappage is a TextBlock bound to a class property.

@dotMorten
Copy link
Contributor

dotMorten commented Nov 9, 2023

My point is you should count the number of finalizers being hit, since that's the number that matters - number of dispose calls doesn't.
And you really only need to implement dispose when releasing native or disposable resources you created.
It sounds to me like the controls you use on certain pages are the cause of your leaks. There are plenty of memory analyzer tools that can help you analyze your graph of what is holding on to what.

@Omenrevlis
Copy link

My point is you should count the number of finalizers being hit, since that's the number that matters - number of dispose calls doesn't. And you really only need to implement dispose when releasing native or disposable resources you created. It sounds to me like the controls you use on certain pages are the cause of your leaks. There are plenty of memory analyzer tools that can help you analyze your graph of what is holding on to what.

I checked how many times the finalizer was hit and it aligns with the 50% of mappages being released from memory even though my dispose of the object still says for all of them that the control was already disposed. For the remaining in memory there is the full stack of objects that a Mapsui map has. Nothing surprising since they were not disposed.

Tonight I'll test the disposing of Mapsui maps to see if its the dispose of that control causing the leak.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixed Issue has been fixed in an upcoming or existing release gc Related to garbage collection pri-0 Blocking/issue has no workaround
Projects
None yet
Development

Successfully merging a pull request may close this issue.