fix(v3/windows): don't fatal on GetCursorPos failure (locked/secure desktop)#5789
fix(v3/windows): don't fatal on GetCursorPos failure (locked/secure desktop)#5789wayneforrest wants to merge 1 commit into
Conversation
…esktop) GetAllScreens() aborted with "GetCursorPos failed" when GetCursorPos returned FALSE. GetCursorPos returns ERROR_ACCESS_DENIED whenever the calling process is not on the interactive input desktop - the workstation is locked, on the secure/UAC desktop, or mid session switch (logon, RDP, fast-user-switch). At startup, processAndCacheScreens() turns that error into a fatal os.Exit, so an app launched onto a locked desktop (e.g. an MSIX StartupTask firing at logon before the desktop is interactive) dies before it ever shows. The cursor is only used to set the cosmetic IsCurrent flag, which has no readers. Degrade gracefully: query the cursor through a seam, and when it fails leave IsCurrent=false and keep enumerating. The flag self-corrects on the next WM_DISPLAYCHANGE re-cache (already non-fatal). Also fix the sibling Win32Menu.ShowAtCursor(), which fatal()s on the same failure; a context menu simply cannot be shown on a non-input desktop, so skip it - matching windowsMenu.ShowAtCursor()'s graceful return. Adds a Windows regression test that overrides the cursor seam. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011jmjTLaZ8nFB5veaPjRnhA
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughCursor-position failures on Windows are now handled gracefully. Popup menus log and return instead of terminating, while screen enumeration continues without marking a current monitor when the cursor location is unavailable. Windows-only tests cover both behaviors and the default cursor-position seam. ChangesCursor Failure Handling
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain modules listed in go.work or their selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
One more PR - then my app will be crash free. |
Problem
On Windows,
w32.GetAllScreens()queriesGetCursorPos(only to set the cosmeticScreen.IsCurrentflag) and treats a failure as a hard error:GetCursorPosreturnsFALSE/ERROR_ACCESS_DENIEDwhenever the calling process is not attached to the interactive input desktop — the workstation is locked, on the secure/UAC desktop, or mid session switch (logon, RDP, fast-user-switch).At startup this is fatal.
processAndCacheScreens()runs duringApp.Run()(vianewPlatformApp) and turns the error intoos.Exit(1):So an app launched onto a non-interactive desktop dies before it ever shows, printing the "catastrophic failure … GetCursorPos failed" banner. This is reproducible for packaged/MSIX apps registered as a StartupTask (the shell launches them at logon, before the session's input desktop is switched away from Winlogon), and for background-update restarts that land while the machine is locked. We hit it in the field on a Microsoft Store build.
Fix
The cursor position is used only to compute
IsCurrent(which monitor the pointer is on), and that field has no readers — it isn't even propagated to the application-levelScreen. So a failed query should degrade gracefully instead of aborting:GetAllScreensnow reads the cursor through a small package seam and, when it fails, leavesIsCurrent = falseand keeps enumerating. The flag self-corrects on the nextWM_DISPLAYCHANGE/SPI_SETWORKAREAre-cache, which already handles errors non-fatally viahandleError.Win32Menu.ShowAtCursor()had the sameglobalApplication.fatal("GetCursorPos failed"). A context menu simply cannot be shown on a non-input desktop, so it now returns (with a debug log) — matching the already-gracefulwindowsMenu.ShowAtCursor(), which doesif !ok { return }.A Windows regression test overrides the seam to simulate the failure and asserts
GetAllScreensno longer returns a cursor error and marks no screen current.Notes
windowsMenu.ShowAtCursor()already tolerated the failure whileWin32Menu.ShowAtCursor()andGetAllScreens()treated it as fatal.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests