[iOS]Fix: FlyoutPage memory leak #28769
Conversation
|
Hey there @@bhavanesh2001! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
| @@ -269,9 +270,7 @@ protected override void Dispose(bool disposing) | |||
| } | |||
|
|
|||
| EmptyContainers(); | |||
|
|
|||
| Page.SendDisappearing(); | |||
There was a problem hiding this comment.
this shouldn't be here. We are already calling this above
There was a problem hiding this comment.
Does it matter if we just leave it?
There was a problem hiding this comment.
@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:
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.
src/Controls/src/Core/Compatibility/Handlers/FlyoutPage/iOS/PhoneFlyoutPageRenderer.cs
Outdated
Show resolved
Hide resolved
| { | ||
| App.Screenshot("I am at Bugzilla 31255"); | ||
| await Task.Delay(5000); | ||
| App.WaitForNoElement("Page1. But Page2 IsAlive = False"); | ||
| Thread.Sleep(5000); |
There was a problem hiding this comment.
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
@PureWeen is there a helper method for this in the UITests project?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@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. :)
jsuarezruiz
added
memory-leak 💦
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
The UI test is passing, but the device test failed on Android and Windows. It seems the issue was with the test itself, so I’ve modified it accordingly. |
|
/rebase |
github-actions
bot
force-pushed
the
flyoutpage_fix
branch
from
3f656df to
e444f42
Compare
Description of Change
While enabling the UITest
Bugzilla31255Test, which pushes aFlyoutPagemodally and then pops it, I encountered a memory leak on iOS. TheFlyoutPageinstance was not being collected even after the modal pop.Originally, the test case was also causing a crash because
FlyoutandDetailwere being set to instances ofPage, which is not supported. I updated them to useContentPage, which resolved the crash.The memory leak was caused by the renderer holding a strong reference to the virtual view (
_element), which prevented theFlyoutPagefrom being garbage collected.I also added a device test that reproduces the same modal flow and asserts that the page is collected by the GC. While this device test covers the same scenario as the UITest, I’ve kept both for now.
If you'd prefer to remove the UITest and rely only on the device test, let me know.
Issues Fixed
Fixes #21206