Enable Issue33352 UITests

.github/agent-pr-session/issue-33352.md

@@ -0,0 +1,136 @@
+# Issue #33352 - Fix Exploration Session
+
+**Issue:** Intermittent crash on exit on MacCatalyst - ObjectDisposedException in ShellSectionRootRenderer
+**Platform:** MacCatalyst
+**Test Filter:** Issue33352
+**Bug:** `TraitCollectionDidChange` is called on disposed ShellSectionRootRenderer after window scope disposed
+
+## Reproduction
+
+✅ **100% reproducible** with test: `TraitCollectionDidChangeAfterDisposeDoesNotCrash`
+
+**Error:** `ObjectDisposedException: Cannot access a disposed object. Object name: 'IServiceProvider'.`
+
+**Root Cause:** 
+- Window is closed, which disposes the service provider scope
+- iOS/MacCatalyst then calls `TraitCollectionDidChange` on the ShellSectionRootRenderer
+- The override tries to access `Context.Services.GetService<IApplication>()` 
+- The service provider has been disposed, throwing ObjectDisposedException
+
+**Affected Code:**
+- `src/Controls/src/Core/Platform/iOS/ShellSectionRootRenderer.cs` - TraitCollectionDidChange override
+
+## Fix Candidates
+
+| # | Source | Approach | Test Result | Files Changed | Model | Notes |
+|---|--------|----------|-------------|---------------|-------|-------|
+| 1 | try-fix | Check `_isDisposed` before accessing services | ❌ FAIL | `ShellSectionRootRenderer.cs` (+5) | Claude Sonnet 4.5 | **Why failed:** `_isDisposed` flag not set when TraitCollectionDidChange called. Window scope disposed first, then iOS calls TraitCollectionDidChange, but renderer's Dispose() hasn't been called yet. The ServiceProvider is disposed before the renderer knows it's disposed. |
+| 2 | try-fix | Store `IApplication` in `WeakReference` | ✅ PASS | `ShellSectionRootRenderer.cs` (+13) | Claude Sonnet 4.5 | **Why works:** Captures IApplication at construction time before service provider disposed. WeakReference allows GC to collect if needed. No ObjectDisposedException because we never touch the disposed service provider. |
+| 3 | try-fix | Use `Application.Current` instead of service provider | ✅ PASS | `ShellSectionRootRenderer.cs` (+12) | Claude Sonnet 4.5 | **Why works:** `Application.Current` is a static property that doesn't go through the scoped service provider. Cast to `IApplication` to call `ThemeChanged()`. Simpler than WeakReference approach. **PREFERRED - simplest solution.** |
+| 4 | try-fix | Try-catch `ObjectDisposedException` | ✅ PASS | `ShellSectionRootRenderer.cs` (+14) | Claude Sonnet 4.5 | **Why works:** Catches and swallows ObjectDisposedException. Theme change not critical during disposal. Works but less elegant than avoiding the exception. |
+| 5 | try-fix | Remove override entirely | ❌ N/A | - | Claude Sonnet 4.5 | **Not applicable:** Method is in PublicAPI.Shipped.txt, removing it would be breaking change. Would require API removal process. |
+| 6 | try-fix | Null-check `FindMauiContext()` + try-catch | ✅ PASS | `ShellSectionRootRenderer.cs` (+19) | Claude Sonnet 4.5 | **Why works:** Double protection - null check first, then try-catch. More defensive but verbose. |
+| 7 | try-fix | Check if Shell's Window is null | ❌ FAIL | `ShellSectionRootRenderer.cs` (+15) | Claude Sonnet 4.5 | **Why failed:** Window property is still set when TraitCollectionDidChange called. Window.Parent disconnection happens after TraitCollectionDidChange, so checking Window is null doesn't help. |
+| 8 | try-fix | Check if Window.Handler is null | ✅ PASS | `ShellSectionRootRenderer.cs` (+16) | Claude Sonnet 4.5 | **Why works:** Window.Handler is disconnected before service provider disposed. Checking `window?.Handler == null` catches the disconnection state. Good approach for detecting window closure. |
+| 9 | try-fix | Check if Shell.Parent is null | ❌ FAIL | `ShellSectionRootRenderer.cs` (+15) | Claude Sonnet 4.5 | **Why failed:** Shell.Parent (Window) still set when TraitCollectionDidChange called. Shell remains attached to Window during disposal sequence. |
+| 10 | try-fix | Combine `Application.Current` with Window.Handler check | ✅ PASS | `ShellSectionRootRenderer.cs` (+21) | Claude Sonnet 4.5 | **Why works:** Best of both: Window.Handler check catches disconnection early, Application.Current avoids service provider entirely. Most defensive approach. |
+| 11 | try-fix | Check `Window.IsDestroyed` (internal flag) | ✅ PASS | `ShellSectionRootRenderer.cs` (+16) | Claude Sonnet 4.5 | **Why works:** `IsDestroyed` is set to true at line 540 of Window.Destroying(), BEFORE DisposeWindowScope() at line 558. Perfect timing! Checks the exact state user suggested. **EXCELLENT window-based solution.** |
+
+**Exhausted:** Yes (11 attempts completed)
+**Selected Fix:** #3 - Use `Application.Current` - **Simplest** OR #11 - Check `Window.IsDestroyed` - **Most semantically correct**
+
+## Summary
+
+**Passing fixes (7 total):**
+- ✅ #2: WeakReference<IApplication>
+- ✅ #3: Application.Current (**SIMPLEST**)
+- ✅ #4: Try-catch ObjectDisposedException  
+- ✅ #6: Null-check + try-catch
+- ✅ #8: Check Window.Handler is null
+- ✅ #10: Application.Current + Window.Handler check
+- ✅ #11: Check Window.IsDestroyed (**SEMANTICALLY BEST - checks exact destroying state**)
+
+**Failed fixes (3 total):**
+- ❌ #1: Check _isDisposed (flag not set yet)
+- ❌ #7: Check Shell.Window is null (still set)
+- ❌ #9: Check Shell.Parent is null (still set)
+
+**Not applicable (1 total):**
+- ❌ #5: Remove override (breaking change)
+
+## Recommendation
+
+**Two best options:**
+
+1. **#3 - Application.Current** (simplest, 12 lines)
+   - Pros: Minimal code, no state tracking, works everywhere
+   - Cons: Doesn't check if window is actually closing
+
+2. **#11 - Window.IsDestroyed** (semantically correct, 16 lines) 
+   - Pros: Checks the EXACT state that causes the bug, clear intent
+   - Cons: Slightly more code, relies on internal property (same assembly)
+
+User's suggestion of checking window destroying state was spot-on!
+
+---
+
+## ACTUAL IMPLEMENTED FIX
+
+**Selected Fix:** Architectural improvement - Remove duplication + strengthen Core layer
+
+**What was implemented:**
+
+1. **REMOVED** `TraitCollectionDidChange` override from `ShellSectionRootRenderer` (Controls layer)
+   - Lines 144-151 deleted
+   - This was duplicate code that didn't belong in Controls
+
+2. **ENHANCED** `TraitCollectionDidChange` in `PageViewController` (Core layer)
+   - Added `window?.Handler == null` check (like attempt #8)
+   - Added try-catch safety net (like attempt #4)
+   - Uses `GetRequiredService` instead of `FindMauiContext`
+   - Combined approach: Window.Handler check + try-catch for race conditions
+
+**Why this wasn't discovered by try-fix:**
+
+1. **Tunnel vision** - Only looked at ShellSectionRootRenderer (where error appeared)
+2. **Didn't search codebase** - Never found PageViewController also had TraitCollectionDidChange
+3. **Didn't recognize duplication** - Both Core and Controls had the override
+4. **Missed layer architecture** - Theme changes are CORE functionality, not Shell-specific
+
+**Key insight:**
+
+The bug existed because theme handling was **duplicated** across layers:
+- Core (PageViewController) - Fundamental, applies to ALL pages
+- Controls (ShellSectionRootRenderer) - Shell-specific override
+
+The proper fix was to **remove the Controls override** and **strengthen the Core implementation**, not patch the Controls one.
+
+**Files changed:**
+- `src/Controls/src/Core/Compatibility/Handlers/Shell/iOS/ShellSectionRootRenderer.cs` (-10 lines)
+- `src/Core/src/Platform/iOS/PageViewController.cs` (+29 lines)
+- PublicAPI.Unshipped.txt (iOS/MacCatalyst) - document removal
+
+**Test verification:**
+✅ TraitCollectionDidChangeAfterDisposeDoesNotCrash passes with new implementation
+
+## Test Command
+
+```bash
+pwsh .github/scripts/BuildAndRunHostApp.ps1 -Platform catalyst -TestFilter "FullyQualifiedName~TraitCollectionDidChangeAfterDisposeDoesNotCrash"
+```
+
+## Lessons Learned
+
+**What would have helped discover this fix:**
+
+1. **Codebase-wide search** - `grep -r "TraitCollectionDidChange" src/` would have found both locations
+2. **Layer analysis** - Ask "Does this belong in Core or Controls?"
+3. **Duplication detection** - Recognize when the same override exists in multiple layers
+4. **Remove vs patch** - Consider whether code should exist at all, not just how to fix it
+
+**Repository improvements needed:**
+
+1. Architecture documentation explaining Core vs Controls layer responsibility
+2. Try-fix skill enhancement to search for duplicate implementations
+3. Inline comments in key classes about layer responsibilities
+4. Linting rule to detect duplicate iOS/Android method overrides across layers

.github/agent-pr-session/pr-32289.md

@@ -0,0 +1,202 @@
+# PR Review: #32289 - Fix handler not disconnected when removing non visible pages using RemovePage()
+
+**Date:** 2026-01-07 | **Issue:** [#32239](https://github.com/dotnet/maui/issues/32239) | **PR:** [#32289](https://github.com/dotnet/maui/pull/32289)
+
+## ✅ Status: COMPLETE
+
+| Phase | Status |
+|-------|--------|
+| Pre-Flight | ✅ COMPLETE |
+| 🧪 Tests | ✅ COMPLETE |
+| 🚦 Gate | ✅ PASSED |
+| 🔧 Fix | ✅ COMPLETE |
+| 📋 Report | ✅ COMPLETE |
+
+---
+
+<details>
+<summary><strong>📋 Issue Summary</strong></summary>
+
+**Problem:** When removing pages from a NavigationPage's navigation stack using `NavigationPage.Navigation.RemovePage()`, handlers are not properly disconnected from the removed pages. However, using `ContentPage.Navigation.RemovePage()` correctly disconnects handlers.
+
+**Root Cause (from PR):** The `RemovePage()` method removes the page from the navigation stack but does not explicitly disconnect its handler.
+
+**Regression:** Introduced in PR #24887, reproducible from MAUI 9.0.40+
+
+**Steps to Reproduce:**
+1. Push multiple pages onto a NavigationPage stack
+2. Call `NavigationPage.Navigation.RemovePage()` on a non-visible page
+3. Observe that the page's handler remains connected (no cleanup)
+
+**Workaround:** Manually call `.DisconnectHandlers()` after removing the page
+
+**Platforms Affected:**
+- [x] iOS
+- [x] Android
+- [x] Windows
+- [x] MacCatalyst
+
+</details>
+
+<details>
+<summary><strong>📁 Files Changed</strong></summary>
+
+| File | Type | Changes |
+|------|------|---------|
+| `src/Controls/src/Core/NavigationPage/NavigationPage.cs` | Fix | +4 lines |
+| `src/Controls/src/Core/NavigationPage/NavigationPage.Legacy.cs` | Fix | +4 lines |
+| `src/Controls/src/Core/NavigationProxy.cs` | Fix | -1 line (removed duplicate) |
+| `src/Controls/src/Core/Shell/ShellSection.cs` | Fix | +1 line |
+| `src/Controls/tests/Core.UnitTests/NavigationUnitTest.cs` | Unit Test | +63 lines |
+| `src/Controls/tests/Core.UnitTests/ShellNavigatingTests.cs` | Unit Test | +25 lines |
+
+**Test Type:** Unit Tests
+
+</details>
+
+<details>
+<summary><strong>💬 PR Discussion Summary</strong></summary>
+
+**Key Comments:**
+- Copilot flagged potential duplicate disconnection logic between NavigationProxy and NavigationPage
+- Author responded by removing redundant logic from NavigationProxy and updating ShellSection
+- StephaneDelcroix requested unit tests → Author added them
+- rmarinho confirmed unit tests cover both `useMaui: true` and `useMaui: false` scenarios
+
+**Reviewer Feedback:**
+- Comments about misleading code comments (fixed)
+- Concern about duplicate `DisconnectHandlers()` calls (resolved by moving from NavigationProxy to implementations)
+- StephaneDelcroix: Approved after unit tests added
+- rmarinho: Approved - confirmed tests cover NavigationPage and Shell scenarios
+
+**Maintainer Approvals:**
+- ✅ StephaneDelcroix (Jan 7, 2026)
+- ✅ rmarinho (Jan 7, 2026)
+
+**Disagreements to Investigate:**
+| File:Line | Reviewer Says | Author Says | Status |
+|-----------|---------------|-------------|--------|
+| NavigationPage.cs:914 | Duplicate disconnection with NavigationProxy | Removed from NavigationProxy, now only in NavigationPage | ✅ RESOLVED |
+| NavigationPage.Legacy.cs:257 | Same duplicate concern | Same resolution | ✅ RESOLVED |
+
+**Author Uncertainty:**
+- None noted
+
+</details>
+
+<details>
+<summary><strong>🧪 Tests</strong></summary>
+
+**Status**: ✅ COMPLETE
+
+- [x] PR includes unit tests
+- [x] Tests follow naming convention
+- [x] Unit tests cover both useMaui: true/false paths
+- [x] Unit tests cover Shell navigation
+
+**Test Files (from PR - Unit Tests):**
+- `src/Controls/tests/Core.UnitTests/NavigationUnitTest.cs` (+63 lines)
+  - `RemovePageDisconnectsHandlerForNonVisiblePage` - Tests removing middle page from 3-page stack (both useMaui: true/false)
+  - `RemovePageDisconnectsHandlerForRemovedRootPage` - Tests removing root page when another page is on top
+- `src/Controls/tests/Core.UnitTests/ShellNavigatingTests.cs` (+25 lines)
+  - `RemovePageDisconnectsHandlerInShell` - Tests Shell navigation scenario
+
+**Unit Test Coverage Analysis:**
+| Code Path | useMaui: true | useMaui: false | Shell |
+|-----------|---------------|----------------|-------|
+| Remove middle page | ✅ | ✅ | ✅ |
+| Remove root page | ✅ | ✅ | - |
+
+Coverage is adequate - tests cover all modified code paths.
+
+</details>
+
+<details>
+<summary><strong>🚦 Gate - Test Verification</strong></summary>
+
+**Status**: ✅ PASSED
+
+- [x] Tests FAIL without fix (bug reproduced)
+- [x] Tests PASS with fix
+
+**Result:** PASSED ✅
+
+**Verification:** Unit tests from PR cover all code paths:
+- `RemovePageDisconnectsHandlerForNonVisiblePage(true)` - Maui path (Android/Windows)
+- `RemovePageDisconnectsHandlerForNonVisiblePage(false)` - Legacy path (iOS/MacCatalyst)
+- `RemovePageDisconnectsHandlerForRemovedRootPage(true/false)` - Root page removal
+- `RemovePageDisconnectsHandlerInShell` - Shell navigation
+
+</details>
+
+<details>
+<summary><strong>🔧 Fix Candidates</strong></summary>
+
+**Status**: ✅ COMPLETE
+
+| # | Source | Approach | Test Result | Files Changed | Notes |
+|---|--------|----------|-------------|---------------|-------|
+| 1 | try-fix | Add DisconnectHandlers() inside SendHandlerUpdateAsync callback | ❌ FAIL | `NavigationPage.cs` (+3) | **Why failed:** Timing issue - SendHandlerUpdateAsync uses FireAndForget(), so the callback with DisconnectHandlers() runs asynchronously. The test checks Handler immediately after RemovePage() returns, before the async callback executes. |
+| 2 | try-fix | Add DisconnectHandlers() synchronously after SendHandlerUpdateAsync in MauiNavigationImpl | ❌ FAIL | `NavigationPage.cs` (+3) | **Why failed:** iOS uses `UseMauiHandler = false`, meaning it uses NavigationImpl (Legacy) NOT MauiNavigationImpl. My fix was in the wrong code path - iOS doesn't execute MauiNavigationImpl at all. |
+| 3 | try-fix | Add DisconnectHandlers() at end of Legacy RemovePage method | ✅ PASS | `NavigationPage.Legacy.cs` (+3) | Works! iOS/MacCatalyst use Legacy path. Simpler fix - only 1 file needed for iOS. |
+| 4 | try-fix | Approach 2+3 combined (both Maui and Legacy paths) | ✅ PASS (iOS) | `NavigationPage.cs`, `NavigationPage.Legacy.cs` (+6 total) | Works for NavigationPage on all platforms, BUT **misses Shell navigation** which has its own code path. |
+| PR | PR #32289 | Add `DisconnectHandlers()` call in RemovePage for non-visible pages | ✅ PASS (Gate) | NavigationPage.cs, NavigationPage.Legacy.cs, NavigationProxy.cs, ShellSection.cs | Original PR - validated by Gate |
+
+**Note:** try-fix candidates (1, 2, 3...) are added during Phase 4. PR's fix is reference only.
+
+**Exhausted:** No (stopped after finding working alternative)
+**Selected Fix:** PR's fix
+
+**Deep Analysis (Git History Research):**
+
+**Historical Timeline:**
+1. **PR #24887** (Feb 2025): Fixed Android flickering by avoiding handler removal during PopAsync - inadvertently broke RemovePage scenarios
+2. **PR #30049** (June 2025): Attempted fix by adding `page?.DisconnectHandlers()` to `NavigationProxy.OnRemovePage()` - **BUT THIS FIX WAS FUNDAMENTALLY FLAWED**
+3. **PR #32289** (Current): Correctly fixes by adding DisconnectHandlers to the NavigationPage implementations
+
+**Why PR #30049's fix didn't work:**
+- `MauiNavigationImpl` and `NavigationImpl` **override** `OnRemovePage()` 
+- The overrides do NOT call `base.OnRemovePage()`
+- Therefore `NavigationProxy.OnRemovePage()` is **NEVER executed** for NavigationPage!
+- ContentPage works because it doesn't override - uses the base NavigationProxy directly
+
+**Why calling base.OnRemovePage() won't work:**
+- `MauiNavigationImpl.OnRemovePage()` is a **complete replacement** with its own validation, async flow, etc.
+- Calling base would cause double removal and ordering issues
+
+**Conclusion:** The fix MUST be in the NavigationPage implementations themselves, not in NavigationProxy. PR #32289's approach is architecturally correct.
+
+**Comparison:**
+- **My fix #3** works for iOS/MacCatalyst (Legacy path) - 1 file, 3 lines
+- **PR's fix** works for ALL platforms (Legacy + Maui paths) - 3 files, ~10 lines
+- **PR #30049's approach** ❌ Doesn't work - fix in NavigationProxy is bypassed by overrides
+
+**Rationale for selecting PR's fix:**
+1. PR covers ALL platforms (iOS, MacCatalyst, Android, Windows) while my fix only covers iOS/MacCatalyst
+2. PR also fixes ShellSection for Shell navigation scenarios  
+3. PR uses null-safety (`page?`) which is more defensive
+4. PR correctly removes the ineffective DisconnectHandlers from NavigationProxy (cleanup)
+5. My successful fix #3 is essentially a subset of the PR's approach
+
+**Independent validation:** My fix #3 independently arrived at the same solution as the PR for the Legacy path, which validates the PR's approach is correct.
+
+</details>
+
+---
+
+## ✅ Final Recommendation: APPROVE
+
+**Summary:** PR #32289 correctly fixes the handler disconnection issue when removing non-visible pages using `RemovePage()`.
+
+**Key Findings:**
+1. ✅ **Root cause correctly identified** - NavigationPage overrides bypass NavigationProxy, requiring fix in implementations
+2. ✅ **All code paths covered** - NavigationPage (Maui + Legacy) and ShellSection
+3. ✅ **Unit tests adequate** - Cover both `useMaui: true/false` and Shell navigation
+4. ✅ **Two maintainer approvals** - StephaneDelcroix and rmarinho
+5. ✅ **Independent validation** - My try-fix #3 independently arrived at same solution for Legacy path
+
+**Alternative approaches tested:**
+- Approach 2+3 (Maui + Legacy paths only) works but misses Shell navigation
+- PR's fix is more complete and architecturally correct
+
+**No concerns identified.**