Skip to content

Commit

Permalink
Fix a crash setting the hotkey during teardown (#12580)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@zadjii-msft
zadjii-msft authored Mar 1, 2022
1 parent d0d42c4 commit 8962c75
Show file tree
Hide file tree
Showing 2 changed files with 149 additions and 40 deletions.
146 changes: 108 additions & 38 deletions src/cascadia/WindowsTerminal/AppHost.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,12 +79,29 @@ AppHost::AppHost() noexcept :
_logic,
std::placeholders::_1,
std::placeholders::_2));

// Event handlers:
// MAKE SURE THEY ARE ALL:
// * winrt::auto_revoke
// * revoked manually in the dtor before the window is nulled out.
//
// If you don't, then it's possible for them to get triggered as the app is
// tearing down, after we've nulled out the window, during the dtor. That
// can cause unexpected AV's everywhere.
//
// _window callbacks don't need to be treated this way, because:
// * IslandWindow isn't a WinRT type (so it doesn't have neat revokers like this)
// * This particular bug scenario applies when we've already freed the window.
//
// (Most of these events are actually set up in AppHost::Initialize)
_window->MouseScrolled({ this, &AppHost::_WindowMouseWheeled });
_window->WindowActivated({ this, &AppHost::_WindowActivated });
_window->WindowMoved({ this, &AppHost::_WindowMoved });
_window->HotkeyPressed({ this, &AppHost::_GlobalHotkeyPressed });
_window->SetAlwaysOnTop(_logic.GetInitialAlwaysOnTop());
_window->ShouldExitFullscreen({ &_logic, &winrt::TerminalApp::AppLogic::RequestExitFullscreen });

_window->SetAlwaysOnTop(_logic.GetInitialAlwaysOnTop());

_window->MakeWindow();

_GetWindowLayoutRequestedToken = _windowManager.GetWindowLayoutRequested([this](auto&&, const winrt::Microsoft::Terminal::Remoting::GetWindowLayoutArgs& args) {
Expand All @@ -93,7 +110,7 @@ AppHost::AppHost() noexcept :
args.WindowLayoutJsonAsync(_GetWindowLayoutAsync());
});

_windowManager.BecameMonarch({ this, &AppHost::_BecomeMonarch });
_revokers.BecameMonarch = _windowManager.BecameMonarch(winrt::auto_revoke, { this, &AppHost::_BecomeMonarch });
if (_windowManager.IsMonarch())
{
_BecomeMonarch(nullptr, nullptr);
Expand All @@ -103,6 +120,13 @@ AppHost::AppHost() noexcept :
AppHost::~AppHost()
{
// destruction order is important for proper teardown here

// revoke ALL our event handlers. There's a large class of bugs where we
// might get a callback to one of these when we call app.Close() below. Make
// sure to revoke these first, so we won't get any more callbacks, then null
// out the window, then close the app.
_revokers = {};

_window = nullptr;
_app.Close();
_app = nullptr;
Expand Down Expand Up @@ -232,11 +256,12 @@ void AppHost::_HandleCommandlineArgs()
// future commandline invocations. When our peasant is told to execute a
// commandline (in the future), it'll trigger this callback, that we'll
// use to send the actions to the app.
peasant.ExecuteCommandlineRequested({ this, &AppHost::_DispatchCommandline });
peasant.SummonRequested({ this, &AppHost::_HandleSummon });

peasant.DisplayWindowIdRequested({ this, &AppHost::_DisplayWindowId });
peasant.QuitRequested({ this, &AppHost::_QuitRequested });
//
// MORE EVENT HANDLERS, same rules as the ones above.
_revokers.peasantExecuteCommandlineRequested = peasant.ExecuteCommandlineRequested(winrt::auto_revoke, { this, &AppHost::_DispatchCommandline });
_revokers.peasantSummonRequested = peasant.SummonRequested(winrt::auto_revoke, { this, &AppHost::_HandleSummon });
_revokers.peasantDisplayWindowIdRequested = peasant.DisplayWindowIdRequested(winrt::auto_revoke, { this, &AppHost::_DisplayWindowId });
_revokers.peasantQuitRequested = peasant.QuitRequested(winrt::auto_revoke, { this, &AppHost::_QuitRequested });

// We need this property to be set before we get the InitialSize/Position
// and BecameMonarch which normally sets it is only run after the window
Expand Down Expand Up @@ -320,30 +345,39 @@ void AppHost::Initialize()
_logic.SetTitleBarContent({ this, &AppHost::_UpdateTitleBarContent });
}

// MORE EVENT HANDLERS HERE!
// MAKE SURE THEY ARE ALL:
// * winrt::auto_revoke
// * revoked manually in the dtor before the window is nulled out.
//
// If you don't, then it's possible for them to get triggered as the app is
// tearing down, after we've nulled out the window, during the dtor. That
// can cause unexpected AV's everywhere.
//
// _window callbacks don't need to be treated this way, because:
// * IslandWindow isn't a WinRT type (so it doesn't have neat revokers like this)
// * This particular bug scenario applies when we've already freed the window.

// Register the 'X' button of the window for a warning experience of multiple
// tabs opened, this is consistent with Alt+F4 closing
_window->WindowCloseButtonClicked([this]() {
const auto pos = _GetWindowLaunchPosition();
_logic.CloseWindow(pos);
_CloseRequested(nullptr, nullptr);
});
// If the user requests a close in another way handle the same as if the 'X'
// was clicked.
_logic.CloseRequested([this](auto&&, auto&&) {
const auto pos = _GetWindowLaunchPosition();
_logic.CloseWindow(pos);
});
_revokers.CloseRequested = _logic.CloseRequested(winrt::auto_revoke, { this, &AppHost::_CloseRequested });

// Add an event handler to plumb clicks in the titlebar area down to the
// application layer.
_window->DragRegionClicked([this]() { _logic.TitlebarClicked(); });

_logic.RequestedThemeChanged({ this, &AppHost::_UpdateTheme });
_logic.FullscreenChanged({ this, &AppHost::_FullscreenChanged });
_logic.FocusModeChanged({ this, &AppHost::_FocusModeChanged });
_logic.AlwaysOnTopChanged({ this, &AppHost::_AlwaysOnTopChanged });
_logic.RaiseVisualBell({ this, &AppHost::_RaiseVisualBell });
_logic.SystemMenuChangeRequested({ this, &AppHost::_SystemMenuChangeRequested });
_logic.ChangeMaximizeRequested({ this, &AppHost::_ChangeMaximizeRequested });
_revokers.RequestedThemeChanged = _logic.RequestedThemeChanged(winrt::auto_revoke, { this, &AppHost::_UpdateTheme });
_revokers.FullscreenChanged = _logic.FullscreenChanged(winrt::auto_revoke, { this, &AppHost::_FullscreenChanged });
_revokers.FocusModeChanged = _logic.FocusModeChanged(winrt::auto_revoke, { this, &AppHost::_FocusModeChanged });
_revokers.AlwaysOnTopChanged = _logic.AlwaysOnTopChanged(winrt::auto_revoke, { this, &AppHost::_AlwaysOnTopChanged });
_revokers.RaiseVisualBell = _logic.RaiseVisualBell(winrt::auto_revoke, { this, &AppHost::_RaiseVisualBell });
_revokers.SystemMenuChangeRequested = _logic.SystemMenuChangeRequested(winrt::auto_revoke, { this, &AppHost::_SystemMenuChangeRequested });
_revokers.ChangeMaximizeRequested = _logic.ChangeMaximizeRequested(winrt::auto_revoke, { this, &AppHost::_ChangeMaximizeRequested });

_window->MaximizeChanged([this](bool newMaximize) {
if (_logic)
Expand All @@ -354,16 +388,16 @@ void AppHost::Initialize()

_logic.Create();

_logic.TitleChanged({ this, &AppHost::AppTitleChanged });
_logic.LastTabClosed({ this, &AppHost::LastTabClosed });
_logic.SetTaskbarProgress({ this, &AppHost::SetTaskbarProgress });
_logic.IdentifyWindowsRequested({ this, &AppHost::_IdentifyWindowsRequested });
_logic.RenameWindowRequested({ this, &AppHost::_RenameWindowRequested });
_logic.SettingsChanged({ this, &AppHost::_HandleSettingsChanged });
_logic.IsQuakeWindowChanged({ this, &AppHost::_IsQuakeWindowChanged });
_logic.SummonWindowRequested({ this, &AppHost::_SummonWindowRequested });
_logic.OpenSystemMenu({ this, &AppHost::_OpenSystemMenu });
_logic.QuitRequested({ this, &AppHost::_RequestQuitAll });
_revokers.TitleChanged = _logic.TitleChanged(winrt::auto_revoke, { this, &AppHost::AppTitleChanged });
_revokers.LastTabClosed = _logic.LastTabClosed(winrt::auto_revoke, { this, &AppHost::LastTabClosed });
_revokers.SetTaskbarProgress = _logic.SetTaskbarProgress(winrt::auto_revoke, { this, &AppHost::SetTaskbarProgress });
_revokers.IdentifyWindowsRequested = _logic.IdentifyWindowsRequested(winrt::auto_revoke, { this, &AppHost::_IdentifyWindowsRequested });
_revokers.RenameWindowRequested = _logic.RenameWindowRequested(winrt::auto_revoke, { this, &AppHost::_RenameWindowRequested });
_revokers.SettingsChanged = _logic.SettingsChanged(winrt::auto_revoke, { this, &AppHost::_HandleSettingsChanged });
_revokers.IsQuakeWindowChanged = _logic.IsQuakeWindowChanged(winrt::auto_revoke, { this, &AppHost::_IsQuakeWindowChanged });
_revokers.SummonWindowRequested = _logic.SummonWindowRequested(winrt::auto_revoke, { this, &AppHost::_SummonWindowRequested });
_revokers.OpenSystemMenu = _logic.OpenSystemMenu(winrt::auto_revoke, { this, &AppHost::_OpenSystemMenu });
_revokers.QuitRequested = _logic.QuitRequested(winrt::auto_revoke, { this, &AppHost::_RequestQuitAll });

_window->UpdateTitle(_logic.Title());

Expand Down Expand Up @@ -424,7 +458,7 @@ void AppHost::LastTabClosed(const winrt::Windows::Foundation::IInspectable& /*se
}
else if (_window->IsQuakeWindow())
{
_HideNotificationIconRequested();
_HideNotificationIconRequested(nullptr, nullptr);
}

// We don't want to try to save layouts if we are about to close.
Expand Down Expand Up @@ -690,6 +724,15 @@ void AppHost::_ChangeMaximizeRequested(const winrt::Windows::Foundation::IInspec
void AppHost::_AlwaysOnTopChanged(const winrt::Windows::Foundation::IInspectable&,
const winrt::Windows::Foundation::IInspectable&)
{
// MSFT:34662459
//
// Although we're manually revoking the event handler now in the dtor before
// we null out the window, let's be extra careful and check JUST IN CASE.
if (_window == nullptr)
{
return;
}

_window->SetAlwaysOnTop(_logic.AlwaysOnTop());
}

Expand Down Expand Up @@ -852,6 +895,15 @@ winrt::fire_and_forget AppHost::_WindowActivated()
void AppHost::_BecomeMonarch(const winrt::Windows::Foundation::IInspectable& /*sender*/,
const winrt::Windows::Foundation::IInspectable& /*args*/)
{
// MSFT:35726322
//
// Although we're manually revoking the event handler now in the dtor before
// we null out the window, let's be extra careful and check JUST IN CASE.
if (_window == nullptr)
{
return;
}

_setupGlobalHotkeys();

if (_windowManager.DoesQuakeWindowExist() ||
Expand Down Expand Up @@ -880,11 +932,11 @@ void AppHost::_BecomeMonarch(const winrt::Windows::Foundation::IInspectable& /*s
});

// These events are coming from peasants that become or un-become quake windows.
_windowManager.ShowNotificationIconRequested([this](auto&&, auto&&) { _ShowNotificationIconRequested(); });
_windowManager.HideNotificationIconRequested([this](auto&&, auto&&) { _HideNotificationIconRequested(); });
_revokers.ShowNotificationIconRequested = _windowManager.ShowNotificationIconRequested(winrt::auto_revoke, { this, &AppHost::_ShowNotificationIconRequested });
_revokers.HideNotificationIconRequested = _windowManager.HideNotificationIconRequested(winrt::auto_revoke, { this, &AppHost::_HideNotificationIconRequested });
// If the monarch receives a QuitAll event it will signal this event to be
// ran before each peasant is closed.
_windowManager.QuitAllRequested({ this, &AppHost::_QuitAllRequested });
_revokers.QuitAllRequested = _windowManager.QuitAllRequested(winrt::auto_revoke, { this, &AppHost::_QuitAllRequested });

// The monarch should be monitoring if it should save the window layout.
if (!_getWindowLayoutThrottler.has_value())
Expand Down Expand Up @@ -967,6 +1019,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)
{
// MSFT:36797001 There's a surprising number of hits of this callback
// getting triggered during teardown. As a best practice, we really
// should make sure _window exists before accessing it on any coroutine.
// We might be getting called back after the app already began getting
// cleaned up.
co_return;
}
// Unregister all previously registered hotkeys.
//
// RegisterHotKey(), will not unregister hotkeys automatically.
Expand Down Expand Up @@ -1279,11 +1340,11 @@ void AppHost::_IsQuakeWindowChanged(const winrt::Windows::Foundation::IInspectab
// specifically for the quake window. If not, it should not be destroyed.
if (!_window->IsQuakeWindow() && _logic.IsQuakeWindow())
{
_ShowNotificationIconRequested();
_ShowNotificationIconRequested(nullptr, nullptr);
}
else if (_window->IsQuakeWindow() && !_logic.IsQuakeWindow())
{
_HideNotificationIconRequested();
_HideNotificationIconRequested(nullptr, nullptr);
}

_window->IsQuakeWindow(_logic.IsQuakeWindow());
Expand Down Expand Up @@ -1392,7 +1453,8 @@ void AppHost::_DestroyNotificationIcon()
_notificationIcon = nullptr;
}

void AppHost::_ShowNotificationIconRequested()
void AppHost::_ShowNotificationIconRequested(const winrt::Windows::Foundation::IInspectable& /*sender*/,
const winrt::Windows::Foundation::IInspectable& /*args*/)
{
if (_windowManager.IsMonarch())
{
Expand All @@ -1407,7 +1469,8 @@ void AppHost::_ShowNotificationIconRequested()
}
}

void AppHost::_HideNotificationIconRequested()
void AppHost::_HideNotificationIconRequested(const winrt::Windows::Foundation::IInspectable& /*sender*/,
const winrt::Windows::Foundation::IInspectable& /*args*/)
{
if (_windowManager.IsMonarch())
{
Expand Down Expand Up @@ -1466,3 +1529,10 @@ void AppHost::_WindowMoved()
}
}
}

void AppHost::_CloseRequested(const winrt::Windows::Foundation::IInspectable& /*sender*/,
const winrt::Windows::Foundation::IInspectable& /*args*/)
{
const auto pos = _GetWindowLaunchPosition();
_logic.CloseWindow(pos);
}
43 changes: 41 additions & 2 deletions src/cascadia/WindowsTerminal/AppHost.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,14 +106,18 @@ class AppHost

void _RequestQuitAll(const winrt::Windows::Foundation::IInspectable& sender,
const winrt::Windows::Foundation::IInspectable& args);
void _CloseRequested(const winrt::Windows::Foundation::IInspectable& sender,
const winrt::Windows::Foundation::IInspectable& args);

void _QuitAllRequested(const winrt::Windows::Foundation::IInspectable& sender,
const winrt::Microsoft::Terminal::Remoting::QuitAllRequestedArgs& args);

void _CreateNotificationIcon();
void _DestroyNotificationIcon();
void _ShowNotificationIconRequested();
void _HideNotificationIconRequested();
void _ShowNotificationIconRequested(const winrt::Windows::Foundation::IInspectable& sender,
const winrt::Windows::Foundation::IInspectable& args);
void _HideNotificationIconRequested(const winrt::Windows::Foundation::IInspectable& sender,
const winrt::Windows::Foundation::IInspectable& args);
std::unique_ptr<NotificationIcon> _notificationIcon;
winrt::event_token _ReAddNotificationIconToken;
winrt::event_token _NotificationIconPressedToken;
Expand All @@ -122,4 +126,39 @@ class AppHost
winrt::event_token _GetWindowLayoutRequestedToken;
winrt::event_token _WindowCreatedToken;
winrt::event_token _WindowClosedToken;

// Helper struct. By putting these all into one struct, we can revoke them
// all at once, by assigning _revokers to a fresh Revokers instance. That'll
// cause us to dtor the old one, which will immediately call revoke on all
// the members as a part of their own dtors.
struct Revokers
{
// Event handlers to revoke in ~AppHost, before calling App.Close
winrt::Microsoft::Terminal::Remoting::WindowManager::BecameMonarch_revoker BecameMonarch;
winrt::Microsoft::Terminal::Remoting::Peasant::ExecuteCommandlineRequested_revoker peasantExecuteCommandlineRequested;
winrt::Microsoft::Terminal::Remoting::Peasant::SummonRequested_revoker peasantSummonRequested;
winrt::Microsoft::Terminal::Remoting::Peasant::DisplayWindowIdRequested_revoker peasantDisplayWindowIdRequested;
winrt::Microsoft::Terminal::Remoting::Peasant::QuitRequested_revoker peasantQuitRequested;
winrt::TerminalApp::AppLogic::CloseRequested_revoker CloseRequested;
winrt::TerminalApp::AppLogic::RequestedThemeChanged_revoker RequestedThemeChanged;
winrt::TerminalApp::AppLogic::FullscreenChanged_revoker FullscreenChanged;
winrt::TerminalApp::AppLogic::FocusModeChanged_revoker FocusModeChanged;
winrt::TerminalApp::AppLogic::AlwaysOnTopChanged_revoker AlwaysOnTopChanged;
winrt::TerminalApp::AppLogic::RaiseVisualBell_revoker RaiseVisualBell;
winrt::TerminalApp::AppLogic::SystemMenuChangeRequested_revoker SystemMenuChangeRequested;
winrt::TerminalApp::AppLogic::ChangeMaximizeRequested_revoker ChangeMaximizeRequested;
winrt::TerminalApp::AppLogic::TitleChanged_revoker TitleChanged;
winrt::TerminalApp::AppLogic::LastTabClosed_revoker LastTabClosed;
winrt::TerminalApp::AppLogic::SetTaskbarProgress_revoker SetTaskbarProgress;
winrt::TerminalApp::AppLogic::IdentifyWindowsRequested_revoker IdentifyWindowsRequested;
winrt::TerminalApp::AppLogic::RenameWindowRequested_revoker RenameWindowRequested;
winrt::TerminalApp::AppLogic::SettingsChanged_revoker SettingsChanged;
winrt::TerminalApp::AppLogic::IsQuakeWindowChanged_revoker IsQuakeWindowChanged;
winrt::TerminalApp::AppLogic::SummonWindowRequested_revoker SummonWindowRequested;
winrt::TerminalApp::AppLogic::OpenSystemMenu_revoker OpenSystemMenu;
winrt::TerminalApp::AppLogic::QuitRequested_revoker QuitRequested;
winrt::Microsoft::Terminal::Remoting::WindowManager::ShowNotificationIconRequested_revoker ShowNotificationIconRequested;
winrt::Microsoft::Terminal::Remoting::WindowManager::HideNotificationIconRequested_revoker HideNotificationIconRequested;
winrt::Microsoft::Terminal::Remoting::WindowManager::QuitAllRequested_revoker QuitAllRequested;
} _revokers{};
};

0 comments on commit 8962c75

Please sign in to comment.