From bb9330256ee7bea4aa1e25cf3e5f46b49fe9a428 Mon Sep 17 00:00:00 2001 From: Shane Neuville Date: Wed, 1 Mar 2023 17:54:16 -0600 Subject: [PATCH] [controls] fix memory leak in `Window` (#13400) Fixes: https://github.com/dotnet/maui/issues/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>` 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 --- .../Application/Application.Impl.cs | 20 ++++-- .../Core/HandlerImpl/Window/Window.Impl.cs | 1 + .../tests/Core.UnitTests/WindowsTests.cs | 64 ++++++++++++++++++- 3 files changed, 77 insertions(+), 8 deletions(-) diff --git a/src/Controls/src/Core/HandlerImpl/Application/Application.Impl.cs b/src/Controls/src/Core/HandlerImpl/Application/Application.Impl.cs index 68b6c2e6e717..3f38f65316c7 100644 --- a/src/Controls/src/Core/HandlerImpl/Application/Application.Impl.cs +++ b/src/Controls/src/Core/HandlerImpl/Application/Application.Impl.cs @@ -1,6 +1,7 @@ #nullable enable using System; using System.Collections.Generic; +using System.Runtime.CompilerServices; using Microsoft.Extensions.Logging; using Microsoft.Maui.ApplicationModel; using Microsoft.Maui.Devices; @@ -10,10 +11,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 _windows = new(); - readonly Dictionary _requestedWindows = new(); + readonly Dictionary> _requestedWindows = new(); ILogger? _logger; ILogger? Logger => @@ -30,8 +31,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 @@ -87,9 +94,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)); var state = new PersistedState { diff --git a/src/Controls/src/Core/HandlerImpl/Window/Window.Impl.cs b/src/Controls/src/Core/HandlerImpl/Window/Window.Impl.cs index a77158de08cc..454e1d7b816e 100644 --- a/src/Controls/src/Core/HandlerImpl/Window/Window.Impl.cs +++ b/src/Controls/src/Core/HandlerImpl/Window/Window.Impl.cs @@ -499,6 +499,7 @@ void IWindow.Destroying() OnDestroying(); Application?.RemoveWindow(this); + Handler?.DisconnectHandler(); } void IWindow.Resumed() diff --git a/src/Controls/tests/Core.UnitTests/WindowsTests.cs b/src/Controls/tests/Core.UnitTests/WindowsTests.cs index 71cff9390bc0..fed1f1d23b16 100644 --- a/src/Controls/tests/Core.UnitTests/WindowsTests.cs +++ b/src/Controls/tests/Core.UnitTests/WindowsTests.cs @@ -1,5 +1,9 @@ -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; @@ -554,5 +558,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().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); + } } }