Skip to content

Commit 8d41ace

Browse files
authored
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.
1 parent 7849b00 commit 8d41ace

File tree

2 files changed

+20
-3
lines changed

2 files changed

+20
-3
lines changed

src/cascadia/WindowsTerminal/WindowEmperor.cpp

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -549,6 +549,8 @@ void WindowEmperor::_dispatchSpecialKey(const MSG& msg) const
549549

550550
void WindowEmperor::_dispatchCommandline(winrt::TerminalApp::CommandlineArgs args)
551551
{
552+
_assertIsMainThread();
553+
552554
const auto exitCode = args.ExitCode();
553555

554556
if (const auto msg = args.ExitMessage(); !msg.empty())
@@ -686,6 +688,8 @@ safe_void_coroutine WindowEmperor::_dispatchCommandlineCurrentDesktop(winrt::Ter
686688

687689
bool WindowEmperor::_summonWindow(const SummonWindowSelectionArgs& args) const
688690
{
691+
_assertIsMainThread();
692+
689693
AppHost* window = nullptr;
690694

691695
if (args.WindowID)
@@ -726,6 +730,8 @@ bool WindowEmperor::_summonWindow(const SummonWindowSelectionArgs& args) const
726730

727731
void WindowEmperor::_summonAllWindows() const
728732
{
733+
_assertIsMainThread();
734+
729735
TerminalApp::SummonWindowBehavior args;
730736
args.ToggleVisibility(false);
731737

@@ -863,6 +869,9 @@ LRESULT WindowEmperor::_messageHandler(HWND window, UINT const message, WPARAM c
863869
// Did the window counter get out of sync? It shouldn't.
864870
assert(_windowCount == gsl::narrow_cast<int32_t>(_windows.size()));
865871

872+
// !!! NOTE !!!
873+
// At least theoretically the lParam pointer may be invalid.
874+
// We should only access it if we find it in our _windows array.
866875
const auto host = reinterpret_cast<AppHost*>(lParam);
867876
auto it = _windows.begin();
868877
const auto end = _windows.end();
@@ -871,7 +880,15 @@ LRESULT WindowEmperor::_messageHandler(HWND window, UINT const message, WPARAM c
871880
{
872881
if (host == it->get())
873882
{
874-
host->Close();
883+
// NOTE: The AppHost destructor is highly non-trivial.
884+
//
885+
// It _may_ call into XAML, which _may_ pump the message loop, which would then recursively
886+
// re-enter this function, which _may_ then handle another WM_CLOSE_TERMINAL_WINDOW,
887+
// which would change the _windows array, and invalidate our iterator and crash.
888+
//
889+
// We can prevent this by deferring destruction until after the erase() call.
890+
const auto strong = *it;
891+
strong->Close();
875892
_windows.erase(it);
876893
break;
877894
}

src/cascadia/WindowsTerminal/WindowEmperor.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -84,14 +84,14 @@ class WindowEmperor
8484
int32_t _windowCount = 0;
8585
int32_t _messageBoxCount = 0;
8686

87-
#ifdef NDEBUG
87+
#if 0 // #ifdef NDEBUG
8888
static constexpr void _assertIsMainThread() noexcept
8989
{
9090
}
9191
#else
9292
void _assertIsMainThread() const noexcept
9393
{
94-
assert(_mainThreadId == GetCurrentThreadId());
94+
WI_ASSERT_MSG(_mainThreadId == GetCurrentThreadId(), "This part of WindowEmperor must be accessed from the UI thread");
9595
}
9696
DWORD _mainThreadId = GetCurrentThreadId();
9797
#endif

0 commit comments

Comments
 (0)