Fix a crash setting the hotkey during teardown #12580

zadjii-msft · 2022-02-25T17:01:50Z

From MSFT:36797001. Okay so this is only .22% of our crashes, but every little bit helps, right?

Turns out this is also hitting in:

and together they're a fairly hot bug.

There's a large class of bugs where we might get a callback to one of our event handlers when we call app.Close() in the AppHost dtor. This PR adds manual revokers to these events, and makes sure to revoke them BEFORE nulling out the _window. That will prevent callbacks during the rest of the dtor, when the _window is null.

From MSFT:36797001. Okay so this is only .22% of our crashes, but every little bit helps, right?

DHowett · 2022-02-25T17:28:31Z

src/cascadia/WindowsTerminal/AppHost.cpp

@@ -967,6 +967,15 @@ winrt::fire_and_forget AppHost::_setupGlobalHotkeys()
    // The hotkey MUST be registered on the main thread. It will fail otherwise!
    co_await winrt::resume_foreground(_logic.GetRoot().Dispatcher());

+    if (!_window)


Ah we null it out properly too? Awesome

Wait, is this sufficient? The only time we will observe _window to be null is after our destructor has run... which would make accessing &this->_window an AV (or UB). We need to make sure that AppHost is properly lifetime-managed before we get here

AH YOU'D THINK

Looking at the stack, this hits in this sliver of time where we've called ~AppHost, we've nulled out the _window, but then the Dispatcher like, runs a bunch more and... flushes callbacks? I'm not sure what it's doing here. There's been miscellaneous crashes over the last few months where like, we try to set the title of the window but it doesn't exist, because we're in this bit of Dispatcher rundown. This is just another version of that. They're all crashes once we've already started tearing down, so they're not that user-impactful, but they still happen.

IDK if it's safe to just gank the Dispatcher before we null out the _window to release the XAML bits.

If the ~AppHost dtor is not on the stack, it is still UB to access this member; I'm concerned this is a band-aid? :)

Oh sorry, to clarify, the ~AppHost dtor is on the stack here.

DHowett

notes

He's on vacation and this fix is affecting internal reliability calculations. And Griese answered the notes.

miniksa · 2022-03-01T17:55:40Z

src/cascadia/WindowsTerminal/AppHost.h

+    winrt::TerminalApp::AppLogic::CloseRequested_revoker _CloseRequestedRevoker;
+    winrt::TerminalApp::AppLogic::RequestedThemeChanged_revoker _RequestedThemeChangedRevoker;
+    winrt::TerminalApp::AppLogic::FullscreenChanged_revoker _FullscreenChangedRevoker;
+    winrt::TerminalApp::AppLogic::FocusModeChanged_revoker _FocusModeChangedRevoker;
+    winrt::TerminalApp::AppLogic::AlwaysOnTopChanged_revoker _AlwaysOnTopChangedRevoker;
+    winrt::TerminalApp::AppLogic::RaiseVisualBell_revoker _RaiseVisualBellRevoker;
+    winrt::TerminalApp::AppLogic::SystemMenuChangeRequested_revoker _SystemMenuChangeRequestedRevoker;
+    winrt::TerminalApp::AppLogic::ChangeMaximizeRequested_revoker _ChangeMaximizeRequestedRevoker;
+    winrt::TerminalApp::AppLogic::TitleChanged_revoker _TitleChangedRevoker;
+    winrt::TerminalApp::AppLogic::LastTabClosed_revoker _LastTabClosedRevoker;
+    winrt::TerminalApp::AppLogic::SetTaskbarProgress_revoker _SetTaskbarProgressRevoker;
+    winrt::TerminalApp::AppLogic::IdentifyWindowsRequested_revoker _IdentifyWindowsRequestedRevoker;
+    winrt::TerminalApp::AppLogic::RenameWindowRequested_revoker _RenameWindowRequestedRevoker;
+    winrt::TerminalApp::AppLogic::SettingsChanged_revoker _SettingsChangedRevoker;
+    winrt::TerminalApp::AppLogic::IsQuakeWindowChanged_revoker _IsQuakeWindowChangedRevoker;
+    winrt::TerminalApp::AppLogic::SummonWindowRequested_revoker _SummonWindowRequestedRevoker;
+    winrt::TerminalApp::AppLogic::OpenSystemMenu_revoker _OpenSystemMenuRevoker;
+    winrt::TerminalApp::AppLogic::QuitRequested_revoker _QuitRequestedRevoker;
+    winrt::Microsoft::Terminal::Remoting::WindowManager::ShowNotificationIconRequested_revoker _ShowNotificationIconRequestedRevoker;
+    winrt::Microsoft::Terminal::Remoting::WindowManager::HideNotificationIconRequested_revoker _HideNotificationIconRequestedRevoker;
+    winrt::Microsoft::Terminal::Remoting::WindowManager::QuitAllRequested_revoker _QuitAllRequestedRevoker;
 };


Woof. This is going to be the next candidate to be put into the macros.

I quite nearly had a std::deque<> of event revokers that I just pushed them all into, then pop'd and revoked, but alas, I'm not lucky enough to have them all share some common base class ;____;

miniksa

Good with this.

If you can imagine a way that we could remove this foot-gun with further X-macros... please file that follow-on (aka automate the creation of the revokers and the revocations).

Otherwise just get your typo and let's get this on its way to merging in.

ghost · 2022-03-01T18:04:52Z

Hello @zadjii-msft!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

From MSFT:36797001. Okay so this is only .22% of our crashes, but every little bit helps, right? Turns out this is also hitting in: * MSFT:35726322 * MSFT:34662459 and together they're a fairly hot bug. There's a large class of bugs where we might get a callback to one of our event handlers when we call `app.Close()` in the `AppHost` dtor. This PR adds manual revokers to these events, and makes sure to revoke them BEFORE nulling out the `_window`. That will prevent callbacks during the rest of the dtor, when the `_window` is null.

From MSFT:36797001. Okay so this is only .22% of our crashes, but every little bit helps, right? Turns out this is also hitting in: * MSFT:35726322 * MSFT:34662459 and together they're a fairly hot bug. There's a large class of bugs where we might get a callback to one of our event handlers when we call `app.Close()` in the `AppHost` dtor. This PR adds manual revokers to these events, and makes sure to revoke them BEFORE nulling out the `_window`. That will prevent callbacks during the rest of the dtor, when the `_window` is null. (cherry picked from commit 8962c75)

ghost · 2022-03-25T22:49:38Z

🎉Windows Terminal v1.12.1073 has been released which incorporates this pull request.:tada:

Handy links:

ghost · 2022-03-25T22:51:20Z

🎉Windows Terminal Preview v1.13.1073 has been released which incorporates this pull request.:tada:

Handy links:

Fix a crash setting the hotkey during teardown

f13c81e

From MSFT:36797001. Okay so this is only .22% of our crashes, but every little bit helps, right?

zadjii-msft added Severity-Crash Crashes are real bad news. zStable-Service-Queued-1.12 A floating label that tracks the current Stable version for servicing purposes. zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. labels Feb 25, 2022

DHowett approved these changes Feb 25, 2022

View reviewed changes

DHowett previously requested changes Feb 25, 2022

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Feb 25, 2022

manually revoke every handler

94e3963

lhecker approved these changes Mar 1, 2022

View reviewed changes

cleanup

b1d830f

This comment was marked as resolved.

Sign in to view

manually

22c6a2f

zadjii-msft assigned miniksa Mar 1, 2022

miniksa reviewed Mar 1, 2022

View reviewed changes

miniksa approved these changes Mar 1, 2022

View reviewed changes

zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Mar 1, 2022

zadjii-msft removed the AutoMerge Marked for automatic merge by the bot when requirements are met label Mar 1, 2022

this is clever

d41f7b4

zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Mar 1, 2022

ghost merged commit 8962c75 into main Mar 1, 2022

ghost deleted the dev/migrie/b/36797001-register-hotkey-crash branch March 1, 2022 18:50

miniksa removed the zStable-Service-Queued-1.12 A floating label that tracks the current Stable version for servicing purposes. label Mar 2, 2022

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix a crash setting the hotkey during teardown #12580

Fix a crash setting the hotkey during teardown #12580

zadjii-msft commented Feb 25, 2022 •

edited

Loading

DHowett Feb 25, 2022

DHowett Feb 25, 2022

zadjii-msft Feb 25, 2022

DHowett Feb 25, 2022

zadjii-msft Feb 25, 2022

DHowett left a comment

This comment was marked as resolved.

miniksa Mar 1, 2022

zadjii-msft Mar 1, 2022

miniksa left a comment

ghost commented Mar 1, 2022

ghost commented Mar 25, 2022

ghost commented Mar 25, 2022

Fix a crash setting the hotkey during teardown #12580

Fix a crash setting the hotkey during teardown #12580

Conversation

zadjii-msft commented Feb 25, 2022 • edited Loading

DHowett Feb 25, 2022

Choose a reason for hiding this comment

DHowett Feb 25, 2022

Choose a reason for hiding this comment

zadjii-msft Feb 25, 2022

Choose a reason for hiding this comment

DHowett Feb 25, 2022

Choose a reason for hiding this comment

zadjii-msft Feb 25, 2022

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

miniksa Mar 1, 2022

Choose a reason for hiding this comment

zadjii-msft Mar 1, 2022

Choose a reason for hiding this comment

miniksa left a comment

Choose a reason for hiding this comment

ghost commented Mar 1, 2022

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

ghost commented Mar 25, 2022

ghost commented Mar 25, 2022

zadjii-msft commented Feb 25, 2022 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.