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.

Sign up for GitHub

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

Jump to bottom

Multi-window memory leak #10029

Closed
vfabulokwe opened this issue Sep 9, 2022 · 4 comments · Fixed by #13400
Closed

Multi-window memory leak #10029

vfabulokwe opened this issue Sep 9, 2022 · 4 comments · Fixed by #13400
Assignees
@jonathanpeppers
Labels
fixed-in-8.0.0-preview.3.8149 Look for this fix in 8.0.0-preview.3.8149! legacy-area-perf Startup / Runtime performance memory-leak 💦 Memory usage grows / objects live forever p/1 Work that is important, and has been scheduled for release in this or an upcoming sprint partner/cat 😻 this is an issue that impacts one of our partners or a customer our advisory team is engaged with platform/windows 🪟 t/bug Something isn't working t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.)
Milestone
.NET 8 Planning

Comments

@vfabulokwe
Copy link

vfabulokwe commented Sep 9, 2022 •
edited by Eilon
Loading

Description

Closing a window results in a memory leak. The windows are never GC’d and often all their content is still visible within memory. Comparing this behavior with WinUI3 I have found that it works correctly in WinUI3, windows are created and recycled, counts go up, and back down. On MAUI the same process results in a steady increase in Window counts.

Steps to Reproduce

  1. Create new MAUI app
  2. Add a button in the MainPage.xaml and implement the Clicked event handler in the code behind.
// button - to open a new window
private void BlankWinLeakBtn_Clicked(object sender, EventArgs e)
{
var page = new ContentPage()
{
Content = new Label()
{
Text = ""
}
};
var win = new Window()
{
Page = page
};

    App.Current.OpenWindow(win);
}
  1. Click on the button
  2. Take snapshot
  3. Close window
  4. Click button again
  5. Take snapshot
  6. Close window
  7. Repeat steps 1-6 multiple times
  8. Compare snapshots

Expected Result:
Ref count should go down when a window is closed.

Actual Result:
Ref count keeps going up, window is never GC'd.

Link to public reproduction project repository

NA

Version with bug

Unknown/Other (please specify)

Last version that worked well

Unknown/Other

Affected platforms

Windows

Affected platform versions

All

Did you find any workaround?

No response

Relevant log output

No response
@vfabulokwe vfabulokwe added the t/bug Something isn't working label Sep 9, 2022
@jsuarezruiz jsuarezruiz added the legacy-area-perf Startup / Runtime performance label Sep 12, 2022
@PureWeen PureWeen added this to the Backlog milestone Sep 13, 2022
@ghost
Copy link

ghost commented Sep 13, 2022

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@samhouts samhouts added partner/cat 😻 this is an issue that impacts one of our partners or a customer our advisory team is engaged with p/1 Work that is important, and has been scheduled for release in this or an upcoming sprint labels Sep 20, 2022
@jsuarezruiz
Copy link
Contributor

Related with microsoft/microsoft-ui-xaml#7617

@jsuarezruiz jsuarezruiz mentioned this issue Oct 25, 2022
Closed
@mouralabank
Copy link

The same problem here, any workaround?

@danielancines
Copy link

I've been making some tests here, and the problem appears to be in a Window object that still forever allocated, in my tests, even I closed New Window and check if it removed from App.Current.Windows collection, the object never will be collect by GC and I cannot find who is locking up on the object tree. Unfortunately I didn't find any workaround for this issue, do you guys have any idea?

@samhouts samhouts modified the milestones: Backlog, .NET 8 Planning Jan 26, 2023
@jonathanpeppers jonathanpeppers self-assigned this Feb 16, 2023
jonathanpeppers added a commit to jonathanpeppers/maui that referenced this issue Feb 16, 2023
@jonathanpeppers
[controls] fix memory leak in Application.OpenWindow()
3e9048e 
Context: dotnet#10029

I could reproduce a memory leak by doing:

    App.Current.OpenWindow(new Window { Page = new ContentPage() });

I found that `App.Current._requestedWindows` just held onto every
`Window` object forever.

This is a perfect example of where using `ConditionalWeakTable` solves
the issue:

https://learn.microsoft.com/dotnet/api/system.runtime.compilerservices.conditionalweaktable-2

The only other change I made was to use `Guid.ToString("n")` as it
removes `{`, `}`, and `-` characters from the string.

Note this doesn't fully solve dotnet#10029, as I still see something else
holding onto `Window` in my example.

This is WIP, as I'm interested in what my `WindowsDoNotLeak` test will
do on all platforms.
@jonathanpeppers jonathanpeppers mentioned this issue Feb 16, 2023
Merged
jonathanpeppers added a commit to jonathanpeppers/maui that referenced this issue Feb 21, 2023
@jonathanpeppers
[controls] fix memory leak in Application.OpenWindow()
6d1d60a 
Context: dotnet#10029

I could reproduce a memory leak by doing:

    App.Current.OpenWindow(new Window { Page = new ContentPage() });

I found that `App.Current._requestedWindows` just held onto every
`Window` object forever.

This is a perfect example of where using `ConditionalWeakTable` solves
the issue:

https://learn.microsoft.com/dotnet/api/system.runtime.compilerservices.conditionalweaktable-2

The only other change I made was to use `Guid.ToString("n")` as it
removes `{`, `}`, and `-` characters from the string.

Note this doesn't fully solve dotnet#10029, as I still see something else
holding onto `Window` in my example.

This is WIP, as I'm interested in what my `WindowsDoNotLeak` test will
do on all platforms.
jonathanpeppers added a commit to jonathanpeppers/maui that referenced this issue Feb 22, 2023
@jonathanpeppers
[controls] fix memory leak in Application.OpenWindow()
eb1867c 
Context: dotnet#10029

I could reproduce a memory leak by doing:

    App.Current.OpenWindow(new Window { Page = new ContentPage() });

I found that `App.Current._requestedWindows` just held onto every
`Window` object forever.

This is a perfect example of where using `ConditionalWeakTable` solves
the issue:

https://learn.microsoft.com/dotnet/api/system.runtime.compilerservices.conditionalweaktable-2

The only other change I made was to use `Guid.ToString("n")` as it
removes `{`, `}`, and `-` characters from the string.

Note this doesn't fully solve dotnet#10029, as I still see something else
holding onto `Window` in my example.

This is WIP, as I'm interested in what my `WindowsDoNotLeak` test will
do on all platforms.
jonathanpeppers added a commit to jonathanpeppers/maui that referenced this issue Feb 23, 2023
@jonathanpeppers
[controls] fix memory leak in Window
3e7e522 
Fixes: dotnet#10029

I could reproduce a memory leak by doing:

    App.Current.OpenWindow(new Window { Page = new ContentPage() });

I found that `App.Current._requestedWindows` just held onto every
`Window` object forever.

This is a perfect example of where using `ConditionalWeakTable` solves
the issue:

https://learn.microsoft.com/dotnet/api/system.runtime.compilerservices.conditionalweaktable-2

The only other change I made was to use `Guid.ToString("n")` as it
removes `{`, `}`, and `-` characters from the string.

After these changes, I still found `Window` objects hanging around
that appeared to be held onto by many other objects.

Then I reviewed:

    void IWindow.Destroying()
    {
        SendWindowDisppearing();
        Destroying?.Invoke(this, EventArgs.Empty);
        OnDestroying();

        Application?.RemoveWindow(this);
        // This wasn't here!
        Handler?.DisconnectHandler();
    }

It appears that upon `Window`'s closing, we didn't have any code that
"disconnected" the MAUI handler. After this change, I could see
`Window` objects properly go away.

I added a unit test as well.
jonathanpeppers added a commit to jonathanpeppers/maui that referenced this issue Mar 1, 2023
@jonathanpeppers
[controls] fix memory leak in Window
e9e8def 
Fixes: dotnet#10029

I could reproduce a memory leak by doing:

    App.Current.OpenWindow(new Window { Page = new ContentPage() });

I found that `App.Current._requestedWindows` just held onto every
`Window` object forever.

This is a perfect example of where using `ConditionalWeakTable` solves
the issue:

https://learn.microsoft.com/dotnet/api/system.runtime.compilerservices.conditionalweaktable-2

The only other change I made was to use `Guid.ToString("n")` as it
removes `{`, `}`, and `-` characters from the string.

After these changes, I still found `Window` objects hanging around
that appeared to be held onto by many other objects.

Then I reviewed:

    void IWindow.Destroying()
    {
        SendWindowDisppearing();
        Destroying?.Invoke(this, EventArgs.Empty);
        OnDestroying();

        Application?.RemoveWindow(this);
        // This wasn't here!
        Handler?.DisconnectHandler();
    }

It appears that upon `Window`'s closing, we didn't have any code that
"disconnected" the MAUI handler. After this change, I could see
`Window` objects properly go away.

I added a unit test as well.
jonathanpeppers added a commit to jonathanpeppers/maui that referenced this issue Mar 1, 2023
@jonathanpeppers
[controls] fix memory leak in Window
ef2579a 
Fixes: dotnet#10029

I could reproduce a memory leak by doing:

    App.Current.OpenWindow(new Window { Page = new ContentPage() });

I found that `App.Current._requestedWindows` just held onto every
`Window` object forever.

This is a perfect example of where using `ConditionalWeakTable` solves
the issue:

https://learn.microsoft.com/dotnet/api/system.runtime.compilerservices.conditionalweaktable-2

The only other change I made was to use `Guid.ToString("n")` as it
removes `{`, `}`, and `-` characters from the string.

After these changes, I still found `Window` objects hanging around
that appeared to be held onto by many other objects.

Then I reviewed:

    void IWindow.Destroying()
    {
        SendWindowDisppearing();
        Destroying?.Invoke(this, EventArgs.Empty);
        OnDestroying();

        Application?.RemoveWindow(this);
        // This wasn't here!
        Handler?.DisconnectHandler();
    }

It appears that upon `Window`'s closing, we didn't have any code that
"disconnected" the MAUI handler. After this change, I could see
`Window` objects properly go away.

I added a unit test as well.
jonathanpeppers added a commit to jonathanpeppers/maui that referenced this issue Mar 1, 2023
@jonathanpeppers
[controls] fix memory leak in Window
2e7c3ff 
Fixes: dotnet#10029

I could reproduce a memory leak by doing:

    App.Current.OpenWindow(new Window { Page = new ContentPage() });

I found that `App.Current._requestedWindows` just held onto every
`Window` object forever.

Note that we *can't* use `ConditionalWeakTable` here:

https://learn.microsoft.com/dotnet/api/system.runtime.compilerservices.conditionalweaktable-2

After initially trying this, I added a test showing not to use it. The
problem is if the `string` is GC'd the `Window` will be lost.

We can use `Dictionary<string, WeakReference<Window>>` instead.

The only other change I made was to use `Guid.ToString("n")` as it
removes `{`, `}`, and `-` characters from the string.

After these changes, I still found `Window` objects hanging around
that appeared to be held onto by many other objects.

Then I reviewed:

    void IWindow.Destroying()
    {
        SendWindowDisppearing();
        Destroying?.Invoke(this, EventArgs.Empty);
        OnDestroying();

        Application?.RemoveWindow(this);
        // This wasn't here!
        Handler?.DisconnectHandler();
    }

It appears that upon `Window`'s closing, we didn't have any code that
"disconnected" the MAUI handler. After this change, I could see
`Window` objects properly go away.

I added a unit test as well.
PureWeen added a commit that referenced this issue Mar 1, 2023
@PureWeen
[controls] fix memory leak in Window (#13400)
db7f68f 
Fixes: #10029

I could reproduce a memory leak by doing:

    App.Current.OpenWindow(new Window { Page = new ContentPage() });

I found that `App.Current._requestedWindows` just held onto every
`Window` object forever.

Note that we *can't* use `ConditionalWeakTable` here:


https://learn.microsoft.com/dotnet/api/system.runtime.compilerservices.conditionalweaktable-2

After initially trying this, I added a test showing not to use it. The
problem is if the `string` is GC'd the `Window` will be lost.

We can use `Dictionary<string, WeakReference<Window>>` instead.

The only other change I made was to use `Guid.ToString("n")` as it
removes `{`, `}`, and `-` characters from the string.

After these changes, I still found `Window` objects hanging around that
appeared to be held onto by many other objects.

Then I reviewed:

```csharp
void IWindow.Destroying()
{
    SendWindowDisppearing();
    Destroying?.Invoke(this, EventArgs.Empty);
    OnDestroying();

    Application?.RemoveWindow(this);
    // This wasn't here!
    Handler?.DisconnectHandler();
}
```

It appears that upon `Window`'s closing, we didn't have any code that
"disconnected" the MAUI handler. After this change, I could see `Window`
objects properly go away.

I added a unit test as well.
PureWeen added a commit that referenced this issue Mar 2, 2023
@PureWeen
[controls] fix memory leak in Window (#13400)
e6d3dbb 
Fixes: #10029

I could reproduce a memory leak by doing:

    App.Current.OpenWindow(new Window { Page = new ContentPage() });

I found that `App.Current._requestedWindows` just held onto every
`Window` object forever.

Note that we *can't* use `ConditionalWeakTable` here:

https://learn.microsoft.com/dotnet/api/system.runtime.compilerservices.conditionalweaktable-2

After initially trying this, I added a test showing not to use it. The
problem is if the `string` is GC'd the `Window` will be lost.

We can use `Dictionary<string, WeakReference<Window>>` instead.

The only other change I made was to use `Guid.ToString("n")` as it
removes `{`, `}`, and `-` characters from the string.

After these changes, I still found `Window` objects hanging around that
appeared to be held onto by many other objects.

Then I reviewed:

```csharp
void IWindow.Destroying()
{
    SendWindowDisppearing();
    Destroying?.Invoke(this, EventArgs.Empty);
    OnDestroying();

    Application?.RemoveWindow(this);
    // This wasn't here!
    Handler?.DisconnectHandler();
}
```

It appears that upon `Window`'s closing, we didn't have any code that
"disconnected" the MAUI handler. After this change, I could see `Window`
objects properly go away.

I added a unit test as well.
# Conflicts:
#	src/Controls/tests/Core.UnitTests/WindowsTests.cs
rmarinho pushed a commit that referenced this issue Mar 3, 2023
@PureWeen
[controls] fix memory leak in Window (#13400) (#13654)
5745e3d 
Fixes: #10029

I could reproduce a memory leak by doing:

    App.Current.OpenWindow(new Window { Page = new ContentPage() });

I found that `App.Current._requestedWindows` just held onto every
`Window` object forever.

Note that we *can't* use `ConditionalWeakTable` here:

https://learn.microsoft.com/dotnet/api/system.runtime.compilerservices.conditionalweaktable-2

After initially trying this, I added a test showing not to use it. The
problem is if the `string` is GC'd the `Window` will be lost.

We can use `Dictionary<string, WeakReference<Window>>` instead.

The only other change I made was to use `Guid.ToString("n")` as it
removes `{`, `}`, and `-` characters from the string.

After these changes, I still found `Window` objects hanging around that
appeared to be held onto by many other objects.

Then I reviewed:

```csharp
void IWindow.Destroying()
{
    SendWindowDisppearing();
    Destroying?.Invoke(this, EventArgs.Empty);
    OnDestroying();

    Application?.RemoveWindow(this);
    // This wasn't here!
    Handler?.DisconnectHandler();
}
```

It appears that upon `Window`'s closing, we didn't have any code that
"disconnected" the MAUI handler. After this change, I could see `Window`
objects properly go away.

I added a unit test as well.
# Conflicts:
#	src/Controls/tests/Core.UnitTests/WindowsTests.cs
github-actions bot pushed a commit that referenced this issue Mar 20, 2023
@PureWeen
[controls] fix memory leak in Window (#13400)
bb93302 
Fixes: #10029

I could reproduce a memory leak by doing:

    App.Current.OpenWindow(new Window { Page = new ContentPage() });

I found that `App.Current._requestedWindows` just held onto every
`Window` object forever.

Note that we *can't* use `ConditionalWeakTable` here:

https://learn.microsoft.com/dotnet/api/system.runtime.compilerservices.conditionalweaktable-2

After initially trying this, I added a test showing not to use it. The
problem is if the `string` is GC'd the `Window` will be lost.

We can use `Dictionary<string, WeakReference<Window>>` instead.

The only other change I made was to use `Guid.ToString("n")` as it
removes `{`, `}`, and `-` characters from the string.

After these changes, I still found `Window` objects hanging around that
appeared to be held onto by many other objects.

Then I reviewed:

```csharp
void IWindow.Destroying()
{
    SendWindowDisppearing();
    Destroying?.Invoke(this, EventArgs.Empty);
    OnDestroying();

    Application?.RemoveWindow(this);
    // This wasn't here!
    Handler?.DisconnectHandler();
}
```

It appears that upon `Window`'s closing, we didn't have any code that
"disconnected" the MAUI handler. After this change, I could see `Window`
objects properly go away.

I added a unit test as well.
# Conflicts:
#	src/Controls/tests/Core.UnitTests/WindowsTests.cs
rmarinho pushed a commit that referenced this issue Mar 21, 2023
@github-actions @PureWeen
[controls] fix memory leak in Window (#13400) (#14073)
a35aa8a 
Fixes: #10029

I could reproduce a memory leak by doing:

    App.Current.OpenWindow(new Window { Page = new ContentPage() });

I found that `App.Current._requestedWindows` just held onto every
`Window` object forever.

Note that we *can't* use `ConditionalWeakTable` here:

https://learn.microsoft.com/dotnet/api/system.runtime.compilerservices.conditionalweaktable-2

After initially trying this, I added a test showing not to use it. The
problem is if the `string` is GC'd the `Window` will be lost.

We can use `Dictionary<string, WeakReference<Window>>` instead.

The only other change I made was to use `Guid.ToString("n")` as it
removes `{`, `}`, and `-` characters from the string.

After these changes, I still found `Window` objects hanging around that
appeared to be held onto by many other objects.

Then I reviewed:

```csharp
void IWindow.Destroying()
{
    SendWindowDisppearing();
    Destroying?.Invoke(this, EventArgs.Empty);
    OnDestroying();

    Application?.RemoveWindow(this);
    // This wasn't here!
    Handler?.DisconnectHandler();
}
```

It appears that upon `Window`'s closing, we didn't have any code that
"disconnected" the MAUI handler. After this change, I could see `Window`
objects properly go away.

I added a unit test as well.
# Conflicts:
#	src/Controls/tests/Core.UnitTests/WindowsTests.cs

Co-authored-by: Shane Neuville <shneuvil@microsoft.com>
@ghost ghost locked as resolved and limited conversation to collaborators Apr 1, 2023
@samhouts samhouts added the fixed-in-8.0.0-preview.3.8149 Look for this fix in 8.0.0-preview.3.8149! label Apr 12, 2023
@Eilon Eilon added the t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) label May 10, 2024
@samhouts samhouts added the memory-leak 💦 Memory usage grows / objects live forever label Oct 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@jonathanpeppers jonathanpeppers

Labels
fixed-in-8.0.0-preview.3.8149 Look for this fix in 8.0.0-preview.3.8149! legacy-area-perf Startup / Runtime performance memory-leak 💦 Memory usage grows / objects live forever p/1 Work that is important, and has been scheduled for release in this or an upcoming sprint partner/cat 😻 this is an issue that impacts one of our partners or a customer our advisory team is engaged with platform/windows 🪟 t/bug Something isn't working t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.)
Projects
None yet
Milestone
.NET 8 Planning
8 participants
@Eilon @jonathanpeppers @danielancines @PureWeen @jsuarezruiz @samhouts @vfabulokwe @mouralabank