Skip to content

[iOS]Fix: FlyoutPage memory leak #28769

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
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

[iOS]Fix: FlyoutPage memory leak #28769

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
802f1b6
fix
bhavanesh2001 Apr 2, 2025
4bb19f2
enable Bugzilla31255Test
bhavanesh2001 Apr 2, 2025
d17bd1b
add device test
bhavanesh2001 Apr 2, 2025
451d2cf
make _element a weak reference directly
bhavanesh2001 Apr 3, 2025
e444f42
fix device test
bhavanesh2001 Apr 4, 2025
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
10 changes: 4 additions & 6 deletions src/Controls/src/Core/Compatibility/Handlers/FlyoutPage/iOS/PhoneFlyoutPageRenderer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ public class PhoneFlyoutPageRenderer : UIViewController, IPlatformViewHandler
{
UIView _clickOffView;
UIViewController _detailController;
VisualElement _element;
WeakReference<VisualElement> _element;
bool _disposed;

UIViewController _flyoutController;
Expand Down Expand Up @@ -75,7 +75,7 @@ bool Presented
get { return _presented; }
}

public VisualElement Element => _viewHandlerWrapper.Element ?? _element;
public VisualElement Element => _viewHandlerWrapper.Element ?? _element?.GetTargetOrDefault();

public event EventHandler<VisualElementChangedEventArgs> ElementChanged;

Expand All @@ -100,7 +100,7 @@ public void SetElement(VisualElement element)
_clickOffView = new UIView();
_clickOffView.BackgroundColor = new Color(0, 0, 0, 0).ToPlatform();
_viewHandlerWrapper.SetVirtualView(element, OnElementChanged, false);
_element = element;
_element = new(element);

if (_intialLayoutFinished)
{
Expand Down Expand Up @@ -269,9 +269,7 @@ protected override void Dispose(bool disposing)
}

EmptyContainers();

Page.SendDisappearing();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this shouldn't be here. We are already calling this above

maui/src/Controls/src/Core/Compatibility/Handlers/FlyoutPage/iOS/PhoneFlyoutPageRenderer.cs

Line 132 in 8ee00e0

Page?.SendDisappearing();

Copy link
Member

Choose a reason for hiding this comment

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

Does it matter if we just leave it?

Copy link
Contributor Author

@bhavanesh2001 bhavanesh2001 Apr 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@PureWeen Without the PR changes, since we’re inside Dispose, the wrapper’s Element will be null at this point — so Page falls back to the strongly referenced _element:

maui/src/Controls/src/Core/Compatibility/Handlers/FlyoutPage/iOS/PhoneFlyoutPageRenderer.cs

Line 78 in 8ee00e0

public VisualElement Element => _viewHandlerWrapper.Element ?? _element;

With the PR changes, _element is resolved through a weak reference, so keeping the SendDisappearing() call here won’t make a difference

Let me know if you think I should keep the call.


_element = null;
_disposed = true;
}

Expand Down
33 changes: 33 additions & 0 deletions src/Controls/tests/DeviceTests/Elements/FlyoutPage/FlyoutPageTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,39 @@ await CreateHandlerAndAddToWindow<FlyoutViewHandler>(flyoutPage, async (handler)
});
}

[Fact(DisplayName = "FlyoutPage as Modal Does Not Leak")]
public async Task DoesNotLeakAsModal()
{
SetupBuilder();

var references = new List<WeakReference>();
var launcherPage = new ContentPage();
var window = new Window(launcherPage);

await CreateHandlerAndAddToWindow<WindowHandlerStub>(window, async handler =>
{
var flyoutPage = new FlyoutPage
{
Flyout = new ContentPage
{
Title = "Flyout",
IconImageSource = "icon.png"
},
Detail = new ContentPage { Title = "Detail" }
};

await launcherPage.Navigation.PushModalAsync(flyoutPage, true);

references.Add(new WeakReference(flyoutPage));
references.Add(new WeakReference(flyoutPage.Flyout));
references.Add(new WeakReference(flyoutPage.Detail));

await launcherPage.Navigation.PopModalAsync();
});

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

bool CanDeviceDoSplitMode(FlyoutPage page)
{
return ((IFlyoutPageController)page).ShouldShowSplitMode;
Expand Down
11 changes: 8 additions & 3 deletions src/Controls/tests/TestCases.HostApp/Issues/Bugzilla/Bugzilla31255.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ public MainPage()
{
VerticalOptions = LayoutOptions.Center,
HorizontalTextAlignment = TextAlignment.Center,
Text = "Page 1"
Text = "Page 1",
AutomationId = "MauiLabel"
});

Content = stack;
Expand Down Expand Up @@ -60,12 +61,12 @@ public class Page2 : FlyoutPage
{
public Page2()
{
Flyout = new Page()
Flyout = new ContentPage
{
Title = "Flyout",
IconImageSource = "Icon.png"
};
Detail = new Page() { Title = "Detail" };
Detail = new ContentPage() { Title = "Detail" };
}

protected override async void OnAppearing()
Expand All @@ -75,6 +76,10 @@ protected override async void OnAppearing()
await Task.Delay(1000);
await Navigation.PopModalAsync();
}
protected override void OnDisappearing()
{
base.OnDisappearing();
}
}
}
}
Expand Down
14 changes: 6 additions & 8 deletions src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Bugzilla/Bugzilla31255.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
#if IOS
using NUnit.Framework;
using NUnit.Framework;
using UITest.Appium;
using UITest.Core;

Expand All @@ -14,15 +13,14 @@ public Bugzilla31255(TestDevice testDevice) : base(testDevice)
public override string Issue => "Flyout's page Icon cause memory leak after FlyoutPage is popped out by holding on page";

[Test]
[Ignore("The sample is crashing. More information: https://github.com/dotnet/maui/issues/21206")]
[Category(UITestCategories.Navigation)]
[Category(UITestCategories.Compatibility)]
public async Task Bugzilla31255Test()
public void Bugzilla31255Test()
{
App.Screenshot("I am at Bugzilla 31255");
await Task.Delay(5000);
App.WaitForNoElement("Page1. But Page2 IsAlive = False");
Thread.Sleep(5000);
Copy link
Member

Choose a reason for hiding this comment

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

This test is probably going to be flaky as-is (possible to fail at some percentage), best results are to do something like these other tests:

maui/src/Controls/tests/Core.UnitTests/WeakEventProxyTests.cs

Lines 25 to 27 in dad356b

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

@PureWeen is there a helper method for this in the UITests project?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jonathanpeppers checking for memory leaks in a UI test isn’t really ideal and not something we typically do. That’s exactly why I added a device test that covers the same modal FlyoutPage scenario.

I’ve still kept the UI test for now just because it was already there and passing with the fix, but I think we can safely remove it if we’re good with the device test coverage.

Copy link
Contributor

Choose a reason for hiding this comment

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

@PureWeen is there a helper method for this in the UITests project?

Recently, there was this attempt https://github.com/dotnet/maui/pull/28489/files by @simonrozsival. Hopefully, it will get resurrected. :)

var text = App.WaitForElement("MauiLabel").GetText();
Assert.That(text, Is.EqualTo("Page1. But Page2 IsAlive = False"));
}
}
}
#endif
}