Skip to content

Commit 5d97c3a

Browse files
lheckerDHowett
authored andcommitted
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. **BACKPORT NOTES** I returned the `_assertIsMainThread` check to debug-only to remove the assertion risk from production builds. (cherry picked from commit 8d41ace) Service-Card-Id: PVTI_lADOAF3p4s4AxadtzgebmmE Service-Version: 1.23
1 parent be596f1 commit 5d97c3a

File tree

2 files changed

+19
-2
lines changed

2 files changed

+19
-2
lines changed

src/cascadia/WindowsTerminal/WindowEmperor.cpp

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

548548
void WindowEmperor::_dispatchCommandline(winrt::TerminalApp::CommandlineArgs args)
549549
{
550+
_assertIsMainThread();
551+
550552
const auto exitCode = args.ExitCode();
551553

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

685687
bool WindowEmperor::_summonWindow(const SummonWindowSelectionArgs& args) const
686688
{
689+
_assertIsMainThread();
690+
687691
AppHost* window = nullptr;
688692

689693
if (args.WindowID)
@@ -724,6 +728,8 @@ bool WindowEmperor::_summonWindow(const SummonWindowSelectionArgs& args) const
724728

725729
void WindowEmperor::_summonAllWindows() const
726730
{
731+
_assertIsMainThread();
732+
727733
TerminalApp::SummonWindowBehavior args;
728734
args.ToggleVisibility(false);
729735

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

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

src/cascadia/WindowsTerminal/WindowEmperor.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ class WindowEmperor
8888
#else
8989
void _assertIsMainThread() const noexcept
9090
{
91-
assert(_mainThreadId == GetCurrentThreadId());
91+
WI_ASSERT_MSG(_mainThreadId == GetCurrentThreadId(), "This part of WindowEmperor must be accessed from the UI thread");
9292
}
9393
DWORD _mainThreadId = GetCurrentThreadId();
9494
#endif

0 commit comments

Comments
 (0)