[Peek] Stop fail-fast in AppWindow.Closing path; reset cached preview-handler factories on release#48564
Conversation
…-handler factories on release The Peek MainWindow's AppWindow.Closing handler calls Uninitialize, which in turn calls ShellPreviewHandlerPreviewer.ReleaseHandlerFactories. The static HandlerFactories dictionary kept entries alive after FinalReleaseComObject, so a later use of a cached IClassFactory could touch a separated RCW and throw InvalidComObjectException (HRESULT 0x80131527). Any exception that escapes a WinRT event callback is projected back to CFlat as a failed HR and the CsWinRT dispatcher fail-fasts the process. Three changes: * Clear the HandlerFactories dictionary up front in ReleaseHandlerFactories and call LockServer(false) before FinalReleaseComObject, so the method is idempotent and a concurrent LoadPreviewAsync can't pick up a stale entry. * Wrap Peek MainWindow.AppWindow_Closing in try/catch + Logger.LogError so a transient teardown failure logs instead of crashing Peek. * Apply the same defensive try/catch to RegistryPreview MainWindow.AppWindow_Closing and null-guard jsonWindowPlacement before SetNamedValue. 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 comment has been minimized.
This comment has been minimized.
…arden Uninitialize exit-after-close A second pass over the Peek teardown surface (after the earlier AppWindow.Closing fail-fast fix) turned up a handful of related issues on the preview-handler cache and the CLI exit path. Each one is small on its own; together they explain a few of the lower-frequency Peek crashes / leaks seen during repeated open-close cycles and during exit via `-FilePath`. Changes: 1. `ShellPreviewHandlerPreviewer.LoadPreviewAsync` factory caching: Replace `AddOrUpdate` with `GetOrAdd` + loser-release. The old pattern let two threads concurrently miss `TryGetValue`, both `CoGetClassObject` + `LockServer(true)`, then race on `AddOrUpdate` - the second factory silently won and the first was leaked locked. Now whichever side loses the `GetOrAdd` releases its own factory (`LockServer(false)` + `FinalReleaseComObject`). 2. Same path, retry-on-dead-server branch: when we drop the cached factory after a `CreateInstance` failure we now also release our `LockServer` hold so the dead local server can be torn down, instead of leaving the cache un-paired. 3. `ShellPreviewHandlerPreviewer.LoadPreviewAsync` ownership tracking for the freshly-created preview handler: between the `CreateInstance` call and `Preview = previewHandler`, any cancellation or init failure used to leak the RCW (and the prevhost.exe behind it). Wrap that span in try/finally with an `ownsHandler` flag and `FinalReleaseComObject` if ownership never transfers. 4. `ShellPreviewHandlerPreviewer.Clear` - split the `Unload` and `FinalReleaseComObject` calls into separate try blocks. If `Unload` throws (the preview-handler host process crashed mid-way), the RCW must still be released - the previous single try block would skip the release. 5. `ShellPreviewHandlerPreviewer.ReleaseHandlerFactories` - drain via `TryRemove` per key instead of `ToArray` + `Clear`. The old shape had a race window where a concurrent `LoadPreviewAsync` could `AddOrUpdate` between the snapshot and the `Clear`, and that new entry would then be dropped without the matching `LockServer(false)` + `FinalRelease`, leaking a locked local-server factory. 6. `Peek.UI MainWindow.Uninitialize` - move the try/catch from `AppWindow_Closing` into `Uninitialize` itself, and put `Environment.Exit(0)` in `finally`. The outer wrap was correct for fail-fast prevention but it converted CLI `-FilePath` exit-after- close into a hang any time `Uninitialize` threw: the exception was caught upstream and the `Environment.Exit(0)` tail was never reached. Round-2 cross-review caught this regression; the new shape keeps both contracts (no fail-fast and CLI exit still happens) regardless of cleanup outcome. --- ADO: https://microsoft.visualstudio.com/DefaultCollection/OS/_workitems/edit/58765809/ Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| { | ||
| // Split Unload() and FinalReleaseComObject() into separate try | ||
| // blocks: if Unload throws (the preview-handler host process | ||
| // crashed mid-tear-down) the RCW must still be released, otherwise |
This comment has been minimized.
This comment has been minimized.
@check-spelling-bot Report🔴 Please reviewSee the 📂 files view, the 📜action log, 👼 SARIF report, or 📝 job summary for details.
See ❌ Event descriptions for more information. Some files were automatically ignored 🙈These sample patterns would exclude them: You should consider adding them to: File matching is via Perl regular expressions. To check these files, more of their words need to be in the dictionary than not. You can use To update file exclusions, you could run the following commands... in a clone of the git@github.com:crutkas/autoUpgradeAttempt.git repository curl -s -S -L 'https://raw.githubusercontent.com/check-spelling/check-spelling/cfb6f7e75bbfc89c71eaa30366d0c166f1bd9c8c/apply.pl' |
perl - 'https://github.com/microsoft/PowerToys/actions/runs/27450703081/attempts/1' &&
git commit -m 'Update check-spelling metadata'Forbidden patterns 🙅 (2)In order to address this, you could change the content to not match the forbidden patterns (comments before forbidden patterns may help explain why they're forbidden), add patterns for acceptable instances, or adjust the forbidden patterns themselves. These forbidden patterns matched content: Should be
|
Summary
Harden Peek's
AppWindow.Closingpath so a stale cached preview-handler factory can't fail-fast the Peek process. Also clean up the matching path in RegistryPreview.Background
Spotted while reading through Peek's
MainWindowteardown sequence and theShellPreviewHandlerPreviewercache for an unrelated review of how Peek manages out-of-process preview-handler lifetimes.The Peek
MainWindowsubscribes toAppWindow.Closing. The handler doesn't actually close the window — it setsargs.Cancel = trueand callsUninitialize(), which in turn callsShellPreviewHandlerPreviewer.ReleaseHandlerFactories().ReleaseHandlerFactories()looked like this:Two problems:
HandlerFactoriesdictionary is never cleared. AfterFinalReleaseComObject, the entries still point at separated RCWs. A subsequent activation that races with this cleanup (or a second close in the same process) can pick up the dead RCW from the cache.LockServer(true)called on it when it was first cached, but the matchingLockServer(false)was never paired.Any managed exception that escapes a WinRT event callback is projected back to CFlat as a failed HRESULT and the CsWinRT dispatcher fail-fasts the process. So a single
InvalidComObjectException(HRESULT 0x80131527) thrown out ofUninitialize()is enough to terminate Peek.Changes
ShellPreviewHandlerPreviewer.ReleaseHandlerFactories— snapshot then clear the dictionary up front so that a subsequent call (or a concurrentLoadPreviewAsync) can't pick up a stale RCW. CallLockServer(false)beforeFinalReleaseComObjectto mirror the cache-timeLockServer(true). Both COM calls remain individually wrapped because the RCW may already be unreachable during process teardown.Peek.UI/MainWindow.xaml.cs—AppWindow_Closing— wrap the body in try/catch +Logger.LogError. Any future exception inUninitialize()(or its callees) will now log instead of fail-fasting the process.RegistryPreview/MainWindow.Events.cs—AppWindow_Closing— same defensive try/catch, plus null-guardjsonWindowPlacementbeforeSetNamedValue. The placement dictionary can legitimately be null on first run or after a corrupt placement file; previously that would NRE → fail-fast.Risk
Low. The
ReleaseHandlerFactorieschange matches the documentedLockServer/FinalReleaseComObjectpairing and only widens the lifetime window of the cache byClear()-ing earlier; nothing in Peek calls this method outside of teardown. The two try/catch wrappers strictly add defense — the success path is unchanged.Validation
Spot-built locally; this repo's
dotnet restoreruntime-pack issue (unrelated to this PR — same NU1102 pattern that's affecting other open PRs) prevents a fullBuild.cmdhere. The C++ side of Peek is untouched.Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com
ADO: https://microsoft.visualstudio.com/DefaultCollection/OS/_workitems/edit/58765809/