microsoft/terminal
1
0
Fork 0
mirror of https://github.com/microsoft/terminal.git synced 2025-12-10 00:48:23 -06:00
Code Issues Packages Projects Releases 138 Wiki Activity

Avoid reentrancy issues when dropping AppHost (#19296)

Browse Source 
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 8d41ace320216f4c22b0aa7fb4cd42f362096e9c)
Service-Card-Id: PVTI_lADOAF3p4s4AxadtzgebmmE
Service-Version: 1.23
This commit is contained in:
Leonard Hecker 2025-09-01 15:33:11 +02:00 committed by Dustin L. Howett
parent be596f16a3
commit 5d97c3ae36
2 changed files with 19 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

19
src/cascadia/WindowsTerminal/WindowEmperor.cpp
View File

@ -547,6 +547,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())
@ -684,6 +686,8 @@ safe_void_coroutine WindowEmperor::_dispatchCommandlineCurrentDesktop(winrt::Ter
bool WindowEmperor::_summonWindow(const SummonWindowSelectionArgs& args) const
{
_assertIsMainThread();
AppHost* window = nullptr;
if (args.WindowID)
@ -724,6 +728,8 @@ bool WindowEmperor::_summonWindow(const SummonWindowSelectionArgs& args) const
void WindowEmperor::_summonAllWindows() const
{
_assertIsMainThread();
TerminalApp::SummonWindowBehavior args;
args.ToggleVisibility(false);
@ -861,6 +867,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<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);
auto it = _windows.begin();
const auto end = _windows.end();
@ -869,7 +878,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;
}

2
src/cascadia/WindowsTerminal/WindowEmperor.h
View File

@ -88,7 +88,7 @@ private:
#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