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.
This commit is contained in:
Leonard Hecker
2025-09-01 15:33:11 +02:00
committed by GitHub
parent 7849b00cbd
commit 8d41ace320
2 changed files with 20 additions and 3 deletions

View File

@@ -549,6 +549,8 @@ void WindowEmperor::_dispatchSpecialKey(const MSG& msg) const
void WindowEmperor::_dispatchCommandline(winrt::TerminalApp::CommandlineArgs args) void WindowEmperor::_dispatchCommandline(winrt::TerminalApp::CommandlineArgs args)
{ {
_assertIsMainThread();
const auto exitCode = args.ExitCode(); const auto exitCode = args.ExitCode();
if (const auto msg = args.ExitMessage(); !msg.empty()) 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 bool WindowEmperor::_summonWindow(const SummonWindowSelectionArgs& args) const
{ {
_assertIsMainThread();
AppHost* window = nullptr; AppHost* window = nullptr;
if (args.WindowID) if (args.WindowID)
@@ -726,6 +730,8 @@ bool WindowEmperor::_summonWindow(const SummonWindowSelectionArgs& args) const
void WindowEmperor::_summonAllWindows() const void WindowEmperor::_summonAllWindows() const
{ {
_assertIsMainThread();
TerminalApp::SummonWindowBehavior args; TerminalApp::SummonWindowBehavior args;
args.ToggleVisibility(false); 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. // Did the window counter get out of sync? It shouldn't.
assert(_windowCount == gsl::narrow_cast<int32_t>(_windows.size())); assert(_windowCount == gsl::narrow_cast<int32_t>(_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<AppHost*>(lParam); const auto host = reinterpret_cast<AppHost*>(lParam);
auto it = _windows.begin(); auto it = _windows.begin();
const auto end = _windows.end(); const auto end = _windows.end();
@@ -871,7 +880,15 @@ LRESULT WindowEmperor::_messageHandler(HWND window, UINT const message, WPARAM c
{ {
if (host == it->get()) 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); _windows.erase(it);
break; break;
} }

View File

@@ -84,14 +84,14 @@ private:
int32_t _windowCount = 0; int32_t _windowCount = 0;
int32_t _messageBoxCount = 0; int32_t _messageBoxCount = 0;
#ifdef NDEBUG #if 0 // #ifdef NDEBUG
static constexpr void _assertIsMainThread() noexcept static constexpr void _assertIsMainThread() noexcept
{ {
} }
#else #else
void _assertIsMainThread() const noexcept 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(); DWORD _mainThreadId = GetCurrentThreadId();
#endif #endif