Skip to content

[iOS 26] Fix null reference and cast safety in ShellSectionRenderer navigation #7

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kubaflo merged 4 commits into from
Nov 15, 2025
Merged

[iOS 26] Fix null reference and cast safety in ShellSectionRenderer navigation #7

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a8bd9a2
Initial plan
Copilot Nov 14, 2025
5b4d000
Add null checks in ShellSectionRenderer
kubaflo Nov 8, 2025
c487e36
Fix inconsistent cast in ElementForViewController method
Copilot Nov 14, 2025
9e20d3d
Complete PR #32456 review with detailed analysis
Copilot Nov 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
131 changes: 131 additions & 0 deletions PR_32456_REVIEW_SUMMARY.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
# PR #32456 Review Summary

## Overview
**PR Title:** [iOS 26] Navigation hangs after rapidly open and closing new page using Navigation.PushAsync - fix
**PR Link:** https://github.com/dotnet/maui/pull/32456
**Issue:** https://github.com/dotnet/maui/issues/32425
**Author:** @kubaflo
**Reviewer:** GitHub Copilot

## Problem Statement

Navigation hangs and shows a blank white screen when rapidly opening and closing pages using `Navigation.PushAsync` on iOS 26 with Xcode 26 builds. This is a regression specific to iOS 26.

## Original PR Changes

The PR made two key changes to `src/Controls/src/Core/Compatibility/Handlers/Shell/iOS/ShellSectionRenderer.cs`:

### Change 1: Line 127 - Add null check for pages[targetIndex]
```diff
- if (targetIndex < 0 || targetIndex >= pages.Count)
+ if (targetIndex < 0 || targetIndex >= pages.Count || pages[targetIndex] is null)
```

### Change 2: Line 583 - Add null-conditional operator for renderer.ViewController
```diff
- if (viewController == renderer.ViewController)
+ if (viewController == renderer?.ViewController)
```

## Review Analysis

### Copilot Review Comment #1: Null Check on Line 127

**Copilot's Suggestion:** Remove `|| pages[targetIndex] is null` as redundant

**Analysis:** ❌ **INCORRECT**

**Reasoning:**
- Investigation of `ShellSection.cs` revealed that `_navStack` is initialized with a null entry:
```csharp
List<Page> _navStack = new List<Page> { null }; // Line 251
```
- The stack can be reset to contain null during lifecycle:
```csharp
_navStack = new List<Page> { null }; // Line 174
```
- Therefore, null pages **CAN** exist in the Stack collection
- The null check is **NECESSARY** to prevent null reference exceptions when calling `SyncStackDownTo(pages[targetIndex])`

**Recommendation:** ✅ **KEEP the null check as-is from the PR**

### Copilot Review Comment #2: Inconsistent Cast on Line 582-583

**Copilot's Suggestion:** Change hard cast to safe cast

**Analysis:** ✅ **CORRECT**

**Original Code:**
```csharp
var renderer = (IPlatformViewHandler)child.Handler; // Line 582 - hard cast
if (viewController == renderer?.ViewController) // Line 583 - null-conditional
```

**Problem:**
- If `child.Handler` is null, the hard cast on line 582 will throw `InvalidCastException`
- The null-conditional operator on line 583 never gets a chance to execute
- This is inconsistent with the pattern used elsewhere in the file (see `OnRemoveRequested` at line 517-518)

**Fix Applied:**
```csharp
var renderer = child.Handler as IPlatformViewHandler; // Safe cast
if (viewController == renderer?.ViewController) // Null-safe check
```

**Recommendation:** ✅ **Apply the fix** (changed from hard cast to `as` cast)

## Changes Made During Review

1. ✅ Validated that the null check on line 127 is necessary (kept as-is from PR)
2. ✅ Fixed inconsistent cast on line 582 (changed from `(IPlatformViewHandler)` to `as IPlatformViewHandler`)
3. ✅ Ran code formatting - no issues found
4. ✅ Committed both the original PR changes and the review fix

## Final Recommendation

**APPROVE with modifications:**

The PR correctly addresses the iOS 26 navigation hang issue with appropriate null checks. The review identified one valid improvement (line 582 cast consistency) which has been applied. The first review comment was incorrect and the original PR code should be kept.

### Summary of All Changes:
```diff
diff --git a/src/Controls/src/Core/Compatibility/Handlers/Shell/iOS/ShellSectionRenderer.cs
index 90c0c7fc0e..77cc4f3465 100644
--- a/src/Controls/src/Core/Compatibility/Handlers/Shell/iOS/ShellSectionRenderer.cs
+++ b/src/Controls/src/Core/Compatibility/Handlers/Shell/iOS/ShellSectionRenderer.cs
@@ -124,7 +124,7 @@ namespace Microsoft.Maui.Controls.Platform.Compatibility

// Bounds check: ensure we have a valid index for pages array
int targetIndex = NavigationBar.Items.Length - 1;
- if (targetIndex < 0 || targetIndex >= pages.Count)
+ if (targetIndex < 0 || targetIndex >= pages.Count || pages[targetIndex] is null)
return true;

_shellSection.SyncStackDownTo(pages[targetIndex]);
@@ -579,8 +579,8 @@ namespace Microsoft.Maui.Controls.Platform.Compatibility
{
if (child == null)
continue;
- var renderer = (IPlatformViewHandler)child.Handler;
- if (viewController == renderer.ViewController)
+ var renderer = child.Handler as IPlatformViewHandler;
+ if (viewController == renderer?.ViewController)
return child;
}
```

### Why These Changes Are Correct:

1. **Line 127:** Prevents null reference exception when accessing a page that might be null in the navigation stack during rapid push/pop operations
2. **Line 582:** Prevents InvalidCastException when Handler is null, allowing the null-conditional operator on line 583 to work properly
3. **Line 583:** (Original PR) Prevents null reference exception when accessing ViewController on a null renderer

All three changes work together to handle null scenarios that can occur during rapid navigation operations on iOS 26.

## Testing Recommendation

The PR should be tested with:
- Rapid push/pop navigation on iOS 26 simulator
- Rapid push/pop navigation on iOS 26 physical device
- Verify no regression on iOS 18 and earlier versions
- Test with the reproduction steps provided in issue #32425
6 changes: 3 additions & 3 deletions src/Controls/src/Core/Compatibility/Handlers/Shell/iOS/ShellSectionRenderer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ bool DidPopItem(UINavigationBar _, UINavigationItem __)

// Bounds check: ensure we have a valid index for pages array
int targetIndex = NavigationBar.Items.Length - 1;
if (targetIndex < 0 || targetIndex >= pages.Count)
if (targetIndex < 0 || targetIndex >= pages.Count || pages[targetIndex] is null)
return true;

_shellSection.SyncStackDownTo(pages[targetIndex]);
Expand Down Expand Up @@ -579,8 +579,8 @@ Element ElementForViewController(UIViewController viewController)
{
if (child == null)
continue;
var renderer = (IPlatformViewHandler)child.Handler;
if (viewController == renderer.ViewController)
var renderer = child.Handler as IPlatformViewHandler;
if (viewController == renderer?.ViewController)
return child;
}

Expand Down