February 2nd, Candidate

.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