diff --git a/src/cascadia/WindowsTerminal/WindowEmperor.cpp b/src/cascadia/WindowsTerminal/WindowEmperor.cpp index c19d2f22d5..5b6c9fd833 100644 --- a/src/cascadia/WindowsTerminal/WindowEmperor.cpp +++ b/src/cascadia/WindowsTerminal/WindowEmperor.cpp @@ -253,6 +253,29 @@ void WindowEmperor::CreateNewWindow(winrt::TerminalApp::WindowRequestedArgs args _windowCount += 1; _windows.emplace_back(std::move(host)); + + if (_windowCount == 1) + { + // The first CoreWindow is created implicitly by XAML and parented to the + // first XAML island. We parent it to our initial window for 2 reasons: + // * On Windows 10 the CoreWindow will show up as a visible window on the taskbar + // due to a WinUI bug, and this will hide it, because our initial window is hidden. + // * When we DestroyWindow() the island it will destroy the CoreWindow, + // and it's not possible to recreate it. That's also a WinUI bug. + // + // Note that this must be done after the first window (= first island) is created. + if (const auto coreWindow = winrt::Windows::UI::Core::CoreWindow::GetForCurrentThread()) + { + if (const auto interop = coreWindow.try_as()) + { + HWND coreHandle = nullptr; + if (SUCCEEDED(interop->get_WindowHandle(&coreHandle)) && coreHandle) + { + SetParent(coreHandle, _window.get()); + } + } + } + } } AppHost* WindowEmperor::_mostRecentWindow() const noexcept @@ -395,24 +418,6 @@ void WindowEmperor::HandleCommandlineArgs(int nCmdShow) LOG_IF_WIN32_BOOL_FALSE(SetCurrentDirectoryW(system32.c_str())); } - // The first CoreWindow is created implicitly by XAML and parented to the - // first XAML island. We parent it to our initial window for 2 reasons: - // * On Windows 10 the CoreWindow will show up as a visible window on the taskbar - // due to a WinUI bug, and this will hide it, because our initial window is hidden. - // * When we DestroyWindow() the island it will destroy the CoreWindow, - // and it's not possible to recreate it. That's also a WinUI bug. - if (const auto coreWindow = winrt::Windows::UI::Core::CoreWindow::GetForCurrentThread()) - { - if (const auto interop = coreWindow.try_as()) - { - HWND coreHandle = nullptr; - if (SUCCEEDED(interop->get_WindowHandle(&coreHandle)) && coreHandle) - { - SetParent(coreHandle, _window.get()); - } - } - } - { TerminalConnection::ConptyConnection::NewConnection([this](TerminalConnection::ConptyConnection conn) { TerminalApp::CommandlineArgs args; @@ -544,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()) @@ -681,6 +688,8 @@ safe_void_coroutine WindowEmperor::_dispatchCommandlineCurrentDesktop(winrt::Ter bool WindowEmperor::_summonWindow(const SummonWindowSelectionArgs& args) const { + _assertIsMainThread(); + AppHost* window = nullptr; if (args.WindowID) @@ -721,6 +730,8 @@ bool WindowEmperor::_summonWindow(const SummonWindowSelectionArgs& args) const void WindowEmperor::_summonAllWindows() const { + _assertIsMainThread(); + TerminalApp::SummonWindowBehavior args; args.ToggleVisibility(false); @@ -858,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(); @@ -866,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