From 7849b00cbddfb4d7f532472bc41fd8d8579bd72e Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Mon, 1 Sep 2025 15:32:58 +0200 Subject: [PATCH 1/2] Fix CoreWindow being destroyed after handoff (#19298) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As per: https://github.com/microsoft/terminal/discussions/19280#discussioncomment-14237148 ## Validation Steps Performed * Launch wtd via handoff (spawn cmd, etc.) * Shift+Click the tab bar + button to create a new window * Close the initial window * UI doesn't lock up ✅ --- .../WindowsTerminal/WindowEmperor.cpp | 41 +++++++++++-------- 1 file changed, 23 insertions(+), 18 deletions(-) diff --git a/src/cascadia/WindowsTerminal/WindowEmperor.cpp b/src/cascadia/WindowsTerminal/WindowEmperor.cpp index c19d2f22d5..bb99a12684 100644 --- a/src/cascadia/WindowsTerminal/WindowEmperor.cpp +++ b/src/cascadia/WindowsTerminal/WindowEmperor.cpp @@ -253,6 +253,29 @@ void WindowEmperor::CreateNewWindow(winrt::TerminalApp::WindowRequestedArgs args _windowCount += 1; _windows.emplace_back(std::move(host)); + + if (_windowCount == 1) + { + // The first CoreWindow is created implicitly by XAML and parented to the + // first XAML island. We parent it to our initial window for 2 reasons: + // * On Windows 10 the CoreWindow will show up as a visible window on the taskbar + // due to a WinUI bug, and this will hide it, because our initial window is hidden. + // * When we DestroyWindow() the island it will destroy the CoreWindow, + // and it's not possible to recreate it. That's also a WinUI bug. + // + // Note that this must be done after the first window (= first island) is created. + if (const auto coreWindow = winrt::Windows::UI::Core::CoreWindow::GetForCurrentThread()) + { + if (const auto interop = coreWindow.try_as()) + { + HWND coreHandle = nullptr; + if (SUCCEEDED(interop->get_WindowHandle(&coreHandle)) && coreHandle) + { + SetParent(coreHandle, _window.get()); + } + } + } + } } AppHost* WindowEmperor::_mostRecentWindow() const noexcept @@ -395,24 +418,6 @@ void WindowEmperor::HandleCommandlineArgs(int nCmdShow) LOG_IF_WIN32_BOOL_FALSE(SetCurrentDirectoryW(system32.c_str())); } - // The first CoreWindow is created implicitly by XAML and parented to the - // first XAML island. We parent it to our initial window for 2 reasons: - // * On Windows 10 the CoreWindow will show up as a visible window on the taskbar - // due to a WinUI bug, and this will hide it, because our initial window is hidden. - // * When we DestroyWindow() the island it will destroy the CoreWindow, - // and it's not possible to recreate it. That's also a WinUI bug. - if (const auto coreWindow = winrt::Windows::UI::Core::CoreWindow::GetForCurrentThread()) - { - if (const auto interop = coreWindow.try_as()) - { - HWND coreHandle = nullptr; - if (SUCCEEDED(interop->get_WindowHandle(&coreHandle)) && coreHandle) - { - SetParent(coreHandle, _window.get()); - } - } - } - { TerminalConnection::ConptyConnection::NewConnection([this](TerminalConnection::ConptyConnection conn) { TerminalApp::CommandlineArgs args; From 8d41ace320216f4c22b0aa7fb4cd42f362096e9c Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Mon, 1 Sep 2025 15:33:11 +0200 Subject: [PATCH 2/2] Avoid reentrancy issues when dropping AppHost (#19296) tl;dr: ~Apphost() may pump the message loop. That's no bueno. See comments in the diff. Additionally, this PR enables `_assertIsMainThread` in release to trace down mysterious crashes in those builds. --- .../WindowsTerminal/WindowEmperor.cpp | 19 ++++++++++++++++++- src/cascadia/WindowsTerminal/WindowEmperor.h | 4 ++-- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/src/cascadia/WindowsTerminal/WindowEmperor.cpp b/src/cascadia/WindowsTerminal/WindowEmperor.cpp index bb99a12684..5b6c9fd833 100644 --- a/src/cascadia/WindowsTerminal/WindowEmperor.cpp +++ b/src/cascadia/WindowsTerminal/WindowEmperor.cpp @@ -549,6 +549,8 @@ void WindowEmperor::_dispatchSpecialKey(const MSG& msg) const void WindowEmperor::_dispatchCommandline(winrt::TerminalApp::CommandlineArgs args) { + _assertIsMainThread(); + const auto exitCode = args.ExitCode(); if (const auto msg = args.ExitMessage(); !msg.empty()) @@ -686,6 +688,8 @@ safe_void_coroutine WindowEmperor::_dispatchCommandlineCurrentDesktop(winrt::Ter bool WindowEmperor::_summonWindow(const SummonWindowSelectionArgs& args) const { + _assertIsMainThread(); + AppHost* window = nullptr; if (args.WindowID) @@ -726,6 +730,8 @@ bool WindowEmperor::_summonWindow(const SummonWindowSelectionArgs& args) const void WindowEmperor::_summonAllWindows() const { + _assertIsMainThread(); + TerminalApp::SummonWindowBehavior args; args.ToggleVisibility(false); @@ -863,6 +869,9 @@ LRESULT WindowEmperor::_messageHandler(HWND window, UINT const message, WPARAM c // Did the window counter get out of sync? It shouldn't. assert(_windowCount == gsl::narrow_cast(_windows.size())); + // !!! NOTE !!! + // At least theoretically the lParam pointer may be invalid. + // We should only access it if we find it in our _windows array. const auto host = reinterpret_cast(lParam); auto it = _windows.begin(); const auto end = _windows.end(); @@ -871,7 +880,15 @@ LRESULT WindowEmperor::_messageHandler(HWND window, UINT const message, WPARAM c { if (host == it->get()) { - host->Close(); + // NOTE: The AppHost destructor is highly non-trivial. + // + // It _may_ call into XAML, which _may_ pump the message loop, which would then recursively + // re-enter this function, which _may_ then handle another WM_CLOSE_TERMINAL_WINDOW, + // which would change the _windows array, and invalidate our iterator and crash. + // + // We can prevent this by deferring destruction until after the erase() call. + const auto strong = *it; + strong->Close(); _windows.erase(it); break; } diff --git a/src/cascadia/WindowsTerminal/WindowEmperor.h b/src/cascadia/WindowsTerminal/WindowEmperor.h index b0213e7635..c64b8c5fef 100644 --- a/src/cascadia/WindowsTerminal/WindowEmperor.h +++ b/src/cascadia/WindowsTerminal/WindowEmperor.h @@ -84,14 +84,14 @@ private: int32_t _windowCount = 0; int32_t _messageBoxCount = 0; -#ifdef NDEBUG +#if 0 // #ifdef NDEBUG static constexpr void _assertIsMainThread() noexcept { } #else void _assertIsMainThread() const noexcept { - assert(_mainThreadId == GetCurrentThreadId()); + WI_ASSERT_MSG(_mainThreadId == GetCurrentThreadId(), "This part of WindowEmperor must be accessed from the UI thread"); } DWORD _mainThreadId = GetCurrentThreadId(); #endif