Skip to content

Commit

Permalink
[controls] fix memory leak in Application.OpenWindow()
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
jonathanpeppers committed Feb 22, 2023
1 parent 36dae62 commit eb1867c
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 4 deletions.
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using System;
using System.Collections.Generic;
using System.Runtime.CompilerServices;
using Microsoft.Extensions.Logging;
using Microsoft.Maui.ApplicationModel;
using Microsoft.Maui.Devices;
Expand All @@ -12,7 +13,7 @@ public partial class Application : IApplication
const string MauiWindowIdKey = "__MAUI_WINDOW_ID__";

readonly List<Window> _windows = new();
readonly Dictionary<string, Window> _requestedWindows = new();
readonly ConditionalWeakTable<string, Window> _requestedWindows = new();
ILogger<Application>? _logger;

ILogger<Application>? Logger =>
Expand All @@ -30,7 +31,10 @@ IWindow IApplication.CreateWindow(IActivationState? activationState)
if (activationState?.State?.TryGetValue(MauiWindowIdKey, out var requestedWindowId) ?? false)
{
if (requestedWindowId != null && _requestedWindows.TryGetValue(requestedWindowId, out var w))
{
window = w;
_requestedWindows.Remove(requestedWindowId);
}
}

// create a new one if there is no pending windows
Expand Down Expand Up @@ -86,9 +90,8 @@ internal void RemoveWindow(Window window)

public virtual void OpenWindow(Window window)
{
var id = Guid.NewGuid().ToString();

_requestedWindows[id] = window;
var id = Guid.NewGuid().ToString("n");
_requestedWindows.Add(id, window);

var state = new PersistedState
{
Expand Down
18 changes: 18 additions & 0 deletions src/Controls/tests/DeviceTests/Elements/Window/WindowTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -198,5 +198,23 @@ await CreateHandlerAndAddToWindow<WindowHandlerStub>(window, async (handler) =>
}
#endif

[Fact]
public async Task WindowsDoNotLeak()
{
var window = new Window(new ContentPage());
var windowReference = new WeakReference(window);

await CreateHandlerAndAddToWindow<WindowHandlerStub>(window, _ =>
{
new Application().OpenWindow(window);
});

window = null;
await Task.Yield();
GC.Collect();
GC.WaitForPendingFinalizers();

Assert.False(windowReference.IsAlive, "Window wasn't collected");
}
}
}

0 comments on commit eb1867c

Please sign in to comment.