From e91fa758aa1ab568547ddfd6cd0ed3a3d107351d Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 16 Jan 2024 15:05:09 -0600 Subject: [PATCH 1/6] this is capital-B bodgy --- src/cascadia/WindowsTerminal/WindowEmperor.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/cascadia/WindowsTerminal/WindowEmperor.cpp b/src/cascadia/WindowsTerminal/WindowEmperor.cpp index 47a11bba38e..dc7d0473faf 100644 --- a/src/cascadia/WindowsTerminal/WindowEmperor.cpp +++ b/src/cascadia/WindowsTerminal/WindowEmperor.cpp @@ -56,8 +56,10 @@ WindowEmperor::WindowEmperor() noexcept : WindowEmperor::~WindowEmperor() { - _app.Close(); - _app = nullptr; + // _app.Close(); + // _app = nullptr; + // TerminateProcess(GetCurrentProcess(), 0); + std::exit(0); } void _buildArgsFromCommandline(std::vector& args) From 1ce5f133e693c76effd374f63c3094a2744b66e2 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 17 Jan 2024 05:52:18 -0600 Subject: [PATCH 2/6] fine --- src/cascadia/WindowsTerminal/WindowEmperor.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/cascadia/WindowsTerminal/WindowEmperor.cpp b/src/cascadia/WindowsTerminal/WindowEmperor.cpp index dc7d0473faf..20635350afc 100644 --- a/src/cascadia/WindowsTerminal/WindowEmperor.cpp +++ b/src/cascadia/WindowsTerminal/WindowEmperor.cpp @@ -54,6 +54,8 @@ WindowEmperor::WindowEmperor() noexcept : ::winrt::detach_abi(a); } +// Disable the "destructor never returns, potential memory leak" warning - we're literally already exiting here. +#pragma warning(suppress : 4722) WindowEmperor::~WindowEmperor() { // _app.Close(); From ad2ad9d4c197e68c3ee58a2ee03e6c012cd6a49d Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 19 Jan 2024 13:36:38 -0600 Subject: [PATCH 3/6] clean, add comments --- src/cascadia/WindowsTerminal/WindowEmperor.cpp | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/src/cascadia/WindowsTerminal/WindowEmperor.cpp b/src/cascadia/WindowsTerminal/WindowEmperor.cpp index 20635350afc..d6e7ecc9a81 100644 --- a/src/cascadia/WindowsTerminal/WindowEmperor.cpp +++ b/src/cascadia/WindowsTerminal/WindowEmperor.cpp @@ -54,13 +54,25 @@ WindowEmperor::WindowEmperor() noexcept : ::winrt::detach_abi(a); } -// Disable the "destructor never returns, potential memory leak" warning - we're literally already exiting here. +// Disable the "destructor never returns, potential memory leak" warning - we're literally already exiting here. #pragma warning(suppress : 4722) WindowEmperor::~WindowEmperor() { + // BODGY: If the emperor is being dtor'd, it's because we've gone past the + // end of main, and released the ref in main. Here, we want to manually + // terminate our process. + // + // If you just do a + // // _app.Close(); - // _app = nullptr; - // TerminateProcess(GetCurrentProcess(), 0); + // + // here, then we might run into an edge case where main releases it's ref to + // the emperor, but one of the window threads might be in the process of + // exiting, and still holding a strong ref to the emperor. In that case, we + // can actually end up with the _window thread_ being the last reference, + // and calling App::Close on that thread will crash us with a E_WRONG_THREAD + // + // For more context, see MSFT:46744208 std::exit(0); } From 8b1ffb16be22217ac2e7dd413d9b0f8a8ab0a9f5 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Tue, 23 Jan 2024 20:57:06 +0100 Subject: [PATCH 4/6] Use TerminateProcess --- src/cascadia/WindowsTerminal/AppHost.cpp | 3 +- .../WindowsTerminal/WindowEmperor.cpp | 57 +++---------------- src/cascadia/WindowsTerminal/WindowEmperor.h | 2 +- src/cascadia/WindowsTerminal/main.cpp | 5 +- 4 files changed, 13 insertions(+), 54 deletions(-) diff --git a/src/cascadia/WindowsTerminal/AppHost.cpp b/src/cascadia/WindowsTerminal/AppHost.cpp index 7cae9959998..73e228de277 100644 --- a/src/cascadia/WindowsTerminal/AppHost.cpp +++ b/src/cascadia/WindowsTerminal/AppHost.cpp @@ -166,7 +166,8 @@ void AppHost::_HandleCommandlineArgs(const Remoting::WindowRequestedArgs& window if (_windowLogic.ShouldExitEarly()) { - ExitThread(result); + TerminateProcess(GetCurrentProcess(), gsl::narrow_cast(result)); + __assume(false); } } } diff --git a/src/cascadia/WindowsTerminal/WindowEmperor.cpp b/src/cascadia/WindowsTerminal/WindowEmperor.cpp index d6e7ecc9a81..53cdcdd4434 100644 --- a/src/cascadia/WindowsTerminal/WindowEmperor.cpp +++ b/src/cascadia/WindowsTerminal/WindowEmperor.cpp @@ -38,42 +38,12 @@ WindowEmperor::WindowEmperor() noexcept : }); _dispatcher = winrt::Windows::System::DispatcherQueue::GetForCurrentThread(); - - // BODGY - // - // There's a mysterious crash in XAML on Windows 10 if you just let the App - // get dtor'd. By all accounts, it doesn't make sense. To mitigate this, we - // need to intentionally leak a reference to our App. Crazily, if you just - // let the app get cleaned up with the rest of the process when the process - // exits, then it doesn't crash. But if you let it get explicitly dtor'd, it - // absolutely will crash on exit. - // - // GH#15410 has more details. - - auto a{ _app }; - ::winrt::detach_abi(a); } -// Disable the "destructor never returns, potential memory leak" warning - we're literally already exiting here. -#pragma warning(suppress : 4722) WindowEmperor::~WindowEmperor() { - // BODGY: If the emperor is being dtor'd, it's because we've gone past the - // end of main, and released the ref in main. Here, we want to manually - // terminate our process. - // - // If you just do a - // - // _app.Close(); - // - // here, then we might run into an edge case where main releases it's ref to - // the emperor, but one of the window threads might be in the process of - // exiting, and still holding a strong ref to the emperor. In that case, we - // can actually end up with the _window thread_ being the last reference, - // and calling App::Close on that thread will crash us with a E_WRONG_THREAD - // - // For more context, see MSFT:46744208 - std::exit(0); + _app.Close(); + _app = nullptr; } void _buildArgsFromCommandline(std::vector& args) @@ -98,7 +68,7 @@ void _buildArgsFromCommandline(std::vector& args) } } -bool WindowEmperor::HandleCommandlineArgs() +void WindowEmperor::HandleCommandlineArgs() { std::vector args; _buildArgsFromCommandline(args); @@ -127,15 +97,14 @@ bool WindowEmperor::HandleCommandlineArgs() Remoting::CommandlineArgs eventArgs{ { args }, { cwd }, showWindow, winrt::hstring{ currentEnv.to_string() } }; const auto isolatedMode{ _app.Logic().IsolatedMode() }; - const auto result = _manager.ProposeCommandline(eventArgs, isolatedMode); + int exitCode = 0; - const bool makeWindow = result.ShouldCreateWindow(); - if (makeWindow) + if (result.ShouldCreateWindow()) { _createNewWindowThread(Remoting::WindowRequestedArgs{ result, eventArgs }); - _becomeMonarch(); + WaitForWindows(); } else { @@ -143,11 +112,12 @@ bool WindowEmperor::HandleCommandlineArgs() if (!res.Message.empty()) { AppHost::s_DisplayMessageBox(res); - std::quick_exit(res.ExitCode); } + exitCode = res.ExitCode; } - return makeWindow; + TerminateProcess(GetCurrentProcess(), gsl::narrow_cast(exitCode)); + __assume(false); } void WindowEmperor::WaitForWindows() @@ -600,15 +570,6 @@ LRESULT WindowEmperor::_messageHandler(UINT const message, WPARAM const wParam, // we'll undoubtedly crash. winrt::fire_and_forget WindowEmperor::_close() { - { - auto fridge{ _oldThreads.lock() }; - for (auto& window : *fridge) - { - window->ThrowAway(); - } - fridge->clear(); - } - // Important! Switch back to the main thread for the emperor. That way, the // quit will go to the emperor's message pump. co_await wil::resume_foreground(_dispatcher); diff --git a/src/cascadia/WindowsTerminal/WindowEmperor.h b/src/cascadia/WindowsTerminal/WindowEmperor.h index 47e8b356195..b55d44bab55 100644 --- a/src/cascadia/WindowsTerminal/WindowEmperor.h +++ b/src/cascadia/WindowsTerminal/WindowEmperor.h @@ -27,7 +27,7 @@ class WindowEmperor : public std::enable_shared_from_this ~WindowEmperor(); void WaitForWindows(); - bool HandleCommandlineArgs(); + void HandleCommandlineArgs(); private: void _createNewWindowThread(const winrt::Microsoft::Terminal::Remoting::WindowRequestedArgs& args); diff --git a/src/cascadia/WindowsTerminal/main.cpp b/src/cascadia/WindowsTerminal/main.cpp index 09cac10710c..dc10573df9c 100644 --- a/src/cascadia/WindowsTerminal/main.cpp +++ b/src/cascadia/WindowsTerminal/main.cpp @@ -115,8 +115,5 @@ int __stdcall wWinMain(HINSTANCE, HINSTANCE, LPWSTR, int) winrt::init_apartment(winrt::apartment_type::single_threaded); const auto emperor = std::make_shared<::WindowEmperor>(); - if (emperor->HandleCommandlineArgs()) - { - emperor->WaitForWindows(); - } + emperor->HandleCommandlineArgs(); } From 14057495adb1111e79bacbeac0daa964a474b9f2 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Thu, 25 Jan 2024 20:55:15 +0100 Subject: [PATCH 5/6] Address feedback --- src/cascadia/WindowsTerminal/AppHost.cpp | 3 +-- src/cascadia/WindowsTerminal/WindowEmperor.cpp | 6 ------ src/cascadia/WindowsTerminal/WindowEmperor.h | 1 - 3 files changed, 1 insertion(+), 9 deletions(-) diff --git a/src/cascadia/WindowsTerminal/AppHost.cpp b/src/cascadia/WindowsTerminal/AppHost.cpp index 73e228de277..7cae9959998 100644 --- a/src/cascadia/WindowsTerminal/AppHost.cpp +++ b/src/cascadia/WindowsTerminal/AppHost.cpp @@ -166,8 +166,7 @@ void AppHost::_HandleCommandlineArgs(const Remoting::WindowRequestedArgs& window if (_windowLogic.ShouldExitEarly()) { - TerminateProcess(GetCurrentProcess(), gsl::narrow_cast(result)); - __assume(false); + ExitThread(result); } } } diff --git a/src/cascadia/WindowsTerminal/WindowEmperor.cpp b/src/cascadia/WindowsTerminal/WindowEmperor.cpp index 53cdcdd4434..ec8667ae3ea 100644 --- a/src/cascadia/WindowsTerminal/WindowEmperor.cpp +++ b/src/cascadia/WindowsTerminal/WindowEmperor.cpp @@ -40,12 +40,6 @@ WindowEmperor::WindowEmperor() noexcept : _dispatcher = winrt::Windows::System::DispatcherQueue::GetForCurrentThread(); } -WindowEmperor::~WindowEmperor() -{ - _app.Close(); - _app = nullptr; -} - void _buildArgsFromCommandline(std::vector& args) { if (auto commandline{ GetCommandLineW() }) diff --git a/src/cascadia/WindowsTerminal/WindowEmperor.h b/src/cascadia/WindowsTerminal/WindowEmperor.h index b55d44bab55..8b0b51488dc 100644 --- a/src/cascadia/WindowsTerminal/WindowEmperor.h +++ b/src/cascadia/WindowsTerminal/WindowEmperor.h @@ -24,7 +24,6 @@ class WindowEmperor : public std::enable_shared_from_this { public: WindowEmperor() noexcept; - ~WindowEmperor(); void WaitForWindows(); void HandleCommandlineArgs(); From 5ad7a4263573237f1166ad7093759596a4bf0783 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Thu, 25 Jan 2024 21:01:58 +0100 Subject: [PATCH 6/6] Add a comment --- src/cascadia/WindowsTerminal/WindowEmperor.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/cascadia/WindowsTerminal/WindowEmperor.cpp b/src/cascadia/WindowsTerminal/WindowEmperor.cpp index ec8667ae3ea..e950263ebb6 100644 --- a/src/cascadia/WindowsTerminal/WindowEmperor.cpp +++ b/src/cascadia/WindowsTerminal/WindowEmperor.cpp @@ -110,6 +110,10 @@ void WindowEmperor::HandleCommandlineArgs() exitCode = res.ExitCode; } + // There's a mysterious crash in XAML on Windows 10 if you just let _app get destroyed (GH#15410). + // We also need to ensure that all UI threads exit before WindowEmperor leaves the scope on the main thread (MSFT:46744208). + // Both problems can be solved and the shutdown accelerated by using TerminateProcess. + // std::exit(), etc., cannot be used here, because those use ExitProcess for unpackaged applications. TerminateProcess(GetCurrentProcess(), gsl::narrow_cast(exitCode)); __assume(false); }