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