Add defensive error handling for Settings navigation and search#46688
Open
tburns10 wants to merge 1 commit intomicrosoft:mainfrom
Open
Add defensive error handling for Settings navigation and search#46688tburns10 wants to merge 1 commit intomicrosoft:mainfrom
tburns10 wants to merge 1 commit intomicrosoft:mainfrom
Conversation
09b4592 to
134f12a
Compare
Addresses the root cause chain behind the NullReferenceException crash in Frame_NavigationFailed. Crash dump analysis with WinDbg revealed that Frame.Navigate() can throw native WinUI ABI exceptions that propagate unhandled through multiple code paths. NavigationService.Navigate: - Wrap Frame.Navigate() in try-catch to gracefully handle native WinUI ABI failures (e.g. page XAML parse errors, InitializeComponent failures) - Log the exception details for diagnostics instead of letting it propagate - Return false on failure so callers can handle navigation failure gracefully NavigationService.EnsurePageIsSelected: - Add matching try-catch for consistency — this method also calls Frame.Navigate() without protection ShellViewModel.Frame_NavigationFailed: - Replace 'throw e.Exception' with 'e.Handled = true' and Logger.LogError - The original code threw a NullReferenceException when e.Exception was null (native errors don't always marshal to managed Exception objects) - Mark the event handled to prevent the framework from propagating the error ShellPage.SearchBox_QuerySubmitted: - Add try-catch around the entire async void method body - Prevents unhandled exceptions from crashing the process when search indexing (Task.Run) or navigation to SearchResultsPage fails Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Member
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Author
|
@crutkas anything I need to do here? |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the Settings UI navigation and search flows against WinUI navigation failures that can surface as native/ABI exceptions (or as NavigationFailed with a null Exception), preventing process-terminating unhandled exceptions.
Changes:
- Wrap
Frame.Navigate()call sites inNavigationServicewithtry/catch, log failures, and returnfalsewhen navigation cannot be performed. - Update
ShellViewModel’s navigation failure handler to mark failures as handled and log instead of rethrowinge.Exception. - Add a top-level
try/catchtoShellPage’sasync voidsearch submission handler to prevent unhandled exceptions afterawait.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/settings-ui/Settings.UI/Services/NavigationService.cs | Adds defensive exception handling around navigation calls and logs failures instead of letting exceptions propagate. |
| src/settings-ui/Settings.UI/ViewModels/ShellViewModel.cs | Prevents crashes on NavigationFailed by handling/logging rather than throwing (including null-safe logging). |
| src/settings-ui/Settings.UI/SettingsXAML/Views/ShellPage.xaml.cs | Prevents async void search submission exceptions from crashing the app by catching/logging the full handler body. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses the root cause chain behind the NullReferenceException crash in Settings navigation. Crash dump analysis with WinDbg revealed that
Frame.Navigate()can throw native WinUI ABI exceptions that propagate unhandled through multiple code paths.Changes (3 files)
NavigationService.cs — Defensive try-catch for all
Frame.Navigate()call sites:Navigate(): Wrap in try-catch, log viaLogger.LogError(), returnfalseon failureEnsurePageIsSelected(): Add matching try-catch for consistency (also callsFrame.Navigate()unprotected)ShellViewModel.cs — Fix the crash-causing
Frame_NavigationFailedhandler:throw e.Exceptionwithe.Handled = true+Logger.LogError()NullReferenceExceptionwhene.Exceptionwas null (native WinUI errors don't always marshal to managed Exception objects)ShellPage.xaml.cs — Protect
async void SearchBox_QuerySubmitted:async voidmethods that throw afterawaitproduce unhandled exceptions that crash the processTask.Run) and navigation failuresCrash Dump Evidence
Analysis of
PowerToys.Settings_2025_03_26_48636.dmpwith WinDbg:0x000001c60d3b9100SearchBox_QuerySubmitted→Frame.Navigate()fails at native ABI →NavigationFailedfires with null Exception →throw null→NullReferenceException→FailFastWithStowedExceptionsMicrosoft.UI.Xaml.dll!DirectUI::Frame::NavigateReview Notes
catch (Exception)pattern matches 92.5% of Settings.UI codebase conventionLogger.LogError()withusing ManagedCommonmatches standard import pattern