-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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.
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
Changes from all commits
321d5b9
0607dfc
03024a9
240f17c
5d3fd8f
bedfb80
9d70c64
04a8f30
6792b08
73d7d4a
41ccb0f
6e0ceab
eea1e4d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| <?xml version="1.0" encoding="utf-8" ?> | ||
| <FlyoutPage xmlns="http://schemas.microsoft.com/dotnet/2021/maui" | ||
| xmlns:x="http://schemas.microsoft.com/winfx/2009/xaml" | ||
| x:Class="Maui.Controls.Sample.AppFlyoutPage" | ||
| Title="AppFlyoutPage"> | ||
| <FlyoutPage.Flyout> | ||
| <ContentPage Title="Menu"> | ||
| <CollectionView ItemsSource="{Binding MenuItems}" | ||
| SelectionMode="Single" | ||
| SelectionChanged="OnMenuItemSelected"> | ||
| <CollectionView.ItemTemplate> | ||
| <DataTemplate> | ||
| <Grid Padding="10"> | ||
| <Label Text="{Binding Title}" | ||
| FontSize="16" | ||
| VerticalOptions="Center"/> | ||
| </Grid> | ||
| </DataTemplate> | ||
| </CollectionView.ItemTemplate> | ||
| </CollectionView> | ||
| </ContentPage> | ||
| </FlyoutPage.Flyout> | ||
| </FlyoutPage> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| using Maui.Controls.Sample.Services; | ||
|
|
||
| namespace Maui.Controls.Sample; | ||
|
|
||
| public partial class AppFlyoutPage : FlyoutPage | ||
| { | ||
| public class MenuItem | ||
| { | ||
| public string Title { get; set; } = string.Empty; | ||
| public Type PageType { get; set; } = typeof(Page); | ||
| } | ||
|
|
||
| public List<MenuItem> MenuItems { get; set; } | ||
|
|
||
| public AppFlyoutPage() | ||
| { | ||
| InitializeComponent(); | ||
|
|
||
| MenuItems = new List<MenuItem> | ||
| { | ||
| new MenuItem { Title = "page 1", PageType = typeof(Page1) }, | ||
| new MenuItem { Title = "page 2", PageType = typeof(Page2) } | ||
| }; | ||
|
|
||
| BindingContext = this; | ||
|
|
||
| // Set default detail page | ||
| Detail = new NavigationPage(new Page1()); | ||
| } | ||
|
|
||
| private void OnMenuItemSelected(object? sender, SelectionChangedEventArgs e) | ||
| { | ||
| if (e.CurrentSelection.FirstOrDefault() is MenuItem selectedItem) | ||
| { | ||
| var navigationService = new NavigationService(); | ||
| navigationService.Navigate(selectedItem.PageType); | ||
|
|
||
| // Close the flyout after selection | ||
| IsPresented = false; | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| <?xml version="1.0" encoding="utf-8" ?> | ||
| <ContentPage xmlns="http://schemas.microsoft.com/dotnet/2021/maui" | ||
| xmlns:x="http://schemas.microsoft.com/winfx/2009/xaml" | ||
| x:Class="Maui.Controls.Sample.Page1" | ||
| Title="page 1"> | ||
| <VerticalStackLayout Padding="30" | ||
| Spacing="25" | ||
| VerticalOptions="Center" | ||
| HorizontalOptions="Center"> | ||
| <Button Text="page 1" | ||
| Clicked="OnNavigateToPage2Clicked" | ||
| HorizontalOptions="Center"/> | ||
| <Button Text="Navigate to Page 2" | ||
| Clicked="OnNavigateToPage2AsyncClicked" | ||
| HorizontalOptions="Center"/> | ||
|
|
||
| <Entry Text="Test"/> | ||
| </VerticalStackLayout> | ||
| </ContentPage> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| using Maui.Controls.Sample.Services; | ||
|
|
||
| namespace Maui.Controls.Sample; | ||
|
|
||
| public partial class Page1 : ContentPage | ||
| { | ||
| private readonly NavigationService _navigationService; | ||
|
|
||
| public Page1() | ||
| { | ||
| InitializeComponent(); | ||
| _navigationService = new NavigationService(); | ||
|
|
||
| Loaded += (e, __) => | ||
| { | ||
| _ = MemoryTest.IsAliveAsync(); | ||
| }; | ||
| } | ||
|
|
||
| private void OnNavigateToPage2Clicked(object? sender, EventArgs e) | ||
| { | ||
| _navigationService.NavigateAbsolute(typeof(Page2)); | ||
| } | ||
|
|
||
| private void OnNavigateToPage2AsyncClicked(object? sender, EventArgs e) | ||
| { | ||
| _navigationService.Navigate(typeof(Page2)); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| <?xml version="1.0" encoding="utf-8" ?> | ||
| <ContentPage xmlns="http://schemas.microsoft.com/dotnet/2021/maui" | ||
| xmlns:x="http://schemas.microsoft.com/winfx/2009/xaml" | ||
| x:Class="Maui.Controls.Sample.Page2" | ||
| Title="page 2"> | ||
| <VerticalStackLayout Padding="30" | ||
| Spacing="25" | ||
| VerticalOptions="Center" | ||
| HorizontalOptions="Center"> | ||
| <Label Text="page 2" | ||
| FontSize="32" | ||
| HorizontalOptions="Center"/> | ||
| <Button Text="Navigate to Page 1" | ||
| Clicked="OnNavigateToPage1Clicked" | ||
| HorizontalOptions="Center"/> | ||
| <Entry Text="Test"/> | ||
| </VerticalStackLayout> | ||
| </ContentPage> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| using Maui.Controls.Sample.Services; | ||
|
|
||
| namespace Maui.Controls.Sample; | ||
|
|
||
| public partial class Page2 : ContentPage | ||
| { | ||
| private readonly NavigationService _navigationService; | ||
|
|
||
| public Page2() | ||
| { | ||
| InitializeComponent(); | ||
| _navigationService = new NavigationService(); | ||
| } | ||
|
|
||
| private void OnNavigateToPage1Clicked(object? sender, EventArgs e) | ||
| { | ||
| _navigationService.Navigate(typeof(Page1)); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,102 @@ | ||
| namespace Maui.Controls.Sample.Services; | ||
|
|
||
| public class NavigationService | ||
| { | ||
| public void NavigateAbsolute(Type pageType) | ||
| { | ||
| var flyoutPage = new AppFlyoutPage(); | ||
|
|
||
| if (pageType == typeof(Page1)) | ||
| { | ||
| flyoutPage.Detail = new NavigationPage(new Page1()); | ||
| } | ||
| else if (pageType == typeof(Page2)) | ||
| { | ||
| flyoutPage.Detail = new NavigationPage(new Page2()); | ||
| } | ||
|
|
||
| var page = (flyoutPage.Detail as NavigationPage)!.CurrentPage; | ||
|
|
||
| MemoryTest.Add(flyoutPage); | ||
| MemoryTest.Add(flyoutPage.Detail); | ||
| MemoryTest.Add(page); | ||
| Application.Current!.Windows[0].Page = flyoutPage; | ||
| } | ||
|
|
||
| public void Navigate(Type pageType) | ||
| { | ||
| var currentPage = Application.Current!.Windows[0].Page; | ||
|
|
||
| if (currentPage is FlyoutPage flyoutPage && flyoutPage.Detail is NavigationPage navigationPage) | ||
| { | ||
| Page? targetPage = null; | ||
|
|
||
| if (pageType == typeof(Page1)) | ||
| { | ||
| targetPage = new Page1(); | ||
| } | ||
| else if (pageType == typeof(Page2)) | ||
| { | ||
| targetPage = new Page2(); | ||
| } | ||
|
|
||
| if (targetPage != null) | ||
| { | ||
| flyoutPage.Detail = new NavigationPage(targetPage); | ||
| flyoutPage.IsPresented = false; | ||
| } | ||
|
|
||
|
|
||
| MemoryTest.Add(flyoutPage); | ||
| MemoryTest.Add(flyoutPage.Detail); | ||
| MemoryTest.Add(targetPage!); | ||
| } | ||
| } | ||
| } | ||
| static class MemoryTest | ||
| { | ||
| static readonly List<WeakReference<object>> weakReferences = []; | ||
|
|
||
| public static int Count => weakReferences.Count; | ||
|
|
||
| public static void Add(object obj) | ||
| { | ||
| weakReferences.Add(new(obj)); | ||
| } | ||
|
|
||
| public static async Task IsAliveAsync() | ||
| { | ||
| RunGC(); | ||
| for (var i = 0; i < weakReferences.Count; i++) | ||
| { | ||
| var item = weakReferences[i]; | ||
|
|
||
| if (item.TryGetTarget(out var target)) | ||
| { | ||
| var type = target.GetType(); | ||
| var name = type.FullName; | ||
|
|
||
| #pragma warning disable CS0618 // Type or member is obsolete | ||
| await App.Current!.MainPage!.DisplayAlert("Memory leak!", $"The {name} object of {type} Type is still alive", "Ok"); | ||
| #pragma warning restore CS0618 // Type or member is obsolete | ||
| } | ||
| else | ||
| { | ||
| weakReferences.Remove(item); | ||
| } | ||
| RunGC(); | ||
| } | ||
|
|
||
|
|
||
| #pragma warning disable CS0618 // Type or member is obsolete | ||
| await App.Current!.MainPage!.DisplayAlert("Memory leak!", $"The are {weakReferences.Count} objects still alive", "Ok"); | ||
| #pragma warning restore CS0618 // Type or member is obsolete | ||
| } | ||
|
|
||
| static void RunGC() | ||
| { | ||
| GC.Collect(); | ||
| GC.WaitForPendingFinalizers(); | ||
| GC.Collect(); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -76,6 +76,7 @@ public Page Detail | |
| { | ||
| previousDetail.SendNavigatedFrom( | ||
| new NavigatedFromEventArgs(destinationPage: value, NavigationType.Replace)); | ||
| previousDetail.Handler?.DisconnectHandler(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it will not work, that method isn't called when the |
||
| } | ||
|
|
||
| _detail.SendNavigatedTo(new NavigatedToEventArgs(previousDetail, NavigationType.Replace)); | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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>(); | ||||||||||||||
|
|
@@ -577,6 +578,91 @@ await CreateHandlerAndAddToWindow(window, async () => | |||||||||||||
| await AssertionExtensions.WaitForGC([.. references]); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| [Fact("FlyoutPage Detail Handler Is Disconnected When Replaced")] | ||||||||||||||
| public async Task FlyoutPageDetailDoesNotLeak() | ||||||||||||||
| { | ||||||||||||||
| SetupBuilder(); | ||||||||||||||
|
|
||||||||||||||
| var references = new List<WeakReference>(); | ||||||||||||||
| var flyoutPage = new FlyoutPage | ||||||||||||||
| { | ||||||||||||||
| Flyout = new ContentPage { Title = "Flyout" }, | ||||||||||||||
| Detail = new NavigationPage(new ContentPage { Title = "Initial Detail" }) | ||||||||||||||
| }; | ||||||||||||||
|
|
||||||||||||||
| await CreateHandlerAndAddToWindow(new Window(flyoutPage), async () => | ||||||||||||||
| { | ||||||||||||||
| await OnLoadedAsync(flyoutPage); | ||||||||||||||
|
|
||||||||||||||
| // Capture old detail and its handler before replacement | ||||||||||||||
| var oldDetail = flyoutPage.Detail; | ||||||||||||||
| var oldHandler = oldDetail.Handler; | ||||||||||||||
| Assert.NotNull(oldHandler); | ||||||||||||||
|
|
||||||||||||||
| references.Add(new(oldDetail)); | ||||||||||||||
| references.Add(new(oldHandler)); | ||||||||||||||
| references.Add(new(oldHandler.PlatformView)); | ||||||||||||||
|
|
||||||||||||||
| // Replace detail | ||||||||||||||
| var newDetailPage = new ContentPage { Title = "New Detail" }; | ||||||||||||||
| flyoutPage.Detail = new NavigationPage(newDetailPage); | ||||||||||||||
| await OnLoadedAsync(newDetailPage); | ||||||||||||||
|
|
||||||||||||||
| // The fix calls previousDetail.Handler?.DisconnectHandler() in the Detail setter. | ||||||||||||||
| // Without the fix, the old handler stays connected (non-null). | ||||||||||||||
| Assert.Null(oldDetail.Handler); | ||||||||||||||
|
|
||||||||||||||
| oldDetail = null; | ||||||||||||||
| oldHandler = null; | ||||||||||||||
| }); | ||||||||||||||
|
|
||||||||||||||
| await AssertionExtensions.WaitForGC([.. references]); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| [Fact("FlyoutPage Detail Handler Disconnects With Multiple Replacements")] | ||||||||||||||
| public async Task FlyoutPageDetailDoesNotLeakWithMultipleReplacements() | ||||||||||||||
| { | ||||||||||||||
| SetupBuilder(); | ||||||||||||||
|
|
||||||||||||||
| var references = new List<WeakReference>(); | ||||||||||||||
| var flyoutPage = new FlyoutPage | ||||||||||||||
| { | ||||||||||||||
| Flyout = new ContentPage { Title = "Flyout" }, | ||||||||||||||
| Detail = new NavigationPage(new ContentPage { Title = "Initial Detail" }) | ||||||||||||||
| }; | ||||||||||||||
|
|
||||||||||||||
| await CreateHandlerAndAddToWindow(new Window(flyoutPage), async () => | ||||||||||||||
| { | ||||||||||||||
| await OnLoadedAsync(flyoutPage); | ||||||||||||||
|
|
||||||||||||||
| for (int i = 0; i < 5; i++) | ||||||||||||||
| { | ||||||||||||||
| var oldDetail = flyoutPage.Detail; | ||||||||||||||
| var oldHandler = oldDetail.Handler; | ||||||||||||||
| Assert.NotNull(oldHandler); | ||||||||||||||
|
|
||||||||||||||
| if (i < 3) | ||||||||||||||
| { | ||||||||||||||
| references.Add(new(oldDetail)); | ||||||||||||||
| references.Add(new(oldHandler)); | ||||||||||||||
| references.Add(new(oldHandler.PlatformView)); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
|
||||||||||||||
| // 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. |
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -18,5 +18,11 @@ public MauiNavHostFragment() | |||
| protected MauiNavHostFragment(nint javaReference, JniHandleOwnership transfer) : base(javaReference, transfer) | ||||
| { | ||||
| } | ||||
|
|
||||
| public override void OnDestroy() | ||||
| { | ||||
| base.OnDestroy(); | ||||
| this.Dispose(); | ||||
|
||||
| this.Dispose(); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?