[iOS] Fix ContentPage with ToolbarItem Clicked event leaks when presented as modal page#35009
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 35009Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 35009" |
There was a problem hiding this comment.
Pull request overview
Fixes an iOS-specific modal navigation memory leak where ToolbarItem native wrappers keep Clicked handlers rooted after PopModalAsync() (notably when the modal is wrapped in a NavigationPage).
Changes:
- Dispose modal and child handlers after iOS modal dismissal to release native toolbar item wrappers and break managed reference chains.
- Add a HostApp issue page (
Issue34892) that reproduces the leak scenario and reports surviving page instances. - Add an Appium-based UI test to validate that the modal page does not remain alive after dismissal + forced GC.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/Controls/src/Core/Platform/ModalNavigationManager/ModalNavigationManager.iOS.cs | Disposes modal/child handlers after dismiss to prevent ToolbarItem.Clicked-related retention on iOS. |
| src/Controls/tests/TestCases.HostApp/Issues/Issue34892.cs | Adds a repro UI that pushes a modal NavigationPage, pops it, and reports surviving instances via WeakReference. |
| src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue34892.cs | Adds an automated UI test to exercise the repro and assert that leaked pages are not retained. |
…nted as modal page (#35009) <!-- !!!!!!! MAIN IS THE ONLY ACTIVE BRANCH. MAKE SURE THIS PR IS TARGETING MAIN. !!!!!!! --> <!-- Please keep the note below for people who 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 whether this change resolves your issue. Thank you!<!-- !!!!!!! MAIN IS THE ONLY ACTIVE BRANCH. MAKE SURE THIS PR IS TARGETING MAIN. !!!!!!! --> ### Issue Details When a page with a ToolbarItem + Clicked handler was pushed as a modal wrapped in NavigationPage, then popped via PopModalAsync(), the page was not garbage collected. ### Root Cause iOS maps ToolbarItems → native UIBarButtonItem wrappers at push time. Those wrappers subscribe to Clicked/PropertyChanged on the ToolbarItem. On modal pop, iOS defers the native dismiss animation, so the UIBarButtonItem objects briefly outlive the managed page. That keeps the ToolbarItem event handler references rooted, preventing the page from being collected. ### Description of Change <!-- Enter description of the fix in this section --> Added handler disposal in both dismissal branches: * After successful dismiss via presenting controller * In the fallback branch when presenting controller is already null Note : The user mentioned that this issue was reproduced on Android, but it is not reproducible in the latest source. It seems to be resolved on the Android platform. So fix added only for iOS. ### Issues Fixed <!-- Please make sure that there is a bug logged for the issue being fixed. The bug should describe the problem and how to reproduce it. --> Fixes #34892 <!-- Are you targeting main? All PRs should target the main branch unless otherwise noted. --> **Tested the behavior in the following platforms.** - [x] Android - [x] Windows - [x] iOS - [x] Mac | Before | After | |---------|--------| | **iOS**<br> <video src="https://github.com/user-attachments/assets/cf55be12-7d26-455e-8c38-d7dd54828225" width="300" height="600"> | **iOS**<br> <video src="https://github.com/user-attachments/assets/8fffffb3-8f21-45fb-a53f-80dfaba1aa5e" width="300" height="600"> |
🤖 AI Summary
📊 Review Session —
|
| Check | Expected | Actual | Result |
|---|---|---|---|
| Tests WITHOUT fix | FAIL | FAIL | ✅ |
| Tests WITH fix | PASS | PASS | ✅ |
✅ Final Verdict
VERIFICATION PASSED ✅
The tests correctly detect the issue:
- ✅ Tests FAIL without the fix (as expected - bug is present)
- ✅ Tests PASS with the fix (as expected - bug is fixed)
Conclusion: The tests properly validate the fix and catch the bug when it's present.
Configuration
Platform: ios
Test Filter: Issue34892
Base Branch: main
Merge Base: e85007e
Fix Files
src/Controls/src/Core/Platform/ModalNavigationManager/ModalNavigationManager.iOS.cs
Test Results Details
Test Run 1: WITHOUT Fix
Expected: Tests should FAIL (bug is present)
Actual: Tests FAILED ✅
Test Summary:
- Total:
- Passed: False
- Failed:
- Skipped:
View full test output (without fix)
Determining projects to restore...
Restored /Users/cloudtest/vss/_work/1/s/src/Graphics/src/Graphics/Graphics.csproj (in 643 ms).
Restored /Users/cloudtest/vss/_work/1/s/src/Controls/src/BindingSourceGen/Controls.BindingSourceGen.csproj (in 634 ms).
Restored /Users/cloudtest/vss/_work/1/s/src/Essentials/src/Essentials.csproj (in 15.33 sec).
Restored /Users/cloudtest/vss/_work/1/s/src/Controls/src/Core/Controls.Core.csproj (in 16.08 sec).
Restored /Users/cloudtest/vss/_work/1/s/src/Controls/Foldable/src/Controls.Foldable.csproj (in 16.1 sec).
Restored /Users/cloudtest/vss/_work/1/s/src/Controls/src/Xaml/Controls.Xaml.csproj (in 16.11 sec).
Restored /Users/cloudtest/vss/_work/1/s/src/BlazorWebView/src/Maui/Microsoft.AspNetCore.Components.WebView.Maui.csproj (in 16.15 sec).
Restored /Users/cloudtest/vss/_work/1/s/src/Controls/tests/TestCases.HostApp/Controls.TestCases.HostApp.csproj (in 16.12 sec).
Restored /Users/cloudtest/vss/_work/1/s/src/Core/src/Core.csproj (in 16.09 sec).
Restored /Users/cloudtest/vss/_work/1/s/src/Controls/Maps/src/Controls.Maps.csproj (in 16.19 sec).
Restored /Users/cloudtest/vss/_work/1/s/src/Core/maps/src/Maps.csproj (in 16.19 sec).
/Users/cloudtest/vss/_work/1/s/.dotnet/packs/Microsoft.iOS.Sdk.net10.0_26.0/26.0.11017/targets/Xamarin.Shared.Sdk.targets(309,3): warning : RuntimeIdentifier was set on the command line, and will override the value for RuntimeIdentifiers set in the project file. [/Users/cloudtest/vss/_work/1/s/src/Controls/tests/TestCases.HostApp/Controls.TestCases.HostApp.csproj::TargetFramework=net10.0-ios]
##vso[build.updatebuildnumber]10.0.60-ci+azdo.13907292
Graphics -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Graphics/Debug/net10.0-ios26.0/Microsoft.Maui.Graphics.dll
##vso[build.updatebuildnumber]10.0.60-ci+azdo.13907292
Essentials -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Essentials/Debug/net10.0-ios26.0/Microsoft.Maui.Essentials.dll
##vso[build.updatebuildnumber]10.0.60-ci+azdo.13907292
Core -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Core/Debug/net10.0-ios26.0/Microsoft.Maui.dll
##vso[build.updatebuildnumber]10.0.60-ci+azdo.13907292
Controls.BindingSourceGen -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Controls.BindingSourceGen/Debug/netstandard2.0/Microsoft.Maui.Controls.BindingSourceGen.dll
Maps -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Maps/Debug/net10.0-ios26.0/Microsoft.Maui.Maps.dll
##vso[build.updatebuildnumber]10.0.60-ci+azdo.13907292
Controls.Core -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Controls.Core/Debug/net10.0-ios26.0/Microsoft.Maui.Controls.dll
##vso[build.updatebuildnumber]10.0.60-ci+azdo.13907292
##vso[build.updatebuildnumber]10.0.60-ci+azdo.13907292
##vso[build.updatebuildnumber]10.0.60-ci+azdo.13907292
##vso[build.updatebuildnumber]10.0.60-ci+azdo.13907292
Controls.Foldable -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Controls.Foldable/Debug/net10.0-ios26.0/Microsoft.Maui.Controls.Foldable.dll
Controls.Maps -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Controls.Maps/Debug/net10.0-ios26.0/Microsoft.Maui.Controls.Maps.dll
Microsoft.AspNetCore.Components.WebView.Maui -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Microsoft.AspNetCore.Components.WebView.Maui/Debug/net10.0-ios26.0/Microsoft.AspNetCore.Components.WebView.Maui.dll
Controls.Xaml -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Controls.Xaml/Debug/net10.0-ios26.0/Microsoft.Maui.Controls.Xaml.dll
Detected signing identity:
Code Signing Key: "" (-)
Provisioning Profile: "" () - no entitlements
Bundle Id: com.microsoft.maui.uitests
App Id: com.microsoft.maui.uitests
Controls.TestCases.HostApp -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Controls.TestCases.HostApp/Debug/net10.0-ios/iossimulator-arm64/Controls.TestCases.HostApp.dll
Optimizing assemblies for size may change the behavior of the app. Be sure to test after publishing. See: https://aka.ms/dotnet-illink
Optimizing assemblies for size. This process might take a while.
Build succeeded.
/Users/cloudtest/vss/_work/1/s/.dotnet/packs/Microsoft.iOS.Sdk.net10.0_26.0/26.0.11017/targets/Xamarin.Shared.Sdk.targets(309,3): warning : RuntimeIdentifier was set on the command line, and will override the value for RuntimeIdentifiers set in the project file. [/Users/cloudtest/vss/_work/1/s/src/Controls/tests/TestCases.HostApp/Controls.TestCases.HostApp.csproj::TargetFramework=net10.0-ios]
1 Warning(s)
0 Error(s)
Time Elapsed 00:01:39.84
Test Run 2: WITH Fix
Expected: Tests should PASS (bug is fixed)
Actual: Tests PASSED ✅
Test Summary:
- Total:
- Passed: True
- Failed:
- Skipped:
View full test output (with fix)
Determining projects to restore...
Restored /Users/cloudtest/vss/_work/1/s/src/Controls/src/BindingSourceGen/Controls.BindingSourceGen.csproj (in 338 ms).
Restored /Users/cloudtest/vss/_work/1/s/src/Graphics/src/Graphics/Graphics.csproj (in 349 ms).
Restored /Users/cloudtest/vss/_work/1/s/src/Essentials/src/Essentials.csproj (in 353 ms).
Restored /Users/cloudtest/vss/_work/1/s/src/Controls/src/Core/Controls.Core.csproj (in 388 ms).
Restored /Users/cloudtest/vss/_work/1/s/src/Core/src/Core.csproj (in 404 ms).
6 of 11 projects are up-to-date for restore.
/Users/cloudtest/vss/_work/1/s/.dotnet/packs/Microsoft.iOS.Sdk.net10.0_26.0/26.0.11017/targets/Xamarin.Shared.Sdk.targets(309,3): warning : RuntimeIdentifier was set on the command line, and will override the value for RuntimeIdentifiers set in the project file. [/Users/cloudtest/vss/_work/1/s/src/Controls/tests/TestCases.HostApp/Controls.TestCases.HostApp.csproj::TargetFramework=net10.0-ios]
##vso[build.updatebuildnumber]10.0.60-ci+azdo.13907292
Graphics -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Graphics/Debug/net10.0-ios26.0/Microsoft.Maui.Graphics.dll
##vso[build.updatebuildnumber]10.0.60-ci+azdo.13907292
Essentials -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Essentials/Debug/net10.0-ios26.0/Microsoft.Maui.Essentials.dll
##vso[build.updatebuildnumber]10.0.60-ci+azdo.13907292
Core -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Core/Debug/net10.0-ios26.0/Microsoft.Maui.dll
##vso[build.updatebuildnumber]10.0.60-ci+azdo.13907292
Maps -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Maps/Debug/net10.0-ios26.0/Microsoft.Maui.Maps.dll
Controls.BindingSourceGen -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Controls.BindingSourceGen/Debug/netstandard2.0/Microsoft.Maui.Controls.BindingSourceGen.dll
##vso[build.updatebuildnumber]10.0.60-ci+azdo.13907292
Controls.Core -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Controls.Core/Debug/net10.0-ios26.0/Microsoft.Maui.Controls.dll
##vso[build.updatebuildnumber]10.0.60-ci+azdo.13907292
##vso[build.updatebuildnumber]10.0.60-ci+azdo.13907292
##vso[build.updatebuildnumber]10.0.60-ci+azdo.13907292
##vso[build.updatebuildnumber]10.0.60-ci+azdo.13907292
Controls.Foldable -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Controls.Foldable/Debug/net10.0-ios26.0/Microsoft.Maui.Controls.Foldable.dll
Microsoft.AspNetCore.Components.WebView.Maui -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Microsoft.AspNetCore.Components.WebView.Maui/Debug/net10.0-ios26.0/Microsoft.AspNetCore.Components.WebView.Maui.dll
Controls.Maps -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Controls.Maps/Debug/net10.0-ios26.0/Microsoft.Maui.Controls.Maps.dll
Controls.Xaml -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Controls.Xaml/Debug/net10.0-ios26.0/Microsoft.Maui.Controls.Xaml.dll
Detected signing identity:
Code Signing Key: "" (-)
Provisioning Profile: "" () - no entitlements
Bundle Id: com.microsoft.maui.uitests
App Id: com.microsoft.maui.uitests
Controls.TestCases.HostApp -> /Users/cloudtest/vss/_work/1/s/artifacts/bin/Controls.TestCases.HostApp/Debug/net10.0-ios/iossimulator-arm64/Controls.TestCases.HostApp.dll
Optimizing assemblies for size may change the behavior of the app. Be sure to test after publishing. See: https://aka.ms/dotnet-illink
Optimizing assemblies for size. This process might take a while.
Build succeeded.
/Users/cloudtest/vss/_work/1/s/.dotnet/packs/Microsoft.iOS.Sdk.net10.0_26.0/26.0.11017/targets/Xamarin.Shared.Sdk.targets(309,3): warning : RuntimeIdentifier was set on the command line, and will override the value for RuntimeIdentifiers set in the project file. [/Users/cloudtest/vss/_work/1/s/src/Controls/tests/TestCases.HostApp/Controls.TestCases.HostApp.csproj::TargetFramework=net10.0-ios]
1 Warning(s)
0 Error(s)
Time Elapsed 00:00:44.70
Logs
- Full verification log:
/Users/cloudtest/vss/_work/1/s/CustomAgentLogsTmp/PRState/35009/PRAgent/gate/verify-tests-fail/verification-log.txt - Test output without fix:
/Users/cloudtest/vss/_work/1/s/CustomAgentLogsTmp/PRState/35009/PRAgent/gate/verify-tests-fail/test-without-fix.log - Test output with fix:
/Users/cloudtest/vss/_work/1/s/CustomAgentLogsTmp/PRState/35009/PRAgent/gate/verify-tests-fail/test-with-fix.log - UI test logs:
CustomAgentLogsTmp/UITests/
🔍 Pre-Flight — Context & Validation
Issue: #34892 - ContentPage with ToolbarItem Clicked event leaks when presented as modal page
PR: #35009 - [iOS] Fix ContentPage with ToolbarItem Clicked event leaks when presented as modal page
Platforms Affected: iOS (Android confirmed not reproducible)
Files Changed: 1 implementation, 2 test
PR Status: ✅ MERGED (2026-04-20)
Key Findings
- iOS modal pop does not explicitly dispose handlers, leaving
UIBarButtonItemwrappers rooted viaClicked/PropertyChangedevent subscriptions - Fix adds
DisposeHelpers.DisposeModalAndChildHandlers(modal)in both dismissal branches ofPopModalPlatformAsync DisposeModalAndChildHandlersiterates descendants, disconnects/disposes handlers, disposesControlsModalWrapper, and removes platform view from superview- New using directive added:
Microsoft.Maui.Controls.Handlers.Compatibility(coupling to compat layer exists already via ListViewRenderer pattern) - All 5 prior inline review comments from copilot-pull-request-reviewer were resolved before merge
- kubaflo (MEMBER) approved and merged the PR
Code Review Summary
Verdict: LGTM
Confidence: high
Errors: 0 | Warnings: 2 | Suggestions: 2
Key code review findings:
⚠️ Thread.Sleep(1000)in UI test (src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue34892.cs:26,36) — flaky timing anti-pattern on slow CI machines; should pollStatusLabeltext until changed⚠️ No[Platform(TestDevice.iOS)]attribute on test method — test runs on all platforms trivially (fix is iOS-only); intent should be explicit- 💡 Dead field
_idinIssue34892LeakPage(TestCases.HostApp/Issues/Issue34892.cs:82-87) — assigned but never read - 💡 Cross-layer dependency
Platform→Handlers.Compatibility(pre-existing pattern, no action needed)
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #35009 | Call DisposeHelpers.DisposeModalAndChildHandlers(modal) in both branches of PopModalPlatformAsync after dismiss |
✅ PASSED (Gate) | ModalNavigationManager.iOS.cs |
Original PR — MERGED |
🔬 Code Review — Deep Analysis
Code Review — PR #35009
Independent Assessment
What this changes: In ModalNavigationManager.iOS.cs, both return branches of PopModalPlatformAsync now call DisposeHelpers.DisposeModalAndChildHandlers(modal) before returning the modal page. A new using directive is added to bring in Microsoft.Maui.Controls.Handlers.Compatibility.
Inferred motivation: On iOS, popping a modal page was not triggering explicit handler disposal. Native objects (most likely UIBarButtonItem wrappers for ToolbarItem) were holding references to the managed page graph through event subscriptions, preventing GC collection. Explicitly disconnecting and disposing handlers at pop time breaks those reference chains.
Reconciliation with PR Narrative
Author claims: ToolbarItem's Clicked event creates a reference chain via native UIBarButtonItem wrappers that outlives the managed modal pop. Calling DisposeModalAndChildHandlers in both dismissal branches breaks the cycle. Android/Windows are unaffected (issue not reproducible there).
Agreement: Full agreement. The root cause analysis matches the code: DisposeModalAndChildHandlers iterates all descendants, calls DisconnectHandler() + Dispose() on each, and for the root view disposes the ControlsModalWrapper and removes from superview. This is the same pattern already used by ListViewRenderer.iOS.cs for header/footer cleanup.
Findings
⚠️ Warning — Thread.Sleep(1000) in UI test is a timing anti-pattern
File: src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue34892.cs, lines 26 and 36
Thread.Sleep(1000); // Allow some time for the GC to finalize objects
Assert.That(App.WaitForElement("StatusLabel").GetText(), Is.EqualTo("Still alive: 0"));The test taps ForceGCButton, which triggers an async operation (GarbageCollectionHelper.WaitForGC(2000, ...)) inside the app. Appium's Tap() returns after the gesture is recognized—not after the async void handler completes. The Thread.Sleep(1000) is the test's attempt to wait long enough for the 2-second GC loop to finish and update the label.
This is fragile on slow CI machines. The correct approach is to poll the StatusLabel text value until it changes from its empty/prior state, rather than sleeping a fixed amount.
Per the review rules (§13 Testing): "A Task.Delay(100) in a device test is a flaky test waiting to happen. Reviewers will ask: how do we know Xms is enough?"
⚠️ Warning — Test class is platform-annotated iOS-only but has no platform filtering in test execution
File: src/Controls/tests/TestCases.HostApp/Issues/Issue34892.cs, line 3
[Issue(IssueTracker.Github, 34892, "...", PlatformAffected.iOS)]PlatformAffected.iOS is documentation metadata—it does not prevent the test from running on Android or Windows. Since the fix is iOS-only, the test will trivially pass on other platforms (no leak exists there). A [Platform(TestDevice.iOS)] attribute on the test method would communicate intent explicitly and prevent a confusing false-positive run.
💡 Suggestion — Dead field _id in Issue34892LeakPage
File: src/Controls/tests/TestCases.HostApp/Issues/Issue34892.cs, lines 82–87
static int _instanceCount;
readonly int _id;
// ...
_id = ++_instanceCount;_id is assigned but never read again (not shown in UI, not part of any assertion). If the intent was to track which instance is alive for debugging, it should be surfaced in the label text. If not needed, it's dead code.
💡 Suggestion — Cross-layer namespace dependency worth noting
File: src/Controls/src/Core/Platform/ModalNavigationManager/ModalNavigationManager.iOS.cs, line 5
using Microsoft.Maui.Controls.Handlers.Compatibility;ModalNavigationManager is in the Platform layer; DisposeHelpers lives in the Handlers.Compatibility layer. Both are internal, so this is not a public API concern—but it does create a downward dependency from Platform → CompatibilityHandlers. The coupling already exists elsewhere (ListViewRenderer), so no action required—but flagging for awareness.
CI Status
Several CI jobs are failing (Build .NET MAUI Build Windows, Build .NET MAUI Build macOS, Run Helix Unit Tests Windows, Run Integration Tests Samples). However:
- The PR targets
inflight/current, notmain - The failures span build and unrelated integration jobs across Windows and macOS
- The 2-line iOS change cannot cause Windows/macOS build failures
These failures appear to be pre-existing on the inflight/current branch, not caused by this PR. The PR is already merged.
Devil's Advocate
On double-dispose: Could DisposeModalAndChildHandlers be called on a page whose handlers were already disconnected elsewhere? ClearModalPages only clears the list—it does not dispose handlers. There's no other disposal path for modal in PopModalPlatformAsync. No double-dispose risk found.
On animation timing: DismissViewControllerAsync is awaited before calling dispose. The comments in PresentModal note that iOS sometimes fires completion early; however, this concern is on the push side. For dismiss, awaiting the task gives a reasonable point-in-time to call dispose. The same pattern (dispose-after-await) is used throughout the compat renderers.
On correctness of disposing the NavigationPage wrapper: The test wraps with NavigationPage(page). DisposeModalAndChildHandlers starts from modal (the NavigationPage) and iterates Descendants(), which includes the inner ContentPage and its ToolbarItem. The dispose correctly propagates through the whole subtree.
Am I missing something about the fix not going far enough? On Android the issue doesn't reproduce (confirmed by author + reviewer). No other platform-specific pop paths were changed. iOS-only scope is justified.
Verdict: LGTM
Confidence: High
Summary: The core fix is correct, minimal, and consistent with the existing DisposeHelpers pattern used throughout the compatibility layer. Both dismissal branches now explicitly dispose modal page handlers, breaking the reference chains that prevented GC collection. The two warnings are about test robustness (Thread.Sleep fragility and missing platform scope attribute)—they don't affect correctness of the fix, and the PR is already merged. The dead _id field is minor cleanup.
🔧 Fix — Analysis & Comparison
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #35009 | Call DisposeHelpers.DisposeModalAndChildHandlers(modal) in both branches of PopModalPlatformAsync after dismiss |
✅ PASSED (Gate) | ModalNavigationManager.iOS.cs |
Original PR — MERGED. Uses existing DisposeHelpers pattern (ListViewRenderer precedent). 3 lines in 1 file. |
| 1 | try-fix | Weak ToolbarItem tracking: change List<ToolbarItem> _trackedToolbarItems to List<WeakReference<ToolbarItem>> in NavigationRenderer.ParentingViewController |
✅ PASS | NavigationRenderer.cs (+7/-4) |
Different root cause hypothesis: tracking collection keeps strong refs. Minimal structural fix. Consistent with _child/_target WeakRef pattern already in the codebase. |
| 2 | try-fix | Override ViewDidDisappear in ControlsModalWrapper to call DisconnectHandler() and clear _modal when IsBeingDismissed |
✅ PASS | ControlsModalWrapper.cs (+19) |
More robust: fires from native UIKit lifecycle, covers swipe-to-dismiss not handled by managed PopModalAsync path. |
| 3 | try-fix | Aggressive native UIBarButtonItem disposal in ParentingViewController.ViewDidDisappear |
❌ FAIL | NavigationRenderer.cs |
NRE crashes on iteration 1; pages still alive after fix. Disposal of native bar items alone insufficient — managed tracking list still roots the toolbar items. |
| 4 | try-fix | Targeted post-dismissal toolbar cleanup in NavigationRenderer called from ModalNavigationManager (narrower than full DisposeHelpers) |
✅ PASS | ModalNavigationManager.iOS.cs, NavigationRenderer.cs |
Similar to PR but scoped to toolbar-only — 2 files changed vs 1 for PR. Less minimal than PR. |
| 5 | try-fix | Pre-dismissal toolbar strip: clear managed tracking + native UIBarButtonItems BEFORE calling DismissViewControllerAsync |
✅ PASS | ModalNavigationManager.iOS.cs, NavigationRenderer.cs (+50/+51 lines) |
Most invasive. Different timing (pre vs post-dismiss). 100+ lines across 2 files — much larger than PR. |
Cross-Pollination
| Model | Round | New Ideas? | Details |
|---|---|---|---|
| claude-opus-4.6 | 2 | Yes | Pre-dismissal native toolbar strip (became Attempt 5) |
| claude-sonnet-4.6 | 2 | Yes | WeakEventManager at subscription site in ToolbarItemExtensions |
| gpt-5.3-codex | 2 | Yes | iOS-native dealloc hook (associated-object lifetime token) |
| gpt-5.4 | 2 | Yes | Clear modal page ToolbarItems parent ownership during teardown |
| claude-opus-4.6 | 3 | Yes | CancellationToken-scoped toolbar subscriptions |
Exhausted: Yes (Round 3 — max rounds reached, sufficient candidates found)
Selected Fix: PR's fix — Reason: Most minimal (3 lines in 1 file), uses established DisposeHelpers pattern already present in ListViewRenderer.iOS.cs, correctly places cleanup at the managed modal-pop boundary. Already MERGED and Gate-verified. Attempt 2 (ViewDidDisappear) has a theoretical advantage for swipe-to-dismiss coverage, but ModalNavigationManager already handles swipe dismissal via SyncPlatformModalStack/Resumed event reconciliation.
📋 Report — Final Recommendation
✅ Final Recommendation: APPROVE
Phase Status
| Phase | Status | Notes |
|---|---|---|
| Pre-Flight | ✅ COMPLETE | Issue #34892, iOS-only modal ToolbarItem leak |
| Code Review | LGTM (high) | 0 errors, 2 warnings, 2 suggestions |
| Gate | ✅ PASSED | iOS — tests FAIL without fix, PASS with fix |
| Try-Fix | ✅ COMPLETE | 5 attempts, 4 passing (1 fail) |
| Report | ✅ COMPLETE |
Code Review Impact on Try-Fix
Code review verdict was LGTM (0 errors) with two warnings about test robustness (Thread.Sleep(1000) fragility and missing [Platform(TestDevice.iOS)]). These findings were passed as advisory hints to all 4 models but appropriately did not redirect exploration — since the warnings concern tests, not the fix logic itself. All models correctly focused on alternative retention-chain break approaches (weak refs, lifecycle hooks, pre-dismissal cleanup, scoped subscriptions). No model found code-review errors to address because there were none.
Summary
PR #35009 fixes a real, verified iOS memory leak where ToolbarItem.Clicked event handlers were preventing garbage collection of modal pages. The fix is minimal (3 lines in one platform file), consistent with existing codebase patterns (DisposeHelpers is already used in ListViewRenderer.iOS.cs), and is correctly scoped to iOS only (Android/Windows unaffected). Human reviewer (kubaflo) approved and merged. Gate confirmed tests fail without fix and pass with fix.
Try-Fix exploration found 4 passing alternative approaches, confirming the problem is well-understood with multiple valid solutions. The PR's approach is the simplest and most aligned with existing codebase conventions.
Root Cause
On iOS, PopModalAsync removes the page from the managed modal stack but does not trigger explicit handler disposal. Native UIBarButtonItem wrappers (created by NavigationRenderer.ParentingViewController to represent ToolbarItem objects) subscribe to PropertyChanged on each ToolbarItem. These wrappers are held alive by UIKit's view controller hierarchy during and briefly after the dismissal animation. The strong ToolbarItem → event handler delegate → Page reference chain prevents GC collection of the page graph.
Fix Quality
The fix is correct and minimal. DisposeHelpers.DisposeModalAndChildHandlers already handles: iterating descendants, calling DisconnectHandler() + Dispose() on each, disposing the ControlsModalWrapper, and removing the platform view from superview. Placing this call in both dismissal branches of PopModalPlatformAsync correctly covers: (1) the normal path where PresentingViewController is non-null, and (2) the fallback path where it was already dismissed (e.g., window content swap). The fix does not risk double-dispose since ClearModalPages does not dispose handlers. The new using directive creates a cross-layer dependency (Platform → Handlers.Compatibility) that already exists elsewhere in the codebase.
Two minor test quality issues noted (non-blocking for merge): Thread.Sleep(1000) is fragile on slow CI, and PlatformAffected.iOS metadata does not prevent the test from running on other platforms. Both were flagged in the original Copilot PR review and marked resolved.
🧪 UI Tests — Category Detection & Results
Build #1391097 | ❌ failed | 460/467 passed (98.5%) | 6 failed
🎯 Detected categories: ToolbarItem — ran 7 of 143 matrix cells (skipped 136)
Results by Platform
| Platform | Passed | Failed | Total |
|---|---|---|---|
| ✅ Android (API 30) | 27 | 0 | 27 |
| ✅ Android (API 30) | 27 | 0 | 27 |
| ✅ iOS Mono (18.5) | 24 | 0 | 24 |
| ❌ iOS Mono (latest) | 17 | 6 | 24 |
| ✅ macOS | 23 | 0 | 23 |
| ✅ WinUI | 23 | 0 | 23 |
| ✅ Material3 Android (API 36) | 319 | 0 | 319 |
Failures (6)
🖼️ snapshot: 6
❌ iOS Mono (latest) — 6 failed, 17/24 passed (71%)
| Test | Detail | |
|---|---|---|
| 🖼️ | ToolbarItemCorrectSizeTest |
0.56% diff |
| 🖼️ | ToolbarItemFontColorDynamicUpdate |
0.56% diff |
| 🖼️ | ToolbarItemFontIconSourceChangesAtRunTime |
0.62% diff |
| 🖼️ | ToolbarTextColorOnInteraction |
0.60% diff |
| 🖼️ | UpdateToolbarItemAfterNavigate |
0.60% diff |
| 🖼️ | VerifyToolbarItemIconColor |
0.62% diff |
🔴 Failed stages (1) of 13 total
- ❌ iOS UITests Mono
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 whether this change resolves your issue. Thank you!
Issue Details
When a page with a ToolbarItem + Clicked handler was pushed as a modal wrapped in NavigationPage, then popped via PopModalAsync(), the page was not garbage collected.
Root Cause
iOS maps ToolbarItems → native UIBarButtonItem wrappers at push time. Those wrappers subscribe to Clicked/PropertyChanged on the ToolbarItem. On modal pop, iOS defers the native dismiss animation, so the UIBarButtonItem objects briefly outlive the managed page. That keeps the ToolbarItem event handler references rooted, preventing the page from being collected.
Description of Change
Added handler disposal in both dismissal branches:
Note : The user mentioned that this issue was reproduced on Android, but it is not reproducible in the latest source. It seems to be resolved on the Android platform. So fix added only for iOS.
Issues Fixed
Fixes #34892
Tested the behavior in the following platforms.
Before.mov
After.mov