From 8c2c88e44fa20633be0ed816012de33bd38449cc Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Tue, 20 Aug 2024 13:24:51 +0200 Subject: [PATCH] Fix session persistence when the session ends (#17714) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Once all applications that have received a `WM_ENDSESSION` message have returned from processing said message, windows will terminate all processes. This forces us to process the message synchronously. This meant that this issue was timing dependent. If Windows Terminal was quick at persisting buffers and you had some other application that was slow to shut down (e.g. Steam), you would never see this issue. Closes #17179 Closes #17250 ## Validation Steps Performed * Set up a lean Hyper-V VM for fast reboots * `Set-VMComPort 1 \\.pipe\\` * Hook up WIL to write to COM1 * Add a ton of debug prints all over the place * Read COM output with Putty for hours * RTFM, and notice that the `WM_ENDSESSION` documentation states "the session can end any time after all applications have returned from processing this message" * Be very very sad ✅ * Fix it * Rebooting now shows on COM1 that persistence runs ✅ * Windows get restored after reboot ✅ (cherry picked from commit b439925acc1f6c51180b2fa33775395b595d1c60) Service-Card-Id: PVTI_lADOAF3p4s4AmhmszgSGCik Service-Version: 1.21 --- src/cascadia/Remoting/Monarch.cpp | 9 +-- src/cascadia/Remoting/Monarch.h | 1 + src/cascadia/Remoting/Monarch.idl | 1 + src/cascadia/Remoting/Peasant.cpp | 16 ----- src/cascadia/Remoting/Peasant.h | 2 - src/cascadia/Remoting/Peasant.idl | 2 - src/cascadia/Remoting/WindowManager.cpp | 12 ++++ src/cascadia/Remoting/WindowManager.h | 1 + src/cascadia/Remoting/WindowManager.idl | 1 + .../UnitTests_Remoting/RemotingTests.cpp | 3 +- src/cascadia/WindowsTerminal/AppHost.cpp | 67 +++++++++++++------ src/cascadia/WindowsTerminal/AppHost.h | 4 +- 12 files changed, 68 insertions(+), 51 deletions(-) diff --git a/src/cascadia/Remoting/Monarch.cpp b/src/cascadia/Remoting/Monarch.cpp index 63e74b0fae..b986bfc863 100644 --- a/src/cascadia/Remoting/Monarch.cpp +++ b/src/cascadia/Remoting/Monarch.cpp @@ -96,7 +96,6 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation peasant.ShowNotificationIconRequested([this](auto&&, auto&&) { ShowNotificationIconRequested.raise(*this, nullptr); }); peasant.HideNotificationIconRequested([this](auto&&, auto&&) { HideNotificationIconRequested.raise(*this, nullptr); }); - peasant.QuitAllRequested({ this, &Monarch::_handleQuitAll }); { std::unique_lock lock{ _peasantsMutex }; @@ -134,7 +133,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation // - used // Return Value: // - - void Monarch::_handleQuitAll(const winrt::Windows::Foundation::IInspectable& /*sender*/, const winrt::Windows::Foundation::IInspectable& /*args*/) + void Monarch::QuitAll() { if (_quitting.exchange(true, std::memory_order_relaxed)) { @@ -166,12 +165,6 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation { peasantSearch->second.Quit(); } - else - { - // Somehow we don't have our own peasant, this should never happen. - // We are trying to quit anyways so just fail here. - assert(peasantSearch != _peasants.end()); - } } } diff --git a/src/cascadia/Remoting/Monarch.h b/src/cascadia/Remoting/Monarch.h index 299932d961..4cc74d0c5b 100644 --- a/src/cascadia/Remoting/Monarch.h +++ b/src/cascadia/Remoting/Monarch.h @@ -86,6 +86,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation uint64_t AddPeasant(winrt::Microsoft::Terminal::Remoting::IPeasant peasant); void SignalClose(const uint64_t peasantId); + void QuitAll(); uint64_t GetNumberOfPeasants(); diff --git a/src/cascadia/Remoting/Monarch.idl b/src/cascadia/Remoting/Monarch.idl index c81837e96b..ba00cf47cf 100644 --- a/src/cascadia/Remoting/Monarch.idl +++ b/src/cascadia/Remoting/Monarch.idl @@ -63,6 +63,7 @@ namespace Microsoft.Terminal.Remoting void HandleActivatePeasant(WindowActivatedArgs args); void SummonWindow(SummonWindowSelectionArgs args); void SignalClose(UInt64 peasantId); + void QuitAll(); void SummonAllWindows(); Boolean DoesQuakeWindowExist(); diff --git a/src/cascadia/Remoting/Peasant.cpp b/src/cascadia/Remoting/Peasant.cpp index 6378854d19..e09f5ac67e 100644 --- a/src/cascadia/Remoting/Peasant.cpp +++ b/src/cascadia/Remoting/Peasant.cpp @@ -260,22 +260,6 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation TraceLoggingKeyword(TIL_KEYWORD_TRACE)); } - void Peasant::RequestQuitAll() - { - try - { - QuitAllRequested.raise(*this, nullptr); - } - catch (...) - { - LOG_CAUGHT_EXCEPTION(); - } - TraceLoggingWrite(g_hRemotingProvider, - "Peasant_RequestQuit", - TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), - TraceLoggingKeyword(TIL_KEYWORD_TRACE)); - } - void Peasant::AttachContentToWindow(Remoting::AttachRequest request) { try diff --git a/src/cascadia/Remoting/Peasant.h b/src/cascadia/Remoting/Peasant.h index dc860ba907..1291c8287b 100644 --- a/src/cascadia/Remoting/Peasant.h +++ b/src/cascadia/Remoting/Peasant.h @@ -56,7 +56,6 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation void RequestRename(const winrt::Microsoft::Terminal::Remoting::RenameRequestArgs& args); void RequestShowNotificationIcon(); void RequestHideNotificationIcon(); - void RequestQuitAll(); void Quit(); void AttachContentToWindow(Remoting::AttachRequest request); @@ -76,7 +75,6 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation til::typed_event<> ShowNotificationIconRequested; til::typed_event<> HideNotificationIconRequested; - til::typed_event<> QuitAllRequested; til::typed_event<> QuitRequested; til::typed_event AttachRequested; diff --git a/src/cascadia/Remoting/Peasant.idl b/src/cascadia/Remoting/Peasant.idl index 3463d561f0..3e01e6a6d3 100644 --- a/src/cascadia/Remoting/Peasant.idl +++ b/src/cascadia/Remoting/Peasant.idl @@ -80,7 +80,6 @@ namespace Microsoft.Terminal.Remoting void RequestShowNotificationIcon(); void RequestHideNotificationIcon(); - void RequestQuitAll(); void Quit(); void AttachContentToWindow(AttachRequest request); @@ -95,7 +94,6 @@ namespace Microsoft.Terminal.Remoting event Windows.Foundation.TypedEventHandler ShowNotificationIconRequested; event Windows.Foundation.TypedEventHandler HideNotificationIconRequested; - event Windows.Foundation.TypedEventHandler QuitAllRequested; event Windows.Foundation.TypedEventHandler QuitRequested; event Windows.Foundation.TypedEventHandler AttachRequested; diff --git a/src/cascadia/Remoting/WindowManager.cpp b/src/cascadia/Remoting/WindowManager.cpp index 88c3a0218d..a07a3960b4 100644 --- a/src/cascadia/Remoting/WindowManager.cpp +++ b/src/cascadia/Remoting/WindowManager.cpp @@ -375,6 +375,18 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation WindowClosed.raise(s, e); } + void WindowManager::QuitAll() + { + if (_monarch) + { + try + { + _monarch.QuitAll(); + } + CATCH_LOG() + } + } + void WindowManager::SignalClose(const Remoting::Peasant& peasant) { if (_monarch) diff --git a/src/cascadia/Remoting/WindowManager.h b/src/cascadia/Remoting/WindowManager.h index 6dd935e8a6..936e13bc68 100644 --- a/src/cascadia/Remoting/WindowManager.h +++ b/src/cascadia/Remoting/WindowManager.h @@ -32,6 +32,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation Remoting::Peasant CreatePeasant(const Remoting::WindowRequestedArgs& args); void SignalClose(const Remoting::Peasant& peasant); + void QuitAll(); void SummonWindow(const Remoting::SummonWindowSelectionArgs& args); void SummonAllWindows(); Windows::Foundation::Collections::IVectorView GetPeasantInfos(); diff --git a/src/cascadia/Remoting/WindowManager.idl b/src/cascadia/Remoting/WindowManager.idl index deaf2b7489..1f1b4c4345 100644 --- a/src/cascadia/Remoting/WindowManager.idl +++ b/src/cascadia/Remoting/WindowManager.idl @@ -12,6 +12,7 @@ namespace Microsoft.Terminal.Remoting Peasant CreatePeasant(WindowRequestedArgs args); void SignalClose(Peasant p); + void QuitAll(); void UpdateActiveTabTitle(String title, Peasant p); diff --git a/src/cascadia/UnitTests_Remoting/RemotingTests.cpp b/src/cascadia/UnitTests_Remoting/RemotingTests.cpp index 8fc12f493d..a790b8db41 100644 --- a/src/cascadia/UnitTests_Remoting/RemotingTests.cpp +++ b/src/cascadia/UnitTests_Remoting/RemotingTests.cpp @@ -82,7 +82,6 @@ namespace RemotingUnitTests void Summon(const Remoting::SummonWindowBehavior& /*args*/) DIE; void RequestShowNotificationIcon() DIE; void RequestHideNotificationIcon() DIE; - void RequestQuitAll() DIE; void Quit() DIE; void AttachContentToWindow(Remoting::AttachRequest) DIE; void SendContent(winrt::Microsoft::Terminal::Remoting::RequestReceiveContentArgs) DIE; @@ -94,7 +93,6 @@ namespace RemotingUnitTests til::typed_event SummonRequested; til::typed_event<> ShowNotificationIconRequested; til::typed_event<> HideNotificationIconRequested; - til::typed_event<> QuitAllRequested; til::typed_event<> QuitRequested; til::typed_event AttachRequested; til::typed_event SendContentRequested; @@ -111,6 +109,7 @@ namespace RemotingUnitTests void HandleActivatePeasant(Remoting::WindowActivatedArgs /*args*/) DIE; void SummonWindow(Remoting::SummonWindowSelectionArgs /*args*/) DIE; void SignalClose(uint64_t /*peasantId*/) DIE; + void QuitAll() DIE; void SummonAllWindows() DIE; bool DoesQuakeWindowExist() DIE; diff --git a/src/cascadia/WindowsTerminal/AppHost.cpp b/src/cascadia/WindowsTerminal/AppHost.cpp index 63d8de231d..98eb04619c 100644 --- a/src/cascadia/WindowsTerminal/AppHost.cpp +++ b/src/cascadia/WindowsTerminal/AppHost.cpp @@ -13,6 +13,8 @@ #include +#include + using namespace winrt::Windows::UI; using namespace winrt::Windows::UI::Composition; using namespace winrt::Windows::UI::Xaml; @@ -348,9 +350,29 @@ void AppHost::Initialize() }); _windowCallbacks.AutomaticShutdownRequested = _window->AutomaticShutdownRequested([this]() { - // Raised when the OS is beginning an update of the app. We will quit, - // to save our state, before the OS manually kills us. - _quit(); + // This is the WM_ENDSESSION handler. + // The event is raised when the user is logged out, because the system is rebooting, etc. + // Due to the design of WM_ENDSESSION, returning from the message indicates to the OS that it's fine to + // terminate us at any time. Luckily Windows has never heavily relied on message passing or asynchronous + // eventing in any of its UI frameworks. It also was clearly impossible to use WaitForMultipleObjects with + // bWaitAll=TRUE and a timeout to wait for all applications to exit cleanly. + // As such we attempt to synchronously shut down the app here. Otherwise, it could just call _quit(). + + const auto state = ApplicationState::SharedInstance(); + + state.PersistedWindowLayouts(nullptr); + + // A duplicate of AppHost::_QuitRequested(). + if (_appLogic && _windowLogic && _appLogic.ShouldUsePersistedLayout()) + { + _windowLogic.PersistState(); + } + + _windowManager.SignalClose(_peasant); + _windowManager.QuitAll(); + + // Ensure to write the state.json before we get TerminateProcess()d by the OS (Thanks!). + state.Flush(); }); // Load bearing: make sure the PropertyChanged handler is added before we @@ -440,7 +462,7 @@ winrt::fire_and_forget AppHost::_quit() co_await winrt::resume_background(); ApplicationState::SharedInstance().PersistedWindowLayouts(nullptr); - peasant.RequestQuitAll(); + _windowManager.QuitAll(); } void AppHost::_revokeWindowCallbacks() @@ -1176,25 +1198,32 @@ void AppHost::_IsQuakeWindowChanged(const winrt::Windows::Foundation::IInspectab } // Raised from our Peasant. We handle by propagating the call to our terminal window. -winrt::fire_and_forget AppHost::_QuitRequested(const winrt::Windows::Foundation::IInspectable&, - const winrt::Windows::Foundation::IInspectable&) +void AppHost::_QuitRequested(const winrt::Windows::Foundation::IInspectable&, const winrt::Windows::Foundation::IInspectable&) { - auto weakThis{ weak_from_this() }; - // Need to be on the main thread to close out all of the tabs. - co_await wil::resume_foreground(_windowLogic.GetRoot().Dispatcher()); + // We process the shutdown synchronously here, because otherwise the + // AutomaticShutdownRequested() logic wouldn't run synchronously either. + til::latch latch{ 1 }; - const auto strongThis = weakThis.lock(); - if (!strongThis) - { - co_return; - } + _windowLogic.GetRoot().Dispatcher().RunAsync(winrt::Windows::UI::Core::CoreDispatcherPriority::Normal, [&latch, weakThis = weak_from_this()]() { + const auto countDownOnExit = wil::scope_exit([&latch] { + latch.count_down(); + }); - if (_appLogic && _windowLogic && _appLogic.ShouldUsePersistedLayout()) - { - _windowLogic.PersistState(); - } + const auto self = weakThis.lock(); + if (!self) + { + return; + } - PostQuitMessage(0); + if (self->_appLogic && self->_windowLogic && self->_appLogic.ShouldUsePersistedLayout()) + { + self->_windowLogic.PersistState(); + } + + PostQuitMessage(0); + }); + + latch.wait(); } // Raised from TerminalWindow. We handle by bubbling the request to the window manager. diff --git a/src/cascadia/WindowsTerminal/AppHost.h b/src/cascadia/WindowsTerminal/AppHost.h index cc90c21c1c..1d3331d3ba 100644 --- a/src/cascadia/WindowsTerminal/AppHost.h +++ b/src/cascadia/WindowsTerminal/AppHost.h @@ -123,8 +123,8 @@ private: void _SystemMenuChangeRequested(const winrt::Windows::Foundation::IInspectable& sender, const winrt::TerminalApp::SystemMenuChangeArgs& args); - winrt::fire_and_forget _QuitRequested(const winrt::Windows::Foundation::IInspectable& sender, - const winrt::Windows::Foundation::IInspectable& args); + void _QuitRequested(const winrt::Windows::Foundation::IInspectable& sender, + const winrt::Windows::Foundation::IInspectable& args); void _RequestQuitAll(const winrt::Windows::Foundation::IInspectable& sender, const winrt::Windows::Foundation::IInspectable& args);