Skip to content

Commit

Permalink
[windows] fix VisualDiagnosticsOverlay keeping Page's alive (#23489)
Browse files Browse the repository at this point in the history
Fixes: #23034
Context: https://github.com/user-attachments/files/15817814/MauiApp1.zip

In the above sample app, you can observe a memory leak on Windows
by doing:

    Application.Current.MainPage = new Page1(); // This page lives forever
    Application.Current.MainPage = new Page2();

Reviewing memory snapshots, it appears that the
`VisualDiagnosticsOverlay` class is keeping the `Page1` instance
alive:

    MauiApp1.Page1
    -> Microsoft.Maui.Platform.ContentPanel
        -> Microsoft.Maui.Controls.VisualDiagnostics.VisualDiagnosticsOverlay
            -> Microsoft.Maui.Controls.Window

In this case, the `Window` holds a reference to the
`VisualDiagnosticsOverlay`, and the `Window` is still open.
`VisualDiagnosticsOverlay`'s base class is `WindowOverlay`, which
has a `_platformElement` field holding the `ContentPanel` which
holds the `Page1` instance.

In this case, `_platformElement` is only used for unsubscribing
events, so it feels like we can just change it to a `WeakReference`
to solve the issue.

I was able to write a test for this scenario as well.

I skipped the test on iOS/Catalyst, for now, as I was getting the error:

    MauiContext should have been set on parent.
  • Loading branch information
jonathanpeppers authored Jul 10, 2024
1 parent fe224da commit 191262d
Show file tree
Hide file tree
Showing 2 changed files with 43 additions and 10 deletions.
31 changes: 31 additions & 0 deletions src/Controls/tests/DeviceTests/Memory/MemoryTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -435,6 +435,37 @@ await CreateHandlerAndAddToWindow(window, async () =>
await AssertionExtensions.WaitForGC(references.ToArray());
}

[Fact("VisualDiagnosticsOverlay Does Not Leak"
#if IOS || MACCATALYST
, Skip = "Fails with 'MauiContext should have been set on parent.'"
#endif
)]
public async Task VisualDiagnosticsOverlayDoesNotLeak()
{
SetupBuilder();

var window = new WindowStub();
var overlay = new VisualDiagnosticsOverlay(window);
var references = new List<WeakReference>();

{
await InvokeOnMainThreadAsync(async () =>
{
var page = new ContentPage();
window.Content = page;
await CreateHandlerAsync(page);
overlay.Initialize();
references.Add(new(page));
references.Add(new(page.Handler));
references.Add(new(page.Handler.PlatformView));

window.Content = null;
});
}

await AssertionExtensions.WaitForGC(references.ToArray());
}

#if IOS
[Fact]
public async Task ResignFirstResponderTouchGestureRecognizer()
Expand Down
22 changes: 12 additions & 10 deletions src/Core/src/WindowOverlay/WindowOverlay.Windows.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System.Linq;
using System;
using System.Linq;
using Microsoft.Maui.Graphics;
using Microsoft.Maui.Graphics.Platform;
using Microsoft.Maui.Graphics.Win2D;
Expand All @@ -13,7 +14,7 @@ public partial class WindowOverlay
W2DGraphicsView? _graphicsView;
Frame? _frame;
WindowRootViewContainer? _panel;
FrameworkElement? _platformElement;
WeakReference<FrameworkElement>? _platformElement;

/// <inheritdoc/>
public virtual bool Initialize()
Expand All @@ -24,9 +25,10 @@ public virtual bool Initialize()
if (Window?.Content == null)
return false;

_platformElement = Window.Content.ToPlatform();
if (_platformElement == null)
var platformElement = Window.Content.ToPlatform();
if (platformElement is null)
return false;
_platformElement = new(platformElement);

var handler = Window.Handler as WindowHandler;
if (handler?.PlatformView is not Window _window)
Expand All @@ -38,7 +40,7 @@ public virtual bool Initialize()

// Capture when the frame is navigating.
// When it is, we will clear existing adorners.
if (_platformElement is Frame frame)
if (platformElement is Frame frame)
{
_frame = frame;
_frame.Navigating += FrameNavigating;
Expand All @@ -48,8 +50,8 @@ public virtual bool Initialize()
if (_graphicsView == null)
return false;

_platformElement.Tapped += ViewTapped;
_platformElement.PointerMoved += PointerMoved;
platformElement.Tapped += ViewTapped;
platformElement.PointerMoved += PointerMoved;
_graphicsView.Tapped += ViewTapped;
_graphicsView.PointerMoved += PointerMoved;

Expand Down Expand Up @@ -82,10 +84,10 @@ void DeinitializePlatformDependencies()
{
if (_frame != null)
_frame.Navigating -= FrameNavigating;
if (_platformElement != null)
if (_platformElement is not null && _platformElement.TryGetTarget(out var platformElement))
{
_platformElement.Tapped -= ViewTapped;
_platformElement.PointerMoved -= PointerMoved;
platformElement.Tapped -= ViewTapped;
platformElement.PointerMoved -= PointerMoved;
}
if (_graphicsView != null)
{
Expand Down

0 comments on commit 191262d

Please # to comment.