Skip to content

Commit

Permalink
[controls] fix memory leak in Window
Browse files Browse the repository at this point in the history
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:

    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.
  • Loading branch information
jonathanpeppers committed Mar 1, 2023
1 parent 4487158 commit 2e7c3ff
Show file tree
Hide file tree
Showing 3 changed files with 76 additions and 8 deletions.
20 changes: 13 additions & 7 deletions src/Controls/src/Core/HandlerImpl/Application/Application.Impl.cs
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 @@ -9,10 +10,10 @@ namespace Microsoft.Maui.Controls
{
public partial class Application : IApplication
{
const string MauiWindowIdKey = "__MAUI_WINDOW_ID__";
internal const string MauiWindowIdKey = "__MAUI_WINDOW_ID__";

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

ILogger<Application>? Logger =>
Expand All @@ -29,8 +30,14 @@ IWindow IApplication.CreateWindow(IActivationState? activationState)
// try get the window that is pending
if (activationState?.State?.TryGetValue(MauiWindowIdKey, out var requestedWindowId) ?? false)
{
if (requestedWindowId != null && _requestedWindows.TryGetValue(requestedWindowId, out var w))
window = w;
if (requestedWindowId != null && _requestedWindows.TryGetValue(requestedWindowId, out var r))
{
if (r.TryGetTarget(out var w))
{
window = w;
}
_requestedWindows.Remove(requestedWindowId);
}
}

// create a new one if there is no pending windows
Expand Down Expand Up @@ -86,9 +93,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, new WeakReference<Window>(window));

var state = new PersistedState
{
Expand Down
1 change: 1 addition & 0 deletions src/Controls/src/Core/HandlerImpl/Window/Window.Impl.cs
Original file line number Diff line number Diff line change
Expand Up @@ -504,6 +504,7 @@ void IWindow.Destroying()
OnDestroying();

Application?.RemoveWindow(this);
Handler?.DisconnectHandler();
}

void IWindow.Resumed()
Expand Down
63 changes: 62 additions & 1 deletion src/Controls/tests/Core.UnitTests/WindowsTests.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
using System.Collections.Generic;
using System;
using System.Collections;
using System.Collections.Generic;
using System.Linq;
using System.Reflection;
using System.Threading.Tasks;
using Microsoft.Maui.Graphics;
using Xunit;
Expand Down Expand Up @@ -621,5 +624,63 @@ public void MinDimensionsArePassedToCoreCorrectly(double inW, double inH, double
Assert.Equal(outW, coreWindow.MinimumWidth);
Assert.Equal(outH, coreWindow.MinimumHeight);
}

[Fact]
public async Task WindowDoesNotLeak()
{
var application = new Application();
WeakReference reference;

// Scope for window
{
var window = new Window { Page = new ContentPage() };
reference = new WeakReference(window);
application.OpenWindow(window);
((IWindow)window).Destroying();
}

// GC
await Task.Yield();
GC.Collect();
GC.WaitForPendingFinalizers();

Assert.False(reference.IsAlive, "Window should not be alive!");
}

// NOTE: this test is here to show `ConditionalWeakTable _requestedWindows` was a bad idea
[Fact]
public async Task TwoKeysSameWindow()
{
var application = new Application();
var window = new Window { Page = new ContentPage() };

application.OpenWindow(window);

// Access Application._requestedWindows
var flags = BindingFlags.NonPublic | BindingFlags.Instance;
var table = typeof(Application).GetField("_requestedWindows", flags).GetValue(application) as IDictionary;
Assert.NotNull(table);

// A "cloned" key should still work
string key;
{
var originalKey = table.Keys.OfType<string>().Single();
key = new string(originalKey);
Assert.NotSame(originalKey, key);
}

// GC collect the original key
await Task.Yield();
GC.Collect();
GC.WaitForPendingFinalizers();

// Same window, doesn't create a new one
var actual = ((IApplication)application).CreateWindow(new ActivationState(new MockMauiContext(), new PersistedState
{
{ Application.MauiWindowIdKey, key }
}));
Assert.Same(window, actual);
Assert.Empty(table);
}
}
}

0 comments on commit 2e7c3ff

Please sign in to comment.