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

Memory leak when use x:Bind in winUI3 #7282

Open
1 of 2 tasks
khoabui1412 opened this issue Jun 27, 2022 · 32 comments
Open
1 of 2 tasks

Memory leak when use x:Bind in winUI3 #7282

khoabui1412 opened this issue Jun 27, 2022 · 32 comments
Labels
area-Binding bug Something isn't working product-winui3 WinUI 3 issues team-Markup Issue for the Markup team

Comments

@khoabui1412
Copy link

Describe the bug

Hi,
BlankPage.Xaml code:

<Window>
    <Grid>
        <Button Click="{x:Bind click_event}" />
    </Grid>
</Window

BlankPage.xaml.cs code:

private void click_event(object sender, RoutedEventArgs e)
    {
        int a = 1;
    }
    ~BlankPage1()
    {
        int a = 1;
    }

From main window, I open BlankPage window and close it:

    private  ButtonClick()
            {
                var aa = new BlankPage1();
                aa.Activate();
                aa.Close();
                aa = null;
                GC.Collect();
            }

However, BlankPage object doesn't released (doesn't run into ~BlankPage function). And if you continue click to open and close the BlankPage, memory keeps increasing.

Otherwise, if I don't use x:Bind in BlankPage.xaml:
<Button Click="click_event" />

BlankPage will be released as normal and no memory leak happened.

So, is it a problem on x:Bind? Also, I tried with Bindings.StopTracking(); but doesn't work.

Also asked at: https://docs.microsoft.com/en-us/answers/questions/898874/memory-leak-when-close-window-in-winui3.html
Thanks

Steps to reproduce the bug

Click to open and close BlankPage window and resource of BlankPage's view is not released.

Expected behavior

No response

Screenshots

No response

NuGet package version

WinUI 3 - Windows App SDK 1.1.1

Windows app type

  • UWP
  • Win32

Device form factor

No response

Windows version

No response

Additional context

No response

@khoabui1412 khoabui1412 added the bug Something isn't working label Jun 27, 2022
@ghost ghost added the needs-triage Issue needs to be triaged by the area owners label Jun 27, 2022
@krschau
Copy link
Contributor

krschau commented Jun 27, 2022

Possible duplicate of #3303
@fabiant3 FYI

@krschau krschau added area-Binding team-Markup Issue for the Markup team labels Jun 27, 2022
@khoabui1412
Copy link
Author

Possible duplicate of #3303 @fabiant3 FYI

Calling Bindings.StopTracking doesn't help, it just helps release viewmodel but the view is not.

@bpulliam bpulliam added the product-winui3 WinUI 3 issues label Oct 12, 2022
@khoabui1412
Copy link
Author

khoabui1412 commented Nov 20, 2022

Hi, thing get worse, when I update to WASDK ver 1.2, even not using x:Bind, view did not release.

@bpulliam bpulliam removed the needs-triage Issue needs to be triaged by the area owners label Dec 6, 2022
@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.

@HO-COOH
Copy link

HO-COOH commented Aug 3, 2023

I just want to help to make this not get closed

@akainth015
Copy link

akainth015 commented Sep 13, 2023

This issue is still a problem in Windows App SDK 1.4. I have a background application that will open and close many windows during its lifecycle.

I'm currently working around this issue by restarting the app every time the windows are closed; this is not a long-term solution. It also results an unpleasant user experience because a window pops in and out of existence.

Here is a repro project. It is the Windows App SDK C# template, on top of which I have

  • Removed x:Name
  • Added a button to open and Activate() a new MainWindow
  • Added a button to call Close() on the enclosing MainWindow

Steps to reproduce this behavior are to

  1. Build the Packaging project (both Debug and Release feature the issue)
  2. Run the program with the debugger attached, to view memory usage information in VS
    1. Alternatively, you can use Task Manager to track memory usage
  3. Open one or more new windows
    1. Observe that the memory usage increases as each new window is opened
  4. Close several windows
    1. Observe that the memory associated with the window is not freed

@HO-COOH
Copy link

HO-COOH commented Sep 13, 2023

I can imagine this would soon get solved by Windows explorer team pushing winui team hard. Explorer.exe leaking memory would be unforgivable.

@NicholasChrzan
Copy link

The only way I've found to have things clean up properly is to explicitly unwire all event handlers (including ones specified in the XAML) while either the appwindow is destroying or on the unloaded event for the controls. In those you also need to unwire the destroying or unloaded event as well respectively.

@steam3d
Copy link

steam3d commented Oct 5, 2023

@NicholasChrzan could you provide sample code?

@NicholasChrzan
Copy link

@steam3d If you look at @akainth015 post above mine he has a link to a simple project which illustrates the memory leak. If you profile it with a memory profiler you can see that the resources are not being garbage collected.

To have it be collected on close you just need to unwire the events.

@steam3d
Copy link

steam3d commented Oct 6, 2023

@NicholasChrzan I try tu figure out how to unsubscribe from x:bind from xaml code

@NicholasChrzan
Copy link

NicholasChrzan commented Oct 10, 2023

@steam3d If you care about it being released don't use x:Bind just use the older Binding nomenclature and it will clean up.

@Prochy
Copy link

Prochy commented Jun 4, 2024

Sorry guys, but can someone comment on this? This is absolutely terrible. There is obviously memory leak with using x:Bind for years and in the documentation there is no mention about this issue. I just discovered now and the only solution is to replace all x:Bind by Binding, Bindings.StopTracking() solve nothing in my case. And everywhere in WinUI Documentation is mainly mention x:Bind.

Is there any possibility it will be solved anytime soon? (Mean in way, like 1-3 months, no years). This is the best example how the communication with the public SHOULD NOT look.

@MikeHillberg
Copy link
Contributor

@khoabui1412, in that repro it's not necessary to use a binding, you can just do Click="click_event", but that's not to say that it should leak. But in the @akainth015 it's not using a binding at all. Just curious if this is an x:Bind leak or a window leak.

@NicholasChrzan
Copy link

One thing I've found that if you follow the WinRT/UWP approach of a single window and frame on navigating around (while flushing history) the UI elements clean up as expect. It's only on the window close where this cleanup doesn't occur (I haven't tried forcibly closing the window via WM_CLOSE), but based on memory profilers if you avoid x:Bind and unwire ALL UI event handlers on unload or appwindow destroy then they are properly garbage collected.

@steam3d
Copy link

steam3d commented Jun 4, 2024

This issue will simply increase the number of applications with memory leaks.

@Prochy
Copy link

Prochy commented Jun 13, 2024

@khoabui1412, in that repro it's not necessary to use a binding, you can just do Click="click_event", but that's not to say that it should leak. But in the @akainth015 it's not using a binding at all. Just curious if this is an x:Bind leak or a window leak.

Not sure if I understand you, but in my case event handlers defined directly in XAML don’t cause memory leak. I need to only deattach the ones defined in code behind. Once I replaced all x:Bind by Binding, I’m not facing anymore the memory leak with events defined in the XAML. I’m using multi window application where I open several new windows per day and there can be the memory leak pretty high if I use in the window images and so on.

if you’re not able to easily fix it. This should be mention in the documentation when using x:Bind. It took me a few hours to discover there is a bug in x:Bind.

@lfmouradasilva
Copy link

I have the same problem in my project that has already been launched in production. Does anyone have a workaround?

@steam3d
Copy link

steam3d commented Jun 23, 2024

@lfmouradasilva Replace all x:bind as described above.

@BendicoE
Copy link

Having to replace x:Bind'ings with Binding's in a WinUI 3 application is simply unacceptable. Our multi-window application is already in production and we have 100+ x:Bind'ings in there, and a complex hierarchy. We can encourage users to use only a single window to edit documents and close them when done, thereby "reusing" the single main window. But this defeats the very purpose of the multi-window design, and is also not an acceptable workaround. I see this bug has been present in the Windows App SDK for two years now, and no solution seems to be in sight so far. Are we facing a dead end for our WinUI 3 product?

@khoabui1412
Copy link
Author

I tested with binding and it’s very slow compared to x:Bind

@Prochy
Copy link

Prochy commented Aug 20, 2024

The lack of communication drives me crazy and for me is something unacceptable. No idea if we can expect someone takes a look one day or not, why it is not at least mentioned in the documentation with bold font as this is crucial information!

@BendicoE
Copy link

So, we have realized now, that the only way that we can continue development and sales of our WinUI 3 application is to nail this problem ourselves, possibly with help from the community.

I'd like to share some of my findings so far: I have created a minimal WinUI 3 application that demonstrates the problem:

https://github.com/BendicoE/WinUI3MultiWindowApp

Basically, it opens up 10 sub windows when clicking on the MainWindow button. The SubWindow is using a very simple MVVM design for testing purposes. However, it turns out that x:Bind - at least in this case - isn't the core of the problem. My SubWindow also allocates a 10M array of int's directly and initializes it. Note that this object is not a part of the model or any x:Bind'ings. This results in a memory consumption of approx 40MB per window, which can be easily seen using the profiler in VS. Now here is the interesting part: The array object will remain allocated and not garbage collected if you close the window without any remedies. However, by hooking on to the Closed event of SubWindow, setting the member to null in it, the memory will eventually be released. I have tried to make this a little more evident by hooking on another Closed handler from the "outside" in MainWindow.xaml.cs. In this handler; I force garbage collection. A very puzzling thing when testing on my system, is that I have to close 5-6 SubWindow's before the memory is released. But after all of them have been closed, you end up with approx the initial memory cosumption level of the app.

My conclusion is that the problem lies in the Window class in the Windows App SDK. And, remember, starting with WinUI 3, this class is no longer a FrameworkElement, it doesn't have a XamlRoot, and some things are now trickier to do. Somehow - at least that's my theory now - member objects of Window derived classes are not automatically dereferenced when closing the window, and the GC is unable to collect them. Unless you explicitly dereference it in the Close event. And since x:Bind generates code with explicit object references, these will indirectly cause the same problems. Hence people have been led to believe that x:Bind is the problem.

Encouraged by these findings, we are now working hard to explicitly dereference all objects referenced by our Window class. It's not an easy job, but hopefully, we can continue developing and market our product.

Feel free to comment!

@BendicoE
Copy link

As I said, it's my current theory. I'm working on our huge binding hierarchy now. And I also just thought of something else: How about Mode=OneTime x:Bind's? That's the default. Setting these to null will presumably have no effect.

@garrettpauls
Copy link

@BendicoE I've been fighting a similar issue with page navigation (see #934). It was pointed out that we apparently also need to set ItemsSource to null on various items controls to get them to release native memory, so I wrote a helper class to do that on page navigation which significantly improve our app's memory consumption.

I'd also recommend profiling with a 3rd party profiler such as dotMemory which imo shows native memory consumption better than the VS profiler, which can help determine whether the root of the issue is somewhere in WinRT or not. (All the major memory leaks I've found in our app so far are in native memory, which certainly point to WinRT or the C# bridge to it not cleaning things up effectively.)

@BendicoE
Copy link

I have updated my test app

https://github.com/BendicoE/WinUI3MultiWindowApp

I have moved the big allocation into the model class, and if you take a look, I am now positive that setting Model = null prevents the leak in this case. No need for Bindings.StopTracking() or GC.Collect(), just line 46 in SubWindow.xaml.cs does the job.

Feel free to comment further!

@khoabui1412
Copy link
Author

khoabui1412 commented Aug 23, 2024

Yes, the case at here is the view is leaked not the viewmodel, since view keeps reference the viewmodel, assign viewmodel to null will help release the viewmodel memory.
I think the question is, is there other element in view needed assign to null ? as @garrettpauls pointed out that we have to set itemsource to null.
Further, in your previous example, set array to null in the view may be just help release that array object mem but the view is still leak, because the simple xaml so the leak is small and you don’t mind to it, in the complex xaml, it will be more clear?

@lhak
Copy link

lhak commented Aug 24, 2024

I have looked at the test app by @BendicoE. Using the Visual Studio memory snapshot function, it is quite easy to see which references are still active that keep the window, and, thus, the model object alive. I have found two issues, both related to event handlers added to the window object. These prevent the model from being freed.

Event handlers in the XAML file of the window

Adding the Closed (or other) event handler to the SubWindow.xaml file prevents deallocation to the window. Registering and deregistering them in code works fine.

Using x:Bind in the XAML file of the window

Adding any x:Bind statements to the xaml file causes the generation of code that subscribes to the Activated event handler of the window. Probably related to the first issue, this causes a circular reference which apparently cannot be resolved by the xaml framework.

I created a separate SubWindowPage, added it to the SubWindow and moved all xaml and c# code to this page (and removed the window close handler). I can confirm that with these changes the model is correctly being freed and memory usage drops. I also tested adding event handlers in the page xaml/c# code and using x:Bind statements with updates enabled. These scenarios seem to be working well. Thus, I conclude that there are serious issues regarding the behavior of the window object that need to be fixed. I guess currently, the best way is to always to create separate page and move all xaml/c# code there.

@BendicoE
Copy link

Thanks Ihak for your thorough investigation! I have updated my app and added code similar to what you have done.

An interesting thing, I think, is that Window doesn't have a DataContext, but Page has. Another, very interesting thing is that during performance profiling of the memory, in the snapshots, the int arrays show up normally when the Model is allocated from the Page. However, when allocated from the Window, they show up as "dead objects". So are they "lost" from the sight of the GC? Clearly, there must be something missing in the Window class would ensure memory was handled correctly by the framework. Hence, closing and destroying a Window will always cause leaks of any object referenced from it, including x:Binds to the model.

For us, this is a huge problem, because we rely heavily on the Window, lot of customizations in the title bar, several Views, etc. I cannot see that we can eliminate the object references from the Window without redesigning the whole app.

Anyone else care to comment further on this?

@BendicoE
Copy link

Just a quick update: I have now added a third sub window class, where the XAML and bindings are contained in a UserControl. The result is the same as with Page, the objects are released. For us, this is good news. It means we can try to eliminate x:Bind's just from the Window class, using binding in code there, and leave the other parts of the UI that are based on UserControl's as is.

@BendicoE
Copy link

We decided to do a major refactoring of our WinUI 3 code. Basically, our MainWindow now has a single XAML element: A UserControl (we have called it MainView) with basically identical XAML to the old MainWindow. And the only challenge (not a big one), was to handle the custom title bar. We ended up with some code in both classes, but it was quite straightforward. Our application is now performing significantly better with opening and closing multiple windows. But we will continue working on analysing the memory usage, there could be more snags in the WinUI 3 framework.

Lesson learned: In a WinUI 3 app, don't assume the GC will always clean up when necessary, and "automagically" keep your memory usage tidy and efficient.

@lhak
Copy link

lhak commented Aug 30, 2024

As far as I know, there is a specific mechanism to deal with circular references (e.g. event handlers) involving managed and native objects in XAML. However, it looks that at least some of it is implemented in the DependencyObject code, and as Window is not derived from it, this might not work here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Binding bug Something isn't working product-winui3 WinUI 3 issues team-Markup Issue for the Markup team
Projects
None yet
Development

No branches or pull requests