Fix Shell.Items.Clear() memory leak by disconnecting child handlers on removal (#34898)#35031
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://github.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 35031Or
iex "& { $(irm https://github.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 35031" |
|
Hey there @@Shalini-Ashokan! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
|
/azp run maui-pr-uitests , maui-pr-devicetests |
|
Azure Pipelines successfully started running 2 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
This PR addresses a Shell root-items clearing memory leak by ensuring handler disconnection cascades to child views when Shell items/pages are removed, preventing native view retention across root swaps.
Changes:
- Add an Appium UI test + HostApp reproduction for Issue #34898 validating that page + child handlers are disconnected after
Shell.Items.Clear(). - iOS: when removing/disposing old ShellContent renderers, recursively disconnect handlers from the virtual view tree (
DisconnectHandlers) instead of only disconnecting the page handler. - Android: when the
ShellFragmentContainerfragment is destroyed, recursively disconnect handlers for the page and its children.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue34898.cs | Adds UI test asserting handler disconnection after Shell.Items.Clear() + navigation. |
| src/Controls/tests/TestCases.HostApp/Issues/Issue34898.cs | Adds HostApp issue page to reproduce/verify handler disconnection state. |
| src/Controls/src/Core/Compatibility/Handlers/Shell/iOS/ShellSectionRootRenderer.cs | Switches cleanup to recursive handler disconnection for removed/old renderers. |
| src/Controls/src/Core/Compatibility/Handlers/Shell/Android/ShellFragmentContainer.cs | Adds fragment OnDestroy cleanup to disconnect page + child handlers to avoid leaks. |
| @@ -0,0 +1,105 @@ | |||
| namespace Maui.Controls.Sample.Issues; | |||
|
|
|||
| [Issue(IssueTracker.Github, 34898, "Shell.Items.Clear does not disconnect handlers correctly", PlatformAffected.iOS | PlatformAffected.Android)] | |||
There was a problem hiding this comment.
The new UI test will run on MacCatalyst (TestCases.Mac.Tests defines TEST_FAILS_ON_WINDOWS), but this Issue page is marked as affected only on iOS/Android, so it may not appear/navigate in the HostApp on MacCatalyst. Either include macOS in PlatformAffected here (if the fix is expected to apply to MacCatalyst too) or adjust the test preprocessor guards to exclude MacCatalyst.
| [Issue(IssueTracker.Github, 34898, "Shell.Items.Clear does not disconnect handlers correctly", PlatformAffected.iOS | PlatformAffected.Android)] | |
| [Issue(IssueTracker.Github, 34898, "Shell.Items.Clear does not disconnect handlers correctly", PlatformAffected.iOS | PlatformAffected.Android | PlatformAffected.macOS)] |
kubaflo
left a comment
There was a problem hiding this comment.
Could you please review the ai's summary?
@kubaflo, I addressed the AI concerns |
kubaflo
left a comment
There was a problem hiding this comment.
Could you please review the AI's summary?
🤖 AI Summary
📊 Review Session —
|
| Test | Without Fix (expect FAIL) | With Fix (expect PASS) |
|---|---|---|
🖥️ Issue34898 Issue34898 |
✅ FAIL — 508s | ✅ PASS — 494s |
🔴 Without fix — 🖥️ Issue34898: FAIL ✅ · 508s
Determining projects to restore...
All projects are up-to-date for restore.
##vso[build.updatebuildnumber]10.0.70-ci+azdo.13940072
Graphics -> /home/vsts/work/1/s/artifacts/bin/Graphics/Debug/net10.0-android36.0/Microsoft.Maui.Graphics.dll
##vso[build.updatebuildnumber]10.0.70-ci+azdo.13940072
Essentials -> /home/vsts/work/1/s/artifacts/bin/Essentials/Debug/net10.0-android36.0/Microsoft.Maui.Essentials.dll
##vso[build.updatebuildnumber]10.0.70-ci+azdo.13940072
Core -> /home/vsts/work/1/s/artifacts/bin/Core/Debug/net10.0-android36.0/Microsoft.Maui.dll
Controls.BindingSourceGen -> /home/vsts/work/1/s/artifacts/bin/Controls.BindingSourceGen/Debug/netstandard2.0/Microsoft.Maui.Controls.BindingSourceGen.dll
##vso[build.updatebuildnumber]10.0.70-ci+azdo.13940072
Maps -> /home/vsts/work/1/s/artifacts/bin/Maps/Debug/net10.0-android36.0/Microsoft.Maui.Maps.dll
##vso[build.updatebuildnumber]10.0.70-ci+azdo.13940072
Controls.Core -> /home/vsts/work/1/s/artifacts/bin/Controls.Core/Debug/net10.0-android36.0/Microsoft.Maui.Controls.dll
##vso[build.updatebuildnumber]10.0.70-ci+azdo.13940072
##vso[build.updatebuildnumber]10.0.70-ci+azdo.13940072
Controls.Xaml -> /home/vsts/work/1/s/artifacts/bin/Controls.Xaml/Debug/net10.0-android36.0/Microsoft.Maui.Controls.Xaml.dll
Microsoft.AspNetCore.Components.WebView.Maui -> /home/vsts/work/1/s/artifacts/bin/Microsoft.AspNetCore.Components.WebView.Maui/Debug/net10.0-android36.0/Microsoft.AspNetCore.Components.WebView.Maui.dll
##vso[build.updatebuildnumber]10.0.70-ci+azdo.13940072
##vso[build.updatebuildnumber]10.0.70-ci+azdo.13940072
Controls.Foldable -> /home/vsts/work/1/s/artifacts/bin/Controls.Foldable/Debug/net10.0-android36.0/Microsoft.Maui.Controls.Foldable.dll
Controls.Maps -> /home/vsts/work/1/s/artifacts/bin/Controls.Maps/Debug/net10.0-android36.0/Microsoft.Maui.Controls.Maps.dll
Controls.TestCases.HostApp -> /home/vsts/work/1/s/artifacts/bin/Controls.TestCases.HostApp/Debug/net10.0-android/Controls.TestCases.HostApp.dll
##vso[build.updatebuildnumber]10.0.70-ci+azdo.13940072
Graphics -> /home/vsts/work/1/s/artifacts/bin/Controls.TestCases.HostApp/Debug/net10.0-android/Microsoft.Maui.Graphics.dll
##vso[build.updatebuildnumber]10.0.70-ci+azdo.13940072
Essentials -> /home/vsts/work/1/s/artifacts/bin/Controls.TestCases.HostApp/Debug/net10.0-android/Microsoft.Maui.Essentials.dll
##vso[build.updatebuildnumber]10.0.70-ci+azdo.13940072
Core -> /home/vsts/work/1/s/artifacts/bin/Controls.TestCases.HostApp/Debug/net10.0-android/Microsoft.Maui.dll
##vso[build.updatebuildnumber]10.0.70-ci+azdo.13940072
Maps -> /home/vsts/work/1/s/artifacts/bin/Controls.TestCases.HostApp/Debug/net10.0-android/Microsoft.Maui.Maps.dll
Controls.BindingSourceGen -> /home/vsts/work/1/s/artifacts/bin/Controls.BindingSourceGen/Debug/netstandard2.0/Microsoft.Maui.Controls.BindingSourceGen.dll
##vso[build.updatebuildnumber]10.0.70-ci+azdo.13940072
Controls.Core -> /home/vsts/work/1/s/artifacts/bin/Controls.TestCases.HostApp/Debug/net10.0-android/Microsoft.Maui.Controls.dll
##vso[build.updatebuildnumber]10.0.70-ci+azdo.13940072
##vso[build.updatebuildnumber]10.0.70-ci+azdo.13940072
##vso[build.updatebuildnumber]10.0.70-ci+azdo.13940072
Controls.Xaml -> /home/vsts/work/1/s/artifacts/bin/Controls.TestCases.HostApp/Debug/net10.0-android/Microsoft.Maui.Controls.Xaml.dll
Controls.Foldable -> /home/vsts/work/1/s/artifacts/bin/Controls.TestCases.HostApp/Debug/net10.0-android/Microsoft.Maui.Controls.Foldable.dll
Microsoft.AspNetCore.Components.WebView.Maui -> /home/vsts/work/1/s/artifacts/bin/Controls.TestCases.HostApp/Debug/net10.0-android/Microsoft.AspNetCore.Components.WebView.Maui.dll
##vso[build.updatebuildnumber]10.0.70-ci+azdo.13940072
Controls.Maps -> /home/vsts/work/1/s/artifacts/bin/Controls.TestCases.HostApp/Debug/net10.0-android/Microsoft.Maui.Controls.Maps.dll
Build succeeded.
0 Warning(s)
0 Error(s)
Time Elapsed 00:06:28.26
Broadcasting: Intent { act=android.intent.action.CLOSE_SYSTEM_DIALOGS flg=0x400000 }
Broadcast completed: result=0
Broadcasting: Intent { act=android.intent.action.CLOSE_SYSTEM_DIALOGS flg=0x400000 }
Broadcast completed: result=0
Determining projects to restore...
All projects are up-to-date for restore.
Controls.CustomAttributes -> /home/vsts/work/1/s/artifacts/bin/Controls.CustomAttributes/Debug/net10.0/Controls.CustomAttributes.dll
##vso[build.updatebuildnumber]10.0.70-ci+azdo.13940072
Graphics -> /home/vsts/work/1/s/artifacts/bin/Graphics/Debug/net10.0/Microsoft.Maui.Graphics.dll
##vso[build.updatebuildnumber]10.0.70-ci+azdo.13940072
Essentials -> /home/vsts/work/1/s/artifacts/bin/Essentials/Debug/net10.0/Microsoft.Maui.Essentials.dll
##vso[build.updatebuildnumber]10.0.70-ci+azdo.13940072
Core -> /home/vsts/work/1/s/artifacts/bin/Core/Debug/net10.0/Microsoft.Maui.dll
Controls.BindingSourceGen -> /home/vsts/work/1/s/artifacts/bin/Controls.BindingSourceGen/Debug/netstandard2.0/Microsoft.Maui.Controls.BindingSourceGen.dll
##vso[build.updatebuildnumber]10.0.70-ci+azdo.13940072
Controls.Core -> /home/vsts/work/1/s/artifacts/bin/Controls.Core/Debug/net10.0/Microsoft.Maui.Controls.dll
UITest.Core -> /home/vsts/work/1/s/artifacts/bin/UITest.Core/Debug/net10.0/UITest.Core.dll
VisualTestUtils -> /home/vsts/work/1/s/artifacts/bin/VisualTestUtils/Debug/netstandard2.0/VisualTestUtils.dll
UITest.NUnit -> /home/vsts/work/1/s/artifacts/bin/UITest.NUnit/Debug/net10.0/UITest.NUnit.dll
VisualTestUtils.MagickNet -> /home/vsts/work/1/s/artifacts/bin/VisualTestUtils.MagickNet/Debug/netstandard2.0/VisualTestUtils.MagickNet.dll
UITest.Appium -> /home/vsts/work/1/s/artifacts/bin/UITest.Appium/Debug/net10.0/UITest.Appium.dll
UITest.Analyzers -> /home/vsts/work/1/s/artifacts/bin/UITest.Analyzers/Debug/netstandard2.0/UITest.Analyzers.dll
Controls.TestCases.Android.Tests -> /home/vsts/work/1/s/artifacts/bin/Controls.TestCases.Android.Tests/Debug/net10.0/Controls.TestCases.Android.Tests.dll
Test run for /home/vsts/work/1/s/artifacts/bin/Controls.TestCases.Android.Tests/Debug/net10.0/Controls.TestCases.Android.Tests.dll (.NETCoreApp,Version=v10.0)
VSTest version 18.0.1 (x64)
Starting test execution, please wait...
A total of 1 test files matched the specified pattern.
/home/vsts/work/1/s/artifacts/bin/Controls.TestCases.Android.Tests/Debug/net10.0/Controls.TestCases.Android.Tests.dll
[xUnit.net 00:00:00.00] xUnit.net VSTest Adapter v2.8.2+699d445a1a (64-bit .NET 10.0.0)
[xUnit.net 00:00:00.11] Discovering: Controls.TestCases.Android.Tests
[xUnit.net 00:00:00.29] Discovered: Controls.TestCases.Android.Tests
NUnit Adapter 4.5.0.0: Test execution started
Running selected tests in /home/vsts/work/1/s/artifacts/bin/Controls.TestCases.Android.Tests/Debug/net10.0/Controls.TestCases.Android.Tests.dll
NUnit3TestExecutor discovered 1 of 1 NUnit test cases using Current Discovery mode, Non-Explicit run
>>>>> 04/26/2026 12:12:06 FixtureSetup for Issue34898(Android)
>>>>> 04/26/2026 12:12:09 ShellItemsClearShouldDisconnectChildHandlers Start
>>>>> 04/26/2026 12:12:14 ShellItemsClearShouldDisconnectChildHandlers Stop
>>>>> 04/26/2026 12:12:14 Log types: logcat, bugreport, server
Failed ShellItemsClearShouldDisconnectChildHandlers [6 s]
Error Message:
Page handler should be disconnected after Shell.Items.Clear()
Assert.That(App.WaitForElement("PageHandlerStatus").GetText(), Is.EqualTo("Disconnected"))
Expected string length 12 but was 9. Strings differ at index 0.
Expected: "Disconnected"
But was: "Connected"
-----------^
Stack Trace:
at Microsoft.Maui.TestCases.Tests.Issues.Issue34898.ShellItemsClearShouldDisconnectChildHandlers() in /_/src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue34898.cs:line 27
1) at Microsoft.Maui.TestCases.Tests.Issues.Issue34898.ShellItemsClearShouldDisconnectChildHandlers() in /_/src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue34898.cs:line 27
NUnit Adapter 4.5.0.0: Test execution complete
Test Run Failed.
Total tests: 1
Failed: 1
Total time: 23.0702 Seconds
🟢 With fix — 🖥️ Issue34898: PASS ✅ · 494s
Determining projects to restore...
All projects are up-to-date for restore.
##vso[build.updatebuildnumber]10.0.70-ci+azdo.13940072
Graphics -> /home/vsts/work/1/s/artifacts/bin/Graphics/Debug/net10.0-android36.0/Microsoft.Maui.Graphics.dll
##vso[build.updatebuildnumber]10.0.70-ci+azdo.13940072
Essentials -> /home/vsts/work/1/s/artifacts/bin/Essentials/Debug/net10.0-android36.0/Microsoft.Maui.Essentials.dll
##vso[build.updatebuildnumber]10.0.70-ci+azdo.13940072
Core -> /home/vsts/work/1/s/artifacts/bin/Core/Debug/net10.0-android36.0/Microsoft.Maui.dll
Controls.BindingSourceGen -> /home/vsts/work/1/s/artifacts/bin/Controls.BindingSourceGen/Debug/netstandard2.0/Microsoft.Maui.Controls.BindingSourceGen.dll
##vso[build.updatebuildnumber]10.0.70-ci+azdo.13940072
##vso[build.updatebuildnumber]10.0.70-ci+azdo.13940072
Maps -> /home/vsts/work/1/s/artifacts/bin/Maps/Debug/net10.0-android36.0/Microsoft.Maui.Maps.dll
Controls.Core -> /home/vsts/work/1/s/artifacts/bin/Controls.Core/Debug/net10.0-android36.0/Microsoft.Maui.Controls.dll
##vso[build.updatebuildnumber]10.0.70-ci+azdo.13940072
##vso[build.updatebuildnumber]10.0.70-ci+azdo.13940072
##vso[build.updatebuildnumber]10.0.70-ci+azdo.13940072
##vso[build.updatebuildnumber]10.0.70-ci+azdo.13940072
Controls.Foldable -> /home/vsts/work/1/s/artifacts/bin/Controls.Foldable/Debug/net10.0-android36.0/Microsoft.Maui.Controls.Foldable.dll
Controls.Xaml -> /home/vsts/work/1/s/artifacts/bin/Controls.Xaml/Debug/net10.0-android36.0/Microsoft.Maui.Controls.Xaml.dll
Controls.Maps -> /home/vsts/work/1/s/artifacts/bin/Controls.Maps/Debug/net10.0-android36.0/Microsoft.Maui.Controls.Maps.dll
Microsoft.AspNetCore.Components.WebView.Maui -> /home/vsts/work/1/s/artifacts/bin/Microsoft.AspNetCore.Components.WebView.Maui/Debug/net10.0-android36.0/Microsoft.AspNetCore.Components.WebView.Maui.dll
Controls.TestCases.HostApp -> /home/vsts/work/1/s/artifacts/bin/Controls.TestCases.HostApp/Debug/net10.0-android/Controls.TestCases.HostApp.dll
##vso[build.updatebuildnumber]10.0.70-ci+azdo.13940072
Graphics -> /home/vsts/work/1/s/artifacts/bin/Controls.TestCases.HostApp/Debug/net10.0-android/Microsoft.Maui.Graphics.dll
##vso[build.updatebuildnumber]10.0.70-ci+azdo.13940072
Essentials -> /home/vsts/work/1/s/artifacts/bin/Controls.TestCases.HostApp/Debug/net10.0-android/Microsoft.Maui.Essentials.dll
##vso[build.updatebuildnumber]10.0.70-ci+azdo.13940072
Core -> /home/vsts/work/1/s/artifacts/bin/Controls.TestCases.HostApp/Debug/net10.0-android/Microsoft.Maui.dll
Controls.BindingSourceGen -> /home/vsts/work/1/s/artifacts/bin/Controls.BindingSourceGen/Debug/netstandard2.0/Microsoft.Maui.Controls.BindingSourceGen.dll
##vso[build.updatebuildnumber]10.0.70-ci+azdo.13940072
Maps -> /home/vsts/work/1/s/artifacts/bin/Controls.TestCases.HostApp/Debug/net10.0-android/Microsoft.Maui.Maps.dll
##vso[build.updatebuildnumber]10.0.70-ci+azdo.13940072
Controls.Core -> /home/vsts/work/1/s/artifacts/bin/Controls.TestCases.HostApp/Debug/net10.0-android/Microsoft.Maui.Controls.dll
##vso[build.updatebuildnumber]10.0.70-ci+azdo.13940072
##vso[build.updatebuildnumber]10.0.70-ci+azdo.13940072
##vso[build.updatebuildnumber]10.0.70-ci+azdo.13940072
##vso[build.updatebuildnumber]10.0.70-ci+azdo.13940072
Microsoft.AspNetCore.Components.WebView.Maui -> /home/vsts/work/1/s/artifacts/bin/Controls.TestCases.HostApp/Debug/net10.0-android/Microsoft.AspNetCore.Components.WebView.Maui.dll
Controls.Maps -> /home/vsts/work/1/s/artifacts/bin/Controls.TestCases.HostApp/Debug/net10.0-android/Microsoft.Maui.Controls.Maps.dll
Controls.Foldable -> /home/vsts/work/1/s/artifacts/bin/Controls.TestCases.HostApp/Debug/net10.0-android/Microsoft.Maui.Controls.Foldable.dll
Controls.Xaml -> /home/vsts/work/1/s/artifacts/bin/Controls.TestCases.HostApp/Debug/net10.0-android/Microsoft.Maui.Controls.Xaml.dll
Build succeeded.
0 Warning(s)
0 Error(s)
Time Elapsed 00:06:21.91
Broadcasting: Intent { act=android.intent.action.CLOSE_SYSTEM_DIALOGS flg=0x400000 }
Broadcast completed: result=0
Broadcasting: Intent { act=android.intent.action.CLOSE_SYSTEM_DIALOGS flg=0x400000 }
Broadcast completed: result=0
Determining projects to restore...
All projects are up-to-date for restore.
##vso[build.updatebuildnumber]10.0.70-ci+azdo.13940072
Graphics -> /home/vsts/work/1/s/artifacts/bin/Graphics/Debug/net10.0/Microsoft.Maui.Graphics.dll
Controls.CustomAttributes -> /home/vsts/work/1/s/artifacts/bin/Controls.CustomAttributes/Debug/net10.0/Controls.CustomAttributes.dll
##vso[build.updatebuildnumber]10.0.70-ci+azdo.13940072
Essentials -> /home/vsts/work/1/s/artifacts/bin/Essentials/Debug/net10.0/Microsoft.Maui.Essentials.dll
##vso[build.updatebuildnumber]10.0.70-ci+azdo.13940072
Core -> /home/vsts/work/1/s/artifacts/bin/Core/Debug/net10.0/Microsoft.Maui.dll
Controls.BindingSourceGen -> /home/vsts/work/1/s/artifacts/bin/Controls.BindingSourceGen/Debug/netstandard2.0/Microsoft.Maui.Controls.BindingSourceGen.dll
##vso[build.updatebuildnumber]10.0.70-ci+azdo.13940072
Controls.Core -> /home/vsts/work/1/s/artifacts/bin/Controls.Core/Debug/net10.0/Microsoft.Maui.Controls.dll
VisualTestUtils -> /home/vsts/work/1/s/artifacts/bin/VisualTestUtils/Debug/netstandard2.0/VisualTestUtils.dll
UITest.Core -> /home/vsts/work/1/s/artifacts/bin/UITest.Core/Debug/net10.0/UITest.Core.dll
UITest.Appium -> /home/vsts/work/1/s/artifacts/bin/UITest.Appium/Debug/net10.0/UITest.Appium.dll
UITest.NUnit -> /home/vsts/work/1/s/artifacts/bin/UITest.NUnit/Debug/net10.0/UITest.NUnit.dll
VisualTestUtils.MagickNet -> /home/vsts/work/1/s/artifacts/bin/VisualTestUtils.MagickNet/Debug/netstandard2.0/VisualTestUtils.MagickNet.dll
UITest.Analyzers -> /home/vsts/work/1/s/artifacts/bin/UITest.Analyzers/Debug/netstandard2.0/UITest.Analyzers.dll
Controls.TestCases.Android.Tests -> /home/vsts/work/1/s/artifacts/bin/Controls.TestCases.Android.Tests/Debug/net10.0/Controls.TestCases.Android.Tests.dll
Test run for /home/vsts/work/1/s/artifacts/bin/Controls.TestCases.Android.Tests/Debug/net10.0/Controls.TestCases.Android.Tests.dll (.NETCoreApp,Version=v10.0)
VSTest version 18.0.1 (x64)
Starting test execution, please wait...
A total of 1 test files matched the specified pattern.
/home/vsts/work/1/s/artifacts/bin/Controls.TestCases.Android.Tests/Debug/net10.0/Controls.TestCases.Android.Tests.dll
[xUnit.net 00:00:00.00] xUnit.net VSTest Adapter v2.8.2+699d445a1a (64-bit .NET 10.0.0)
[xUnit.net 00:00:00.11] Discovering: Controls.TestCases.Android.Tests
[xUnit.net 00:00:00.34] Discovered: Controls.TestCases.Android.Tests
NUnit Adapter 4.5.0.0: Test execution started
Running selected tests in /home/vsts/work/1/s/artifacts/bin/Controls.TestCases.Android.Tests/Debug/net10.0/Controls.TestCases.Android.Tests.dll
NUnit3TestExecutor discovered 1 of 1 NUnit test cases using Current Discovery mode, Non-Explicit run
>>>>> 04/26/2026 12:20:21 FixtureSetup for Issue34898(Android)
>>>>> 04/26/2026 12:20:24 ShellItemsClearShouldDisconnectChildHandlers Start
>>>>> 04/26/2026 12:20:30 ShellItemsClearShouldDisconnectChildHandlers Stop
Passed ShellItemsClearShouldDisconnectChildHandlers [5 s]
NUnit Adapter 4.5.0.0: Test execution complete
Test Run Successful.
Total tests: 1
Passed: 1
Total time: 22.1597 Seconds
📁 Fix files reverted (2 files)
src/Controls/src/Core/Compatibility/Handlers/Shell/Android/ShellFragmentContainer.cssrc/Controls/src/Core/Compatibility/Handlers/Shell/iOS/ShellSectionRootRenderer.cs
🧪 UI Tests — Category Detection
Detected UI test categories: Shell
🔍 Pre-Flight — Context & Validation
Issue: #34898 - Shell.Items.Clear() does not disconnect handlers correctly
PR: #35031 - Fix Shell.Items.Clear() memory leak by disconnecting child handlers on removal
Platforms Affected: Android, iOS (macOS/MacCatalyst noted in PR as also validated)
Files Changed: 2 implementation, 2 test
Key Findings
- Shell.Items.Clear() only disconnected the top-level page handler, leaving child element handlers (Label, Button, Entry, etc.) still wired to native views
- On Android: neither page handler nor child handlers were disconnected — fragment's
OnDestroyView()was recycling the page without cleaning up handlers - On iOS: page handler was disconnected but child handlers inside the page were skipped —
DisconnectHandler()called instead of recursiveDisconnectHandlers() - PR fixes both platforms by calling
DisconnectHandlers()(recursive) instead ofDisconnectHandler()at all relevant teardown callsites - Test added with
#if TEST_FAILS_ON_WINDOWSand tracking issue [Windows]Shell.Items.Clear() does not disconnect handlers correctly #35035 for Windows
Edge Cases from Comments
- Copilot reviewer noted
PlatformAffected.macOSshould be included in the HostApp test attribute (already applied by author per current state) - Reviewer
kubaflorequested AI summary review (author states addressed in latest commit)
Code Review Summary
Verdict: LGTM
Confidence: high
Errors: 0 | Warnings: 1 | Suggestions: 1
Key code review findings:
⚠️ ShellFragmentContainer.OnDestroyView()callsDisconnectHandlers()BEFORERecyclePage(), inverting the ordering pattern used by sibling classShellContentFragment.RecyclePageis currently a no-op so there is no functional impact today, but ifRecyclePageis ever implemented to cache/reuse pages, the recycled page would have null handlers. Safer order: RecyclePage → DisconnectHandlers.- 💡 Test HostApp uses static fields for tracked elements — safe for CI (one run) but could cause issues in multi-run scenarios (effectively benign given
Init()reassigns them)
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #35031 | Call DisconnectHandlers() (recursive) before recycling on Android, replace DisconnectHandler() with DisconnectHandlers() on iOS |
✅ PASSED (Gate) | ShellFragmentContainer.cs, ShellSectionRootRenderer.cs |
Original PR |
🔬 Code Review — Deep Analysis
Code Review — PR #35031
Independent Assessment
What this changes:
- Android (
ShellFragmentContainer): Adds_page?.DisconnectHandlers()inOnDestroyView()beforeRecyclePage()and_page = null, so all child element handlers are cleaned up when the fragment view is destroyed. - iOS (
ShellSectionRootRenderer): In three teardown callsites (Dispose(),RemoveNonVisibleRenderers(),OnShellSectionItemsChanged()), replacesoldRenderer.DisconnectHandler()with a pattern that callsview.DisconnectHandlers()when the virtual view is anIView, falling back tooldRenderer.DisconnectHandler()otherwise.
Inferred motivation: Shell navigation infrastructure was only disconnecting the top-level page handler, leaving all child element handlers (Labels, Buttons, Entries, etc.) still wired up. These retained native view references after the page was no longer part of the visual hierarchy, causing memory growth with each Shell restructure.
Is the approach sound? Yes. DisconnectHandlers() is the established recursive-teardown pattern used in NavigationPage, ShellSectionRenderer, Window, and BindableLayout. The iOS change correctly leaves IDisconnectable.Disconnect() untouched (temporary disconnect for tab switching, not permanent teardown) which is the right call — per Shell's re-add pattern (rule §9), permanent handler teardown must not happen in temporary disconnect paths.
Reconciliation with PR Narrative
Author claims: Shell.Items.Clear() leaks native views because child handlers aren't disconnected. Android leaks both page and children; iOS leaks only the children.
Agreement: Code analysis confirms this exactly. The fix targets the right callsites. Both the Android and iOS paths were in "permanent removal" contexts (fragment destroyed, renderer disposed) where full recursive disconnection is correct.
Findings
⚠️ Warning — Android: DisconnectHandlers() called before RecyclePage(), inverting sibling class pattern
ShellFragmentContainer.OnDestroyView() now does:
_page?.DisconnectHandlers(); // NEW
((IShellContentController)ShellContentTab).RecyclePage(_page);
_page = null;The sibling ShellContentFragment uses the opposite order:
// In Destroy():
((IShellContentController)_shellContent).RecyclePage(_page);
_page.Handler = null;
// In DisposePage():
_page.DisconnectHandlers();
_page = null;RecyclePage is currently a no-op ({ } in ShellContent.cs:124), so there is zero functional difference today. However, calling DisconnectHandlers() — which nulls out all child Handler references — before RecyclePage() means that if anyone ever fills in RecyclePage to cache pages for reuse, the recycled page would have null handlers and be unusable. The safer and more consistent ordering mirrors the existing pattern: RecyclePage → DisconnectHandlers.
💡 Suggestion — Test: HostApp Issue34898 class uses static fields that persist across test runs
static Label _trackedLabel;
static Entry _trackedEntry;
static Button _trackedButton;
static ContentPage _trackedPage;Static fields are a valid approach here (the second page needs to reach back to check the first page's handler state), but they also keep hard references to the discarded page/elements for the lifetime of the process. This is fine for CI since the test only runs once, but if the test is ever run multiple times in the same process (e.g., test-retry scenarios), Init() reassigns them so there's no real issue. Just worth noting.
Devil's Advocate
"Is DisconnectHandlers() in OnDestroyView() causing handlers to be torn down on every tab switch, not just on Items.Clear()?"
Investigated: ShellFragmentContainer is used by ShellFragmentStateAdapter (a FragmentStateAdapter for ViewPager2). ViewPager2 can call OnDestroyView() when off-screen pages are destroyed during tab switches. However, looking at the pre-existing code: RecyclePage + _page = null was already called in OnDestroyView(), meaning OnCreateView() was already creating a fresh page on every view recreation. The PR only adds proper handler cleanup before that discard. This is not a regression.
"Could the iOS IView type check ever be false for a page renderer, causing the fallback DisconnectHandler() to be used instead of DisconnectHandlers()?"
Page implements IView via VisualElement → View → IView. The renderers in _renderers are created from pages via page.ToHandler(...). VirtualView will always be the page, which is always an IView. The fallback is a correct defensive guard but will not execute in practice.
"Is CI green?" Yes — all maui-pr checks pass (build, unit tests, integration tests, helix). The test is appropriately gated with #if TEST_FAILS_ON_WINDOWS with a reference to the tracking issue #35035.
Verdict: LGTM
Confidence: high
The fix correctly addresses a genuine memory leak by switching from single-handler to recursive-handler disconnection at all appropriate teardown callsites on both platforms. The approach is consistent with the established MAUI pattern used in NavigationPage, ShellSectionRenderer, Window, and BindableLayout. CI is fully green. The one ordering concern in Android (DisconnectHandlers before RecyclePage) is harmless today given RecyclePage is an empty method, but worth noting for future-proofing.
🔧 Fix — Analysis & Comparison
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| 1 | try-fix (claude-opus-4.6) | Centralize disconnect in ShellContent.RecyclePage() for Android; same recursive DisconnectHandlers() on iOS |
✅ PASS | ShellContent.cs + ShellSectionRootRenderer.cs |
Fixes Android at model layer — ShellFragmentContainer.cs untouched |
| 2 | try-fix (claude-sonnet-4.6) | Cross-platform: extend ShellContent.EvaluateDisconnect() to call DisconnectHandlers() for non-service pages when Window == null |
✅ PASS | ShellContent.cs only |
Fully cross-platform, leverages existing Unloaded event architecture; no platform files touched |
| 3 | try-fix (gpt-5.3-codex) | Explicit visual-tree recursive disconnect helper; Android defers to OnDestroy() rather than OnDestroyView(); iOS same multi-site pattern |
✅ PASS | ShellFragmentContainer.cs + ShellSectionRootRenderer.cs |
More code (37+ additions) — adds a custom tree walker instead of using DisconnectHandlers() |
| 4 | try-fix (gpt-5.5) | Disconnect in ShellFragmentStateAdapter.Dispose() using _createdShellContent inventory |
✅ PASS | ShellFragmentStateAdapter.cs |
Hooks at adapter layer; minimal (+8 lines); semantically cleanest Android-only approach |
| PR | PR #35031 | Add DisconnectHandlers() in ShellFragmentContainer.OnDestroyView() for Android; replace DisconnectHandler() with DisconnectHandlers() at 3 iOS sites |
✅ PASSED (Gate) | ShellFragmentContainer.cs + ShellSectionRootRenderer.cs |
Original PR |
Cross-Pollination
| Model | Round | New Ideas? | Details |
|---|---|---|---|
| claude-opus-4.6 | 2 | No | All seams covered |
| claude-sonnet-4.6 | 2 | No | All lifecycle hooks covered |
| gpt-5.3-codex | 2 | Proposed | Proactive teardown in ShellSection.OnVisibleChildRemoved on Items.Clear — rejected: unsafe during transition animations (gpt-5.5 analysis) |
| gpt-5.5 | 2 | No | PR's OnDestroyView site is the correct safe point |
Exhausted: Yes — all 4 models said NO NEW IDEAS after Round 2 (one idea proposed but evaluated unsafe)
Selected Fix: PR's fix — simple, minimal (3 lines on Android, 12 on iOS), targets the correct teardown sites, passes Gate, consistent with MAUI patterns. Attempt 1 (RecyclePage) is also compelling for Android — but iOS still requires the 3-site fix regardless. The PR's combined approach is the most direct and surgical solution.
📋 Report — Final Recommendation
✅ Final Recommendation: APPROVE
Phase Status
| Phase | Status | Notes |
|---|---|---|
| Pre-Flight | ✅ COMPLETE | Issue #34898 + PR #35031 reviewed; 4 files classified |
| Code Review | LGTM (high) | 0 errors, 1 warning, 1 suggestion |
| Gate | ✅ PASSED | Android · Issue34898 FAIL→PASS |
| Try-Fix | ✅ COMPLETE | 4 attempts, 4 passing |
| Report | ✅ COMPLETE |
Code Review Impact on Try-Fix
Code review flagged a DisconnectHandlers() before RecyclePage() in Android. This directly influenced Try-Fix exploration: Attempt 1 (claude-opus-4.6) specifically fixed the ordering concern by centralizing the disconnect inside RecyclePage() itself rather than adding a line before it. Attempt 4 (gpt-5.5) took a different architectural approach (adapter-layer disposal) that also avoids the ordering concern entirely. The failure-mode probe about tab-switch regression shaped Attempts 2 and 4, both of which explicitly guard against disconnecting handlers during transient off-screen scenarios.
Summary
PR #35031 fixes a genuine memory leak in Shell.Items.Clear() by switching from single-handler to recursive-handler disconnection at the correct platform teardown callsites on both Android and iOS. The fix is minimal (3 lines on Android, 12 on iOS), targeted at the right lifecycle points, consistent with established MAUI patterns (DisconnectHandlers() is used in NavigationPage, Window, BindableLayout), and validated by Gate. Try-Fix exploration with 4 models all produced passing alternatives — confirming the fix surface is correct — but the PR's approach remains the simplest and most readable. The one warning (Android ordering) is harmless today since RecyclePage is an empty no-op.
Root Cause
When Shell items are removed via Shell.Items.Clear(), the platform teardown code was only calling DisconnectHandler() (single handler) instead of DisconnectHandlers() (recursive tree disconnect). On Android, ShellFragmentContainer.OnDestroyView() called RecyclePage but never called any handler disconnect at all. On iOS, ShellSectionRootRenderer called oldRenderer.DisconnectHandler() which only disconnected the page-level handler, leaving all child element handlers (Labels, Buttons, Entries) still wired to native platform views that accumulated in memory.
Fix Quality
The PR's fix is well-targeted and minimal. The Android change (3 lines) adds _page?.DisconnectHandlers() in OnDestroyView() — the correct permanent-removal lifecycle event. The iOS change (12 lines) consistently replaces DisconnectHandler() with the recursive DisconnectHandlers() pattern at all 3 renderer teardown sites. The approach is idiomatic — DisconnectHandlers() is the established MAUI pattern for recursive teardown. Gate confirms the fix actually resolves the issue. Code review found no errors. The only outstanding concern (ordering relative to RecyclePage) is future-proofing advice, not a blocking issue.
|
I addressed the AI concerns |
…n removal (#34898) (#35031) <!-- !!!!!!! MAIN IS THE ONLY ACTIVE BRANCH. MAKE SURE THIS PR IS TARGETING MAIN. !!!!!!! --> <!-- Please let the below note in for people that find this PR --> > [!NOTE] > Are you waiting for the changes in this PR to be merged? > It would be very helpful if you could [test the resulting artifacts](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! <!-- !!!!!!! MAIN IS THE ONLY ACTIVE BRANCH. MAKE SURE THIS PR IS TARGETING MAIN. !!!!!!! --> ### Issue Details Clearing the Shell's root items causes a memory leak. On iOS, the old page is cleaned up but its child elements (labels, buttons, etc.) are not. On Android, neither the page nor its children are cleaned up. Leaked native views accumulate with each root page swap, eventually degrading performance or crashing the app. ### Root Cause When items are removed from Shell, the system only removes the old page from its internal list — it does not tell the native platform to destroy the actual screen elements. On Android, the fragment cleanup recycled the page without disconnecting its child handlers. On iOS, only the page-level handler was disconnected but the children inside it were skipped. So every time items were removed, the old native views stayed alive in memory, invisible but never freed. ### Description of Change On Android, when the old page's view is destroyed, we now also disconnect all its children (labels, buttons, entries, etc.) — not just recycle the page. On iOS, when the renderer is disposed, we now disconnect the children inside the page — not just the page itself. This ensures all native views are properly freed when Shell items are removed. Validated the behavior in the following platforms - [x] Android - [ ] Windows - [x] iOS - [x ] Mac ### Issues Fixed Fixes #34898 ### Output ScreenShot |Before|After| |--|--| | <video src="https://github.com/user-attachments/assets/db75c53e-3fdf-4b28-ab16-aa48a6ea9cd7" >| <video src="https://github.com/user-attachments/assets/38ffe4ae-e6c8-4673-867c-61ade57a0b96">|
…n removal (#34898) (#35031) <!-- !!!!!!! MAIN IS THE ONLY ACTIVE BRANCH. MAKE SURE THIS PR IS TARGETING MAIN. !!!!!!! --> <!-- Please let the below note in for people that find this PR --> > [!NOTE] > Are you waiting for the changes in this PR to be merged? > It would be very helpful if you could [test the resulting artifacts](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! <!-- !!!!!!! MAIN IS THE ONLY ACTIVE BRANCH. MAKE SURE THIS PR IS TARGETING MAIN. !!!!!!! --> ### Issue Details Clearing the Shell's root items causes a memory leak. On iOS, the old page is cleaned up but its child elements (labels, buttons, etc.) are not. On Android, neither the page nor its children are cleaned up. Leaked native views accumulate with each root page swap, eventually degrading performance or crashing the app. ### Root Cause When items are removed from Shell, the system only removes the old page from its internal list — it does not tell the native platform to destroy the actual screen elements. On Android, the fragment cleanup recycled the page without disconnecting its child handlers. On iOS, only the page-level handler was disconnected but the children inside it were skipped. So every time items were removed, the old native views stayed alive in memory, invisible but never freed. ### Description of Change On Android, when the old page's view is destroyed, we now also disconnect all its children (labels, buttons, entries, etc.) — not just recycle the page. On iOS, when the renderer is disposed, we now disconnect the children inside the page — not just the page itself. This ensures all native views are properly freed when Shell items are removed. Validated the behavior in the following platforms - [x] Android - [ ] Windows - [x] iOS - [x ] Mac ### Issues Fixed Fixes #34898 ### Output ScreenShot |Before|After| |--|--| | <video src="https://github.com/user-attachments/assets/db75c53e-3fdf-4b28-ab16-aa48a6ea9cd7" >| <video src="https://github.com/user-attachments/assets/38ffe4ae-e6c8-4673-867c-61ade57a0b96">|
…ability (#35133) <!-- Please let the below note in for people that find this PR --> > [!NOTE] > Are you waiting for the changes in this PR to be merged? > It would be very helpful if you could [test the resulting artifacts](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! > **Depends on #35136** (pipeline category detection — should merge first) ## What this does Two things: ### 1. UI test category detection in PR review During the PR review workflow, Step 0.5 detects which UI test categories the PR impacts and writes the result to the AI summary comment. This gives reviewers visibility into which UI tests are relevant. **Detection** reuses the 3-tier script from #35136 (test attributes → source paths → AI reasoning). **AI summary** shows a new 🧪 UI Tests section with detected categories before the gate section. ### 2. Gate reliability fixes Multiple fixes to make the gate (`verify-tests-fail.ps1`) more deterministic: | Fix | Problem it solves | |-----|-------------------| | **Absolute path resolution** | Gate scripts not found on Linux CI agents (`Resolve-Path`, `GetFullPath`) | | **File existence check** | Instant cryptic failure when verify script is missing — now logs clear error | | **3x retry on ENV ERROR** | Emulator timeouts, ADB failures, app crashes — transient issues that pass on retry | | **Strip bad report blocks** | Old verify script produces `Passed: False` with empty counts — stripped instead of shown | | **Gate log in fallback** | When report is missing, shows last 20 lines of gate output instead of just `❌ FAILED / Platform: IOS` | ## Files | File | Changes | |------|---------| | `.github/scripts/Review-PR.ps1` | Step 0.5 category detection + all 5 gate fixes | | `.github/scripts/post-ai-summary-comment.ps1` | Add `uitests` phase to render detected categories | | `.github/pr-review/pr-preflight.md` | Step 7: AI identifies impacted UI test categories | ## Validation — PR reviewer builds (Apr 26) 10 builds against real PRs — all succeeded ✅. Category detection shown in AI summary comment. | PR | Categories Detected | Build | AI Summary | |----|-------------------|-------|------------| | #35037 (WebView theme) | `ViewBaseTests,WebView` | [13940071](https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=13940071) | [comment](#35037 (comment)) | | #35031 (Shell memory leak) | `Shell` | [13940072](https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=13940072) | [comment](#35031 (comment)) | | #35020 (XAML Hot Reload) | _(none — XAML only)_ | [13940073](https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=13940073) | ✅ Shows "No UI test categories" | | #35008 (Shell SearchHandler) | `Shell` | [13940074](https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=13940074) | ✅ | | #34997 (RadioButton gradient) | `RadioButton,ViewBaseTests` | [13940075](https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=13940075) | ✅ | | #34980 (DatePicker rotation) | `ViewBaseTests` | [13940076](https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=13940076) | ✅ | | #34974 (Picker CharacterSpacing) | `ViewBaseTests` | [13940077](https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=13940077) | ✅ | | #34923 (SwipeView threshold) | `SwipeView,ViewBaseTests` | [13940078](https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=13940078) | ✅ | | #34907 (CollectionView ScrollTo) | `CollectionView` | [13940079](https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=13940079) | ✅ | | #34845 (RefreshView binding) | `RefreshView,ViewBaseTests` | [13940080](https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=13940080) | ✅ | --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Issue Details
Clearing the Shell's root items causes a memory leak. On iOS, the old page is cleaned up but its child elements (labels, buttons, etc.) are not. On Android, neither the page nor its children are cleaned up. Leaked native views accumulate with each root page swap, eventually degrading performance or crashing the app.
Root Cause
When items are removed from Shell, the system only removes the old page from its internal list — it does not tell the native platform to destroy the actual screen elements. On Android, the fragment cleanup recycled the page without disconnecting its child handlers.
On iOS, only the page-level handler was disconnected but the children inside it were skipped. So every time items were removed, the old native views stayed alive in memory, invisible but never freed.
Description of Change
On Android, when the old page's view is destroyed, we now also disconnect all its children (labels, buttons, entries, etc.) — not just recycle the page.
On iOS, when the renderer is disposed, we now disconnect the children inside the page — not just the page itself. This ensures all native views are properly freed when Shell items are removed.
Validated the behavior in the following platforms
Issues Fixed
Fixes #34898
Output ScreenShot
34898-BeforeFix.mov
34898-AfterFix.mov