Conversation
Added additional null checks for page and renderer references in ShellSectionRenderer to prevent potential null reference exceptions during navigation and view controller handling.
Co-authored-by: kubaflo <42434498+kubaflo@users.noreply.github.com>
PR dotnet#32456 Review SummaryOverviewPR Title: [iOS 26] Navigation hangs after rapidly open and closing new page using Navigation.PushAsync - fix Problem StatementNavigation hangs and shows a blank white screen when rapidly opening and closing pages using Original PR ChangesThe PR made two key changes to Change 1: Line 127 - Add null check for pages[targetIndex]- 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- if (viewController == renderer.ViewController)
+ if (viewController == renderer?.ViewController)Review AnalysisCopilot Review Comment #1: Null Check on Line 127Copilot's Suggestion: Remove Analysis: ❌ INCORRECT Reasoning:
Recommendation: ✅ KEEP the null check as-is from the PR Copilot Review Comment #2: Inconsistent Cast on Line 582-583Copilot's Suggestion: Change hard cast to safe cast Analysis: ✅ CORRECT Original Code: var renderer = (IPlatformViewHandler)child.Handler; // Line 582 - hard cast
if (viewController == renderer?.ViewController) // Line 583 - null-conditionalProblem:
Fix Applied: var renderer = child.Handler as IPlatformViewHandler; // Safe cast
if (viewController == renderer?.ViewController) // Null-safe checkRecommendation: ✅ Apply the fix (changed from hard cast to Changes Made During Review
Final RecommendationAPPROVE 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 --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:
All three changes work together to handle null scenarios that can occur during rapid navigation operations on iOS 26. Testing RecommendationThe PR should be tested with:
|
) * [iOS 26] Fix null reference and cast safety in ShellSectionRenderer navigation (#7) * Initial plan * Add null checks in ShellSectionRenderer Added additional null checks for page and renderer references in ShellSectionRenderer to prevent potential null reference exceptions during navigation and view controller handling. * Fix inconsistent cast in ElementForViewController method * Complete PR dotnet#32456 review with detailed analysis Co-authored-by: kubaflo <42434498+kubaflo@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Jakub Florkowski <42434498+kubaflo@users.noreply.github.com> * Initial plan * Applied PR dotnet#32648 changes for review Co-authored-by: kubaflo <42434498+kubaflo@users.noreply.github.com> * Add UI test case for issue dotnet#32616 Co-authored-by: kubaflo <42434498+kubaflo@users.noreply.github.com> * Final review summary for PR dotnet#32648 Co-authored-by: kubaflo <42434498+kubaflo@users.noreply.github.com> --------- Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> Co-authored-by: Jakub Florkowski <42434498+kubaflo@users.noreply.github.com>
* [iOS 26] Fix null reference and cast safety in ShellSectionRenderer navigation (#7) * Initial plan * Add null checks in ShellSectionRenderer Added additional null checks for page and renderer references in ShellSectionRenderer to prevent potential null reference exceptions during navigation and view controller handling. * Fix inconsistent cast in ElementForViewController method * Complete PR dotnet#32456 review with detailed analysis Co-authored-by: kubaflo <42434498+kubaflo@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Jakub Florkowski <42434498+kubaflo@users.noreply.github.com> * Initial plan * Applied PR dotnet#32648 changes for review Co-authored-by: kubaflo <42434498+kubaflo@users.noreply.github.com> * Add UI test case for issue dotnet#32616 Co-authored-by: kubaflo <42434498+kubaflo@users.noreply.github.com> * Final review summary for PR dotnet#32648 Co-authored-by: kubaflo <42434498+kubaflo@users.noreply.github.com> --------- Co-Authored-By: Copilot <198982749+Copilot@users.noreply.github.com> Co-Authored-By: Jakub Florkowski <42434498+kubaflo@users.noreply.github.com>
* [iOS 26] Fix null reference and cast safety in ShellSectionRenderer navigation (#7) * Initial plan * Add null checks in ShellSectionRenderer Added additional null checks for page and renderer references in ShellSectionRenderer to prevent potential null reference exceptions during navigation and view controller handling. * Fix inconsistent cast in ElementForViewController method * Complete PR dotnet#32456 review with detailed analysis Co-authored-by: kubaflo <42434498+kubaflo@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Jakub Florkowski <42434498+kubaflo@users.noreply.github.com> * Initial plan * Applied PR dotnet#32648 changes for review Co-authored-by: kubaflo <42434498+kubaflo@users.noreply.github.com> * Add UI test case for issue dotnet#32616 Co-authored-by: kubaflo <42434498+kubaflo@users.noreply.github.com> * Final review summary for PR dotnet#32648 Co-authored-by: kubaflo <42434498+kubaflo@users.noreply.github.com> --------- Co-Authored-By: Copilot <198982749+Copilot@users.noreply.github.com> Co-Authored-By: Jakub Florkowski <42434498+kubaflo@users.noreply.github.com>
…tnet#33071) App crashes because of changing a shared Drawable on the fly. I found out it is the SearchBar. It only happens on Android and in large apps with lots of screens. Unfortunately I can't reproduce it in the TestSuite. But by reading the docs of Android this crash is common and caused by changing a Drawable which is still referenced. With a custom SearchBarHandler the issues is resolved. ```Backtrace (top frames): #00 pc 00000000006b8694 /system/lib64/libhwui.so android::getRootAlpha(_JNIEnv*, _jobject*, long) +4 #1 pc 0000000002256c90 /memfd:jit-cache (deleted) art_jni_trampoline +112 #2 pc 000000000223bc4c /memfd:jit-cache (deleted) android.graphics.drawable.VectorDrawable.-$$Nest$smnGetRootAlpha +108 #3 pc 000000000223bb20 /memfd:jit-cache (deleted) android.graphics.drawable.VectorDrawable$VectorDrawableState.getAlpha +144 #4 pc 00000000025c50e0 /memfd:jit-cache (deleted) android.graphics.drawable.VectorDrawable.getAlpha +128 #5 pc 00000000025c4f9c /memfd:jit-cache (deleted) android.graphics.drawable.VectorDrawable.getOpacity +124 #6 pc 00000000025c1ea8 /memfd:jit-cache (deleted) android.widget.ImageView.isOpaque +152 #7 pc 000000000227979c /memfd:jit-cache (deleted) android.view.View.invalidateInternal +428 #8 pc 00000000025c4790 /memfd:jit-cache (deleted) android.widget.ImageView.invalidateDrawable +256 #9 pc 000000000224419c /memfd:jit-cache (deleted) android.graphics.drawable.Drawable.invalidateSelf +156 #10 pc 000000000260e710 /memfd:jit-cache (deleted) android.graphics.drawable.VectorDrawable.setTintList +192 #11 pc 00000000025d0094 /memfd:jit-cache (deleted) **android.graphics.drawable.Drawable.setTint +148** ``` ### Description of Change - Changes tinting of Androids SearchBar to unified setTint instead of setColorFilter - Mutates the drawable before setting the tint. ### Issues Fixed Issue is fixed with a custom handler for now. Fixes dotnet#33070
…tnet#33071) App crashes because of changing a shared Drawable on the fly. I found out it is the SearchBar. It only happens on Android and in large apps with lots of screens. Unfortunately I can't reproduce it in the TestSuite. But by reading the docs of Android this crash is common and caused by changing a Drawable which is still referenced. With a custom SearchBarHandler the issues is resolved. ```Backtrace (top frames): #00 pc 00000000006b8694 /system/lib64/libhwui.so android::getRootAlpha(_JNIEnv*, _jobject*, long) +4 #1 pc 0000000002256c90 /memfd:jit-cache (deleted) art_jni_trampoline +112 #2 pc 000000000223bc4c /memfd:jit-cache (deleted) android.graphics.drawable.VectorDrawable.-$$Nest$smnGetRootAlpha +108 #3 pc 000000000223bb20 /memfd:jit-cache (deleted) android.graphics.drawable.VectorDrawable$VectorDrawableState.getAlpha +144 #4 pc 00000000025c50e0 /memfd:jit-cache (deleted) android.graphics.drawable.VectorDrawable.getAlpha +128 #5 pc 00000000025c4f9c /memfd:jit-cache (deleted) android.graphics.drawable.VectorDrawable.getOpacity +124 #6 pc 00000000025c1ea8 /memfd:jit-cache (deleted) android.widget.ImageView.isOpaque +152 #7 pc 000000000227979c /memfd:jit-cache (deleted) android.view.View.invalidateInternal +428 #8 pc 00000000025c4790 /memfd:jit-cache (deleted) android.widget.ImageView.invalidateDrawable +256 #9 pc 000000000224419c /memfd:jit-cache (deleted) android.graphics.drawable.Drawable.invalidateSelf +156 #10 pc 000000000260e710 /memfd:jit-cache (deleted) android.graphics.drawable.VectorDrawable.setTintList +192 #11 pc 00000000025d0094 /memfd:jit-cache (deleted) **android.graphics.drawable.Drawable.setTint +148** ``` ### Description of Change - Changes tinting of Androids SearchBar to unified setTint instead of setColorFilter - Mutates the drawable before setting the tint. ### Issues Fixed Issue is fixed with a custom handler for now. Fixes dotnet#33070
…tnet#33071) App crashes because of changing a shared Drawable on the fly. I found out it is the SearchBar. It only happens on Android and in large apps with lots of screens. Unfortunately I can't reproduce it in the TestSuite. But by reading the docs of Android this crash is common and caused by changing a Drawable which is still referenced. With a custom SearchBarHandler the issues is resolved. ```Backtrace (top frames): #00 pc 00000000006b8694 /system/lib64/libhwui.so android::getRootAlpha(_JNIEnv*, _jobject*, long) +4 #1 pc 0000000002256c90 /memfd:jit-cache (deleted) art_jni_trampoline +112 #2 pc 000000000223bc4c /memfd:jit-cache (deleted) android.graphics.drawable.VectorDrawable.-$$Nest$smnGetRootAlpha +108 #3 pc 000000000223bb20 /memfd:jit-cache (deleted) android.graphics.drawable.VectorDrawable$VectorDrawableState.getAlpha +144 #4 pc 00000000025c50e0 /memfd:jit-cache (deleted) android.graphics.drawable.VectorDrawable.getAlpha +128 #5 pc 00000000025c4f9c /memfd:jit-cache (deleted) android.graphics.drawable.VectorDrawable.getOpacity +124 #6 pc 00000000025c1ea8 /memfd:jit-cache (deleted) android.widget.ImageView.isOpaque +152 #7 pc 000000000227979c /memfd:jit-cache (deleted) android.view.View.invalidateInternal +428 #8 pc 00000000025c4790 /memfd:jit-cache (deleted) android.widget.ImageView.invalidateDrawable +256 #9 pc 000000000224419c /memfd:jit-cache (deleted) android.graphics.drawable.Drawable.invalidateSelf +156 #10 pc 000000000260e710 /memfd:jit-cache (deleted) android.graphics.drawable.VectorDrawable.setTintList +192 #11 pc 00000000025d0094 /memfd:jit-cache (deleted) **android.graphics.drawable.Drawable.setTint +148** ``` ### Description of Change - Changes tinting of Androids SearchBar to unified setTint instead of setColorFilter - Mutates the drawable before setting the tint. ### Issues Fixed Issue is fixed with a custom handler for now. Fixes dotnet#33070
…tnet#33071) App crashes because of changing a shared Drawable on the fly. I found out it is the SearchBar. It only happens on Android and in large apps with lots of screens. Unfortunately I can't reproduce it in the TestSuite. But by reading the docs of Android this crash is common and caused by changing a Drawable which is still referenced. With a custom SearchBarHandler the issues is resolved. ```Backtrace (top frames): #00 pc 00000000006b8694 /system/lib64/libhwui.so android::getRootAlpha(_JNIEnv*, _jobject*, long) +4 #1 pc 0000000002256c90 /memfd:jit-cache (deleted) art_jni_trampoline +112 #2 pc 000000000223bc4c /memfd:jit-cache (deleted) android.graphics.drawable.VectorDrawable.-$$Nest$smnGetRootAlpha +108 #3 pc 000000000223bb20 /memfd:jit-cache (deleted) android.graphics.drawable.VectorDrawable$VectorDrawableState.getAlpha +144 #4 pc 00000000025c50e0 /memfd:jit-cache (deleted) android.graphics.drawable.VectorDrawable.getAlpha +128 #5 pc 00000000025c4f9c /memfd:jit-cache (deleted) android.graphics.drawable.VectorDrawable.getOpacity +124 #6 pc 00000000025c1ea8 /memfd:jit-cache (deleted) android.widget.ImageView.isOpaque +152 #7 pc 000000000227979c /memfd:jit-cache (deleted) android.view.View.invalidateInternal +428 #8 pc 00000000025c4790 /memfd:jit-cache (deleted) android.widget.ImageView.invalidateDrawable +256 #9 pc 000000000224419c /memfd:jit-cache (deleted) android.graphics.drawable.Drawable.invalidateSelf +156 #10 pc 000000000260e710 /memfd:jit-cache (deleted) android.graphics.drawable.VectorDrawable.setTintList +192 #11 pc 00000000025d0094 /memfd:jit-cache (deleted) **android.graphics.drawable.Drawable.setTint +148** ``` ### Description of Change - Changes tinting of Androids SearchBar to unified setTint instead of setColorFilter - Mutates the drawable before setting the tint. ### Issues Fixed Issue is fixed with a custom handler for now. Fixes dotnet#33070
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!
Description of Change
Addresses navigation hangs on iOS 26 when rapidly pushing/popping pages. The
_navStackin ShellSection initializes with{ null }and can contain null entries during lifecycle transitions. Two safety issues identified:DidPopItem method (line 127):
pages[targetIndex]before passing toSyncStackDownToElementForViewController method (line 582):
(IPlatformViewHandler)child.Handlerto safe castchild.Handler as IPlatformViewHandlerOnRemoveRequestedmethodIssues Fixed
Fixes dotnet#32425
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.