microsoft / microsoft-ui-xaml

Windows UI Library: the latest Windows 10 native controls and Fluent styles for your applications
MIT License
6.28k stars 675 forks source link

Memory leak when use x:Bind in winUI3 #7282

Open khoabui1412 opened 2 years ago

khoabui1412 commented 2 years ago

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

Device form factor

No response

Windows version

No response

Additional context

No response

krschau commented 2 years ago

Possible duplicate of https://github.com/microsoft/microsoft-ui-xaml/issues/3303 @fabiant3 FYI

khoabui1412 commented 2 years ago

Possible duplicate of #3303 @fabiant3 FYI

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

khoabui1412 commented 1 year ago

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

github-actions[bot] commented 1 year ago

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 commented 1 year ago

I just want to help to make this not get closed

akainth015 commented 1 year ago

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

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 commented 1 year ago

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

NicholasChrzan commented 1 year ago

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 commented 11 months ago

@NicholasChrzan could you provide sample code?

NicholasChrzan commented 11 months ago

@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 commented 11 months ago

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

NicholasChrzan commented 11 months ago

@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 commented 3 months ago

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 commented 3 months ago

@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 commented 3 months ago

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 commented 3 months ago

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

Prochy commented 3 months ago

@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 commented 2 months ago

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

steam3d commented 2 months ago

@lfmouradasilva Replace all x:bind as described above.

BendicoE commented 4 weeks ago

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 commented 4 weeks ago

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

Prochy commented 4 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

@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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 2 weeks ago

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.