Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix a crash setting the hotkey during teardown #12580

Merged
5 commits merged into from
Mar 1, 2022
Merged

Fix a crash setting the hotkey during teardown #12580

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f13c81e
Fix a crash setting the hotkey during teardown
zadjii-msft Feb 25, 2022
94e3963
manually revoke every handler
zadjii-msft Mar 1, 2022
b1d830f
cleanup
zadjii-msft Mar 1, 2022
22c6a2f
manually
zadjii-msft Mar 1, 2022
d41f7b4
this is clever
zadjii-msft Mar 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
146 changes: 108 additions & 38 deletions src/cascadia/WindowsTerminal/AppHost.cpp
View file
Open in desktop
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah we null it out properly too? Awesome

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
// 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
Open in desktop
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{};
};