Skip to content

Commit 8c2c88e

Browse files
lheckerDHowett
authored andcommitted
Fix session persistence when the session ends (#17714)
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 <vm> 1 \\.pipe\\<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 b439925) Service-Card-Id: PVTI_lADOAF3p4s4AmhmszgSGCik Service-Version: 1.21
1 parent 1742dc0 commit 8c2c88e

File tree

12 files changed

+68
-51
lines changed

12 files changed

+68
-51
lines changed

src/cascadia/Remoting/Monarch.cpp

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,6 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
9696

9797
peasant.ShowNotificationIconRequested([this](auto&&, auto&&) { ShowNotificationIconRequested.raise(*this, nullptr); });
9898
peasant.HideNotificationIconRequested([this](auto&&, auto&&) { HideNotificationIconRequested.raise(*this, nullptr); });
99-
peasant.QuitAllRequested({ this, &Monarch::_handleQuitAll });
10099

101100
{
102101
std::unique_lock lock{ _peasantsMutex };
@@ -134,7 +133,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
134133
// - <none> used
135134
// Return Value:
136135
// - <none>
137-
void Monarch::_handleQuitAll(const winrt::Windows::Foundation::IInspectable& /*sender*/, const winrt::Windows::Foundation::IInspectable& /*args*/)
136+
void Monarch::QuitAll()
138137
{
139138
if (_quitting.exchange(true, std::memory_order_relaxed))
140139
{
@@ -166,12 +165,6 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
166165
{
167166
peasantSearch->second.Quit();
168167
}
169-
else
170-
{
171-
// Somehow we don't have our own peasant, this should never happen.
172-
// We are trying to quit anyways so just fail here.
173-
assert(peasantSearch != _peasants.end());
174-
}
175168
}
176169
}
177170

src/cascadia/Remoting/Monarch.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
8686

8787
uint64_t AddPeasant(winrt::Microsoft::Terminal::Remoting::IPeasant peasant);
8888
void SignalClose(const uint64_t peasantId);
89+
void QuitAll();
8990

9091
uint64_t GetNumberOfPeasants();
9192

src/cascadia/Remoting/Monarch.idl

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,7 @@ namespace Microsoft.Terminal.Remoting
6363
void HandleActivatePeasant(WindowActivatedArgs args);
6464
void SummonWindow(SummonWindowSelectionArgs args);
6565
void SignalClose(UInt64 peasantId);
66+
void QuitAll();
6667

6768
void SummonAllWindows();
6869
Boolean DoesQuakeWindowExist();

src/cascadia/Remoting/Peasant.cpp

Lines changed: 0 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -260,22 +260,6 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
260260
TraceLoggingKeyword(TIL_KEYWORD_TRACE));
261261
}
262262

263-
void Peasant::RequestQuitAll()
264-
{
265-
try
266-
{
267-
QuitAllRequested.raise(*this, nullptr);
268-
}
269-
catch (...)
270-
{
271-
LOG_CAUGHT_EXCEPTION();
272-
}
273-
TraceLoggingWrite(g_hRemotingProvider,
274-
"Peasant_RequestQuit",
275-
TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE),
276-
TraceLoggingKeyword(TIL_KEYWORD_TRACE));
277-
}
278-
279263
void Peasant::AttachContentToWindow(Remoting::AttachRequest request)
280264
{
281265
try

src/cascadia/Remoting/Peasant.h

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,6 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
5656
void RequestRename(const winrt::Microsoft::Terminal::Remoting::RenameRequestArgs& args);
5757
void RequestShowNotificationIcon();
5858
void RequestHideNotificationIcon();
59-
void RequestQuitAll();
6059
void Quit();
6160

6261
void AttachContentToWindow(Remoting::AttachRequest request);
@@ -76,7 +75,6 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
7675

7776
til::typed_event<> ShowNotificationIconRequested;
7877
til::typed_event<> HideNotificationIconRequested;
79-
til::typed_event<> QuitAllRequested;
8078
til::typed_event<> QuitRequested;
8179

8280
til::typed_event<winrt::Windows::Foundation::IInspectable, winrt::Microsoft::Terminal::Remoting::AttachRequest> AttachRequested;

src/cascadia/Remoting/Peasant.idl

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,6 @@ namespace Microsoft.Terminal.Remoting
8080

8181
void RequestShowNotificationIcon();
8282
void RequestHideNotificationIcon();
83-
void RequestQuitAll();
8483
void Quit();
8584

8685
void AttachContentToWindow(AttachRequest request);
@@ -95,7 +94,6 @@ namespace Microsoft.Terminal.Remoting
9594

9695
event Windows.Foundation.TypedEventHandler<Object, Object> ShowNotificationIconRequested;
9796
event Windows.Foundation.TypedEventHandler<Object, Object> HideNotificationIconRequested;
98-
event Windows.Foundation.TypedEventHandler<Object, Object> QuitAllRequested;
9997
event Windows.Foundation.TypedEventHandler<Object, Object> QuitRequested;
10098

10199
event Windows.Foundation.TypedEventHandler<Object, AttachRequest> AttachRequested;

src/cascadia/Remoting/WindowManager.cpp

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -375,6 +375,18 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
375375
WindowClosed.raise(s, e);
376376
}
377377

378+
void WindowManager::QuitAll()
379+
{
380+
if (_monarch)
381+
{
382+
try
383+
{
384+
_monarch.QuitAll();
385+
}
386+
CATCH_LOG()
387+
}
388+
}
389+
378390
void WindowManager::SignalClose(const Remoting::Peasant& peasant)
379391
{
380392
if (_monarch)

src/cascadia/Remoting/WindowManager.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
3232
Remoting::Peasant CreatePeasant(const Remoting::WindowRequestedArgs& args);
3333

3434
void SignalClose(const Remoting::Peasant& peasant);
35+
void QuitAll();
3536
void SummonWindow(const Remoting::SummonWindowSelectionArgs& args);
3637
void SummonAllWindows();
3738
Windows::Foundation::Collections::IVectorView<winrt::Microsoft::Terminal::Remoting::PeasantInfo> GetPeasantInfos();

src/cascadia/Remoting/WindowManager.idl

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ namespace Microsoft.Terminal.Remoting
1212
Peasant CreatePeasant(WindowRequestedArgs args);
1313

1414
void SignalClose(Peasant p);
15+
void QuitAll();
1516

1617
void UpdateActiveTabTitle(String title, Peasant p);
1718

src/cascadia/UnitTests_Remoting/RemotingTests.cpp

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,6 @@ namespace RemotingUnitTests
8282
void Summon(const Remoting::SummonWindowBehavior& /*args*/) DIE;
8383
void RequestShowNotificationIcon() DIE;
8484
void RequestHideNotificationIcon() DIE;
85-
void RequestQuitAll() DIE;
8685
void Quit() DIE;
8786
void AttachContentToWindow(Remoting::AttachRequest) DIE;
8887
void SendContent(winrt::Microsoft::Terminal::Remoting::RequestReceiveContentArgs) DIE;
@@ -94,7 +93,6 @@ namespace RemotingUnitTests
9493
til::typed_event<winrt::Windows::Foundation::IInspectable, Remoting::SummonWindowBehavior> SummonRequested;
9594
til::typed_event<> ShowNotificationIconRequested;
9695
til::typed_event<> HideNotificationIconRequested;
97-
til::typed_event<> QuitAllRequested;
9896
til::typed_event<> QuitRequested;
9997
til::typed_event<winrt::Windows::Foundation::IInspectable, winrt::Microsoft::Terminal::Remoting::AttachRequest> AttachRequested;
10098
til::typed_event<winrt::Windows::Foundation::IInspectable, winrt::Microsoft::Terminal::Remoting::RequestReceiveContentArgs> SendContentRequested;
@@ -111,6 +109,7 @@ namespace RemotingUnitTests
111109
void HandleActivatePeasant(Remoting::WindowActivatedArgs /*args*/) DIE;
112110
void SummonWindow(Remoting::SummonWindowSelectionArgs /*args*/) DIE;
113111
void SignalClose(uint64_t /*peasantId*/) DIE;
112+
void QuitAll() DIE;
114113

115114
void SummonAllWindows() DIE;
116115
bool DoesQuakeWindowExist() DIE;

src/cascadia/WindowsTerminal/AppHost.cpp

Lines changed: 48 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,8 @@
1313

1414
#include <TerminalThemeHelpers.h>
1515

16+
#include <til/latch.h>
17+
1618
using namespace winrt::Windows::UI;
1719
using namespace winrt::Windows::UI::Composition;
1820
using namespace winrt::Windows::UI::Xaml;
@@ -348,9 +350,29 @@ void AppHost::Initialize()
348350
});
349351

350352
_windowCallbacks.AutomaticShutdownRequested = _window->AutomaticShutdownRequested([this]() {
351-
// Raised when the OS is beginning an update of the app. We will quit,
352-
// to save our state, before the OS manually kills us.
353-
_quit();
353+
// This is the WM_ENDSESSION handler.
354+
// The event is raised when the user is logged out, because the system is rebooting, etc.
355+
// Due to the design of WM_ENDSESSION, returning from the message indicates to the OS that it's fine to
356+
// terminate us at any time. Luckily Windows has never heavily relied on message passing or asynchronous
357+
// eventing in any of its UI frameworks. It also was clearly impossible to use WaitForMultipleObjects with
358+
// bWaitAll=TRUE and a timeout to wait for all applications to exit cleanly.
359+
// As such we attempt to synchronously shut down the app here. Otherwise, it could just call _quit().
360+
361+
const auto state = ApplicationState::SharedInstance();
362+
363+
state.PersistedWindowLayouts(nullptr);
364+
365+
// A duplicate of AppHost::_QuitRequested().
366+
if (_appLogic && _windowLogic && _appLogic.ShouldUsePersistedLayout())
367+
{
368+
_windowLogic.PersistState();
369+
}
370+
371+
_windowManager.SignalClose(_peasant);
372+
_windowManager.QuitAll();
373+
374+
// Ensure to write the state.json before we get TerminateProcess()d by the OS (Thanks!).
375+
state.Flush();
354376
});
355377

356378
// Load bearing: make sure the PropertyChanged handler is added before we
@@ -440,7 +462,7 @@ winrt::fire_and_forget AppHost::_quit()
440462
co_await winrt::resume_background();
441463

442464
ApplicationState::SharedInstance().PersistedWindowLayouts(nullptr);
443-
peasant.RequestQuitAll();
465+
_windowManager.QuitAll();
444466
}
445467

446468
void AppHost::_revokeWindowCallbacks()
@@ -1176,25 +1198,32 @@ void AppHost::_IsQuakeWindowChanged(const winrt::Windows::Foundation::IInspectab
11761198
}
11771199

11781200
// Raised from our Peasant. We handle by propagating the call to our terminal window.
1179-
winrt::fire_and_forget AppHost::_QuitRequested(const winrt::Windows::Foundation::IInspectable&,
1180-
const winrt::Windows::Foundation::IInspectable&)
1201+
void AppHost::_QuitRequested(const winrt::Windows::Foundation::IInspectable&, const winrt::Windows::Foundation::IInspectable&)
11811202
{
1182-
auto weakThis{ weak_from_this() };
1183-
// Need to be on the main thread to close out all of the tabs.
1184-
co_await wil::resume_foreground(_windowLogic.GetRoot().Dispatcher());
1203+
// We process the shutdown synchronously here, because otherwise the
1204+
// AutomaticShutdownRequested() logic wouldn't run synchronously either.
1205+
til::latch latch{ 1 };
11851206

1186-
const auto strongThis = weakThis.lock();
1187-
if (!strongThis)
1188-
{
1189-
co_return;
1190-
}
1207+
_windowLogic.GetRoot().Dispatcher().RunAsync(winrt::Windows::UI::Core::CoreDispatcherPriority::Normal, [&latch, weakThis = weak_from_this()]() {
1208+
const auto countDownOnExit = wil::scope_exit([&latch] {
1209+
latch.count_down();
1210+
});
11911211

1192-
if (_appLogic && _windowLogic && _appLogic.ShouldUsePersistedLayout())
1193-
{
1194-
_windowLogic.PersistState();
1195-
}
1212+
const auto self = weakThis.lock();
1213+
if (!self)
1214+
{
1215+
return;
1216+
}
1217+
1218+
if (self->_appLogic && self->_windowLogic && self->_appLogic.ShouldUsePersistedLayout())
1219+
{
1220+
self->_windowLogic.PersistState();
1221+
}
1222+
1223+
PostQuitMessage(0);
1224+
});
11961225

1197-
PostQuitMessage(0);
1226+
latch.wait();
11981227
}
11991228

12001229
// Raised from TerminalWindow. We handle by bubbling the request to the window manager.

src/cascadia/WindowsTerminal/AppHost.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -123,8 +123,8 @@ class AppHost : public std::enable_shared_from_this<AppHost>
123123
void _SystemMenuChangeRequested(const winrt::Windows::Foundation::IInspectable& sender,
124124
const winrt::TerminalApp::SystemMenuChangeArgs& args);
125125

126-
winrt::fire_and_forget _QuitRequested(const winrt::Windows::Foundation::IInspectable& sender,
127-
const winrt::Windows::Foundation::IInspectable& args);
126+
void _QuitRequested(const winrt::Windows::Foundation::IInspectable& sender,
127+
const winrt::Windows::Foundation::IInspectable& args);
128128

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

0 commit comments

Comments
 (0)