Skip to content

Fix memory leak in FlyoutPage when changing the Detail #33358

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

Open
pictos wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Fix memory leak in FlyoutPage when changing the Detail #33358

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
c448ecc
clean up the DetailView
Jan 2, 2026
832c180
calling Disconnect handler when removing the FlyoutPage.Detail
Jan 2, 2026
65cf2c4
fix ScopedFragment leak
Jan 2, 2026
6ac1a37
remove and dispose the NavigationViewFragment
Jan 2, 2026
989ca43
call Dispose in MauiNavHostFragment
Jan 2, 2026
975eb88
code style
Jan 2, 2026
1e77dfb
Add unit test
pictos Jan 2, 2026
c935d8c
remove comments
pictos Jan 2, 2026
9a351d5
unit test
Jan 2, 2026
3ccd638
remove finalizer
Jan 2, 2026
5ca66d6
commit suggestions from copilot
pictos Jan 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Controls/src/Core/FlyoutPage/FlyoutPage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ public Page Detail
{
previousDetail.SendNavigatedFrom(
new NavigatedFromEventArgs(destinationPage: value, NavigationType.Replace));
previousDetail.Handler?.DisconnectHandler();
Copy link
Contributor Author

@pictos pictos Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not 100% sure about this one, maybe the dev. should be responsible to call the DisconnectHandler?

Copy link
Member

@PureWeen PureWeen Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine but we would want to call DisconnectHAndler after the page is unloaded

if you look at "void OnPageChanged(Page? oldPage, Page? newPage)" inside window you'll see where I wire into the unloaded event and then call disocnnecthandler from there.

Copy link
Contributor Author

@pictos pictos Jan 2, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it will not work, that method isn't called when the FyoutPage.Detail is replaced. For reference this is the scenario that opened the issue

https://github.com/3sRykaert/MauiMemoryleak/blob/5aced80e41a6a70b17b58b5a93054207150a4d05/MauiMemoryleak/NavigationService.cs#L18-L31

}

_detail.SendNavigatedTo(new NavigatedToEventArgs(previousDetail, NavigationType.Replace));
Expand Down
85 changes: 85 additions & 0 deletions src/Controls/tests/DeviceTests/Memory/MemoryTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ void SetupBuilder()
handlers.AddHandler<TimePicker, TimePickerHandler>();
handlers.AddHandler<Toolbar, ToolbarHandler>();
handlers.AddHandler<WebView, WebViewHandler>();
handlers.AddHandler<FlyoutPage, FlyoutViewHandler>();

#if IOS || MACCATALYST
handlers.AddHandler<NavigationPage, NavigationRenderer>();
Expand Down Expand Up @@ -577,6 +578,90 @@ await CreateHandlerAndAddToWindow(window, async () =>
await AssertionExtensions.WaitForGC([.. references]);
}

[Fact("FlyoutPage Detail Does Not Leak When Replaced")]
public async Task FlyoutPageDetailDoesNotLeak()
{
SetupBuilder();

var references = new List<WeakReference>();
var flyoutPage = new FlyoutPage
{
Flyout = new ContentPage { Title = "Flyout" },
Detail = new ContentPage { Title = "Detail" }
};

await CreateHandlerAndAddToWindow(new Window(flyoutPage), async () =>
{
await OnLoadedAsync(flyoutPage);

var detailPage1 = new ContentPage { Title = "Detail 1" };
var navPage1 = new NavigationPage(detailPage1);
flyoutPage.Detail = navPage1;

await OnLoadedAsync(detailPage1);

references.Add(new(navPage1));
references.Add(new(navPage1.Handler));
references.Add(new(navPage1.Handler.PlatformView));
references.Add(new(detailPage1));
references.Add(new(detailPage1.Handler));
references.Add(new(detailPage1.Handler.PlatformView));

var detailPage2 = new ContentPage { Title = "Detail 2" };
var navPage2 = new NavigationPage(detailPage2);
flyoutPage.Detail = navPage2;

await OnLoadedAsync(detailPage2);

navPage1 = null;
detailPage1 = null;
});

await AssertionExtensions.WaitForGC([.. references]);
}

[Fact("FlyoutPage Detail Does Not Leak With Multiple Replacements")]
public async Task FlyoutPageDetailDoesNotLeakWithMultipleReplacements()
{
SetupBuilder();

var references = new List<WeakReference>();
var flyoutPage = new FlyoutPage
{
Flyout = new ContentPage { Title = "Flyout" },
Detail = new ContentPage { Title = "Detail" }
};

await CreateHandlerAndAddToWindow(new Window(flyoutPage), async () =>
{
await OnLoadedAsync(flyoutPage);

for (int i = 0; i < 5; i++)
{
var detailPage = new ContentPage { Title = $"Detail {i}" };
var navPage = new NavigationPage(detailPage);

flyoutPage.Detail = navPage;
await OnLoadedAsync(detailPage);

if (i < 3)
{
references.Add(new(navPage));
references.Add(new(navPage.Handler));
references.Add(new(navPage.Handler.PlatformView));
references.Add(new(detailPage));
references.Add(new(detailPage.Handler));
references.Add(new(detailPage.Handler.PlatformView));
}

Copy link

Copilot AI Jan 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test delays (Task.Delay(50)) appear to be used to allow async operations to complete, but this could make tests flaky. Consider using a more deterministic approach, such as waiting for specific conditions or events to complete. If the delays are necessary for the GC to run or for fragments to be destroyed, add a comment explaining why this specific delay is needed.

Suggested change
// Give the platform time to complete disposal/teardown of the previous Detail
// (handlers, native views, fragments) before proceeding to the next iteration.
// This small delay has been found necessary to make the subsequent GC-based
// memory assertions reliable across devices.

Copilot uses AI. Check for mistakes.
await Task.Delay(50);
}

});

await AssertionExtensions.WaitForGC([.. references]);
}

[Fact("VisualDiagnosticsOverlay Does Not Leak"
#if IOS || MACCATALYST
, Skip = "Fails with 'MauiContext should have been set on parent.'"
Expand Down
6 changes: 6 additions & 0 deletions src/Core/src/Platform/Android/Navigation/MauiNavHostFragment.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,5 +18,11 @@ public MauiNavHostFragment()
protected MauiNavHostFragment(nint javaReference, JniHandleOwnership transfer) : base(javaReference, transfer)
{
}

public override void OnDestroy()
{
base.OnDestroy();
this.Dispose();
Copy link

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling Dispose() on a Fragment is unnecessary and potentially problematic. Android's Fragment lifecycle already handles resource cleanup through OnDestroy(). Explicitly calling Dispose() can cause issues with the Fragment lifecycle and may lead to double-disposal or premature resource cleanup.

Suggested change
this.Dispose();

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@pictos pictos Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not calling the dispose causes the memory leak

Copy link
Member

@PureWeen PureWeen Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm this seems wrong though, we shouldn't need to call Dispose here to fix a memory leak.
This seems like maybe the dispose is triggering something else that's possibilty fixing the leak?

Copy link
Contributor Author

@pictos pictos Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PureWeen not sure... Maybe it's somehow removing it from the fragmentManager? The leaks on android specific code was related to bad clean up on FragmentManager, before trying to call dispose I did all I find to remove the navHost from the fragment manager but nothing helped. Maybe calling the dispose from StackNavigationManager would be better?

}
}
}
2 changes: 2 additions & 0 deletions src/Core/src/Platform/Android/Navigation/NavigationViewFragment.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,10 @@ public override void OnDestroy()
{
_currentView = null;
_fragmentContainerView = null;
_navigationManager = null;

base.OnDestroy();
this.Dispose();
Copy link

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling Dispose() on a Fragment is unnecessary and potentially problematic. Android's Fragment lifecycle already handles resource cleanup through OnDestroy(). Explicitly calling Dispose() can cause issues with the Fragment lifecycle and may lead to double-disposal or premature resource cleanup. The nullification of fields in OnDestroy() is sufficient for preventing memory leaks.

Suggested change
this.Dispose();

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@pictos pictos Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not calling the dispose causes the memory leak

}

public override Animation OnCreateAnimation(int transit, bool enter, int nextAnim)
Expand Down
2 changes: 2 additions & 0 deletions src/Core/src/Platform/Android/Navigation/ScopedFragment.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ public override void OnDestroy()
{
base.OnDestroy();
IsDestroyed = true;

DetailView = null!;
Comment on lines 37 to +40
Copy link

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting DetailView to null! after base.OnDestroy() could cause issues if base.OnDestroy() or any fragment lifecycle callbacks try to access DetailView. Consider nullifying DetailView before calling base.OnDestroy() to prevent potential NullReferenceException scenarios during the fragment's destruction process.

Suggested change
base.OnDestroy();
IsDestroyed = true;
DetailView = null!;
IsDestroyed = true;
DetailView = null!;
base.OnDestroy();

Copilot uses AI. Check for mistakes.
}
}
}
38 changes: 35 additions & 3 deletions src/Core/src/Platform/Android/Navigation/StackNavigationManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using System;
using System.Collections.Generic;
using System.Linq;
using Android.Content;
using Android.OS;
using Android.Views;
Expand Down Expand Up @@ -309,14 +310,45 @@ public virtual void Disconnect()
_fragmentContainerView.ViewAttachedToWindow -= OnNavigationPlatformViewAttachedToWindow;
_fragmentContainerView.ChildViewAdded -= OnNavigationHostViewAdded;
}

if (_fragmentManager is not null)
{
CleanUpFragments(_fragmentManager);
}

_fragmentLifecycleCallbacks?.Disconnect();
_fragmentLifecycleCallbacks = null;

VirtualView = null;
NavigationView = null;
SetNavHost(null);
_fragmentNavigator = null;
_fragmentManager = null;
_fragmentContainerView = null;
_navGraph = null;
_currentPage = null;
NavigationStack = [];
ActiveRequestedArgs = null;
OnResumeRequestedArgs = null;
}


static void CleanUpFragments(FragmentManager fragmentManager)
{
fragmentManager.ExecutePendingTransactionsEx();
if (fragmentManager.BackStackEntryCount > 0)
{
fragmentManager.PopBackStackImmediate();
}

var transaction = fragmentManager.BeginTransactionEx();
foreach (var fragment in fragmentManager.Fragments)
{
transaction.RemoveEx(fragment);
}

transaction.CommitNowAllowingStateLoss();
fragmentManager.ExecutePendingTransactionsEx();
}

public virtual void Connect(IView navigationView)
Expand All @@ -343,7 +375,7 @@ public virtual void Connect(IView navigationView)
_fragmentContainerView.ChildViewAdded += OnNavigationHostViewAdded;
}
}

void OnNavigationPlatformViewAttachedToWindow(object? sender, AView.ViewAttachedToWindowEventArgs e)
{
// If the previous Navigation Host Fragment was destroyed then we need to add a new one
Expand Down Expand Up @@ -505,7 +537,7 @@ void SetNavHost(NavHostFragment? navHost)
(FragmentNavigator)NavController
.NavigatorProvider
.GetNavigator(Java.Lang.Class.FromType(typeof(FragmentNavigator)));

foreach (var fragment in _navHost.ChildFragmentManager.Fragments)
{
if (fragment is NavigationViewFragment nvf)
Expand Down
Loading