[Shortcut Guide] Prevent overlay crash on section navigation (#48448)#48481
Open
niels9001 wants to merge 2 commits into
Open
[Shortcut Guide] Prevent overlay crash on section navigation (#48448)#48481niels9001 wants to merge 2 commits into
niels9001 wants to merge 2 commits into
Conversation
…ft#48448) WindowSelector_SelectionChanged invokes App.TaskBarWindow.Activate() and then SetWindowPosition() synchronously. Activate() runs a reentrant Window_Activated/BringToFront/TaskbarWindow.Activated chain that can leave App.TaskBarWindow.AppWindow null momentarily, throwing NRE inside SetWindowPosition. Because SelectionChanged is fired from SetNavItems' initial SelectedItem assignment, the NRE bubbles up to InitializeNavItemsAsync's catch and closes the window with _closeType=InitializationFailed, matching the user-visible 'flash and disappear' / 'overlay closes when clicking Windows icon' reports. - Null-guard App.TaskBarWindow?.AppWindow in SetWindowPosition; skip the overlap adjustment when unavailable. - Wrap SetWindowPosition body in try/catch with Logger.LogError so any future positioning failure keeps the previous layout instead of taking down the overlay. - Null-guard App.TaskBarWindow?.Hide() / Activate() and wrap WindowSelector_SelectionChanged body in try/catch so an exception during navigation no longer reaches InitializeNavItemsAsync's failure path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the Shortcut Guide overlay against a reentrancy-related crash when navigating between sidebar sections by preventing exceptions in the selection-change/positioning path from propagating into initialization failure logic.
Changes:
- Wraps
SetWindowPosition()in atry/catchand skips taskbar overlap calculations whenApp.TaskBarWindow.AppWindowis temporarily unavailable. - Wraps
WindowSelector_SelectionChangedin atry/catchand adds null-guards around taskbar window interactions to avoid closing the overlay on navigation-time exceptions. - Adds new error logging for both the positioning and section-selection failure paths.
Comment on lines
+284
to
+287
| catch (Exception ex) | ||
| { | ||
| Logger.LogError("Failed to set Shortcut Guide window position; keeping previous layout.", ex); | ||
| } |
Comment on lines
+312
to
+316
| if (file.Shortcuts is not null && file.Shortcuts.Any(c => c.SectionName?.StartsWith("<TASKBAR1-9>", StringComparison.Ordinal) == true)) | ||
| { | ||
| this._taskBarWindowActivated = true; | ||
| App.TaskBarWindow?.Activate(); | ||
| } |
…aunch + IO guards) Complements the navigation-race fix from the previous commit with broader crash hardening based on review of issue microsoft#48441 (overlay flashes and disappears, coreclr.dll access violation). - App.xaml.cs: register App.UnhandledException, AppDomain.CurrentDomain. UnhandledException, and TaskScheduler.UnobservedTaskException so a stray exception from a fire-and-forget UI handler or a background Task fault gets logged instead of taking the overlay down with an unhandled access violation in coreclr. Mirrors what Peek, AdvancedPaste, and CmdPal already do. - App.OnLaunched: wrap launch sequence in try/catch and exit cleanly with an error log on failure (LoadData / MainWindow / TaskbarWindow ctors and Activate are all reachable failure points). - App.LoadData: broaden the Pinned.json deserialize catch to also handle IOException / UnauthorizedAccessException, and guard the round-trip SaveSettings call as best-effort with a warning. - PinnedShortcutsHelper.Save: catch IO / Json failures and log; Pin/Unpin runs from a synchronous UI handler so an unguarded File.WriteAllText would tear down the overlay on any disk hiccup. - TaskbarWindow.UpdateTasklistButtons: move the AppWindow.Move calls inside the try block, null-guard App.MainWindow?.AppWindow up front, and wrap the whole body so the method (which runs from the ctor and from Activated) cannot tear the overlay down when MainWindow is in a transient state. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Thank you for contributing to PowerToys. We've detected that this PR might include a new or modified telemetry event. Please ensure the following before merging:
|
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 of the Pull Request
Shortcut Guide overlay crashes and closes when navigating between sidebar sections (e.g. clicking PowerToys, then clicking the Windows icon to come back). Repro and crash logs in #48448. Crash logs in #48441 show the same propagation path plus a follow-up access violation in coreclr, indicating exceptions that escape local catches.
This PR fixes the immediate navigation race and adds broader crash hardening so future exceptions on the UI/background threads are logged instead of tearing down the overlay.
Navigation-race fix (commit 1)
Root cause:
WindowSelector_SelectionChangedcallsApp.TaskBarWindow.Activate()and thenSetWindowPosition()synchronously.Activate()runs a reentrantWindow_Activated→BringToFront→TaskbarWindow.Activatedchain that can leaveApp.TaskBarWindow.AppWindowmomentarily null, soSetWindowPositionthrowsNullReferenceException.Because the initial
SelectedItem = MenuItems[0]is set fromSetNavItems, the NRE bubbles up intoInitializeNavItemsAsync's catch block — which sets_closeType = "InitializationFailed"and closes the window. That matches both user-visible symptoms: the overlay "flashes and disappears" (#48441) on open and "closes when clicking the Windows icon" (#48448).Edits in
src/modules/ShortcutGuide/ShortcutGuide.Ui/ShortcutGuideXAML/MainWindow.xaml.cs:SetWindowPosition: null-guardApp.TaskBarWindow?.AppWindowand skip the taskbar-overlap height adjustment when it is not currently observable. Wrap the body intry/catchwithLogger.LogErrorso any future positioning hiccup keeps the previous layout instead of taking down the overlay.WindowSelector_SelectionChanged: null-guardApp.TaskBarWindow?.Hide()/Activate()and wrap the body intry/catch. This breaks the propagation path that lets a navigation exception reachInitializeNavItemsAsync's "fatal init failure" branch.Crash hardening (commit 2)
Additional defensive changes to cover other unguarded paths surfaced while reviewing #48441:
App.xaml.cs: registerApp.UnhandledException,AppDomain.CurrentDomain.UnhandledException, andTaskScheduler.UnobservedTaskExceptionso a stray exception (e.g. an IO failure during a fire-and-forget UI handler, or a backgroundTaskfault) gets logged instead of tearing the process down with an access violation in coreclr. Mirrors what Peek, AdvancedPaste, and CmdPal already do.App.OnLaunched: wrap the launch sequence intry/catchand exit cleanly with an error log on failure (LoadData/MainWindow/TaskbarWindowctors andActivateare all reachable failure points).App.LoadData: broaden thePinned.jsondeserialize catch to also handleIOException/UnauthorizedAccessException, and guard the round-tripSaveSettingscall as best-effort with a warning log.PinnedShortcutsHelper.Save: catchIOException/UnauthorizedAccessException/JsonExceptionand log;Pin/Unpinruns from a synchronous UI handler so an unguardedFile.WriteAllTextwould tear down the overlay on any disk hiccup.TaskbarWindow.UpdateTasklistButtons: move theAppWindow.Movecalls inside the existingtryblock, null-guardApp.MainWindow?.AppWindowup front, and wrap the whole body so the method (which runs from the ctor and fromActivated) cannot tear the overlay down whenMainWindowis in a transient state.PR Checklist
Detailed Description of the Pull Request / Additional comments
The fix is intentionally defensive (rather than restructuring the reentrant activation flow) because the legacy taskbar UIA enumeration (
TasklistPositions.GetButtons) on Windows 10 is what most reliably widens the race window and is out of scope to redesign for a hotfix.Validation Steps Performed
tools\build\build.cmdfromsrc\modules\ShortcutGuide\ShortcutGuide.Ui— exit 0, errors log empty for both commits.throw new NullReferenceException()at the top ofSetWindowPositionto exercise the exact propagation path the reporter's log shows (SetWindowPosition→SelectionChanged→set_SelectedItem→InitializeNavItemsAsync.catch→Close("InitializationFailed")).Failed to initialize navigation items.with the NRE.Failed to set Shortcut Guide window position; keeping previous layout.from the newcatch.