From 5d2fa4782fcb7658996ea4f2078577bc27a6a6d5 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Mon, 29 Jan 2024 23:01:18 +0100 Subject: [PATCH] Pump the message queue on frozen windows (#16588) This changeset ensures that the message queue of frozen windows is always being serviced. This should ensure that it won't fill up and lead to deadlocks, freezes, or similar. I've tried _a lot_ of different approaches before settling on this one. Introducing a custom `WM_APP` message has the benefit of being the least intrusive to the existing code base. The approach that I would have favored the most would be to never destroy the `AppHost` instance in the first place, as I imagined that this would be more robust in general and resolve other (rare) bugs. However, I found that this requires rewriting some substantial parts of the code base around `AppHost` and it could be something that may be of interest in the future. Closes #16332 Depends on #16587 and #16575 --- .github/actions/spelling/expect/expect.txt | 1 + src/cascadia/WindowsTerminal/AppHost.cpp | 20 ++--- src/cascadia/WindowsTerminal/AppHost.h | 2 + .../WindowsTerminal/NonClientIslandWindow.cpp | 5 ++ .../WindowsTerminal/NonClientIslandWindow.h | 1 + src/cascadia/WindowsTerminal/WindowThread.cpp | 73 +++++++++---------- src/cascadia/WindowsTerminal/WindowThread.h | 3 - 7 files changed, 50 insertions(+), 55 deletions(-) diff --git a/.github/actions/spelling/expect/expect.txt b/.github/actions/spelling/expect/expect.txt index 95ee8bc4cd3..10124d39b00 100644 --- a/.github/actions/spelling/expect/expect.txt +++ b/.github/actions/spelling/expect/expect.txt @@ -482,6 +482,7 @@ directio DIRECTX DISABLEDELAYEDEXPANSION DISABLENOSCROLL +DISPATCHNOTIFY DISPLAYATTRIBUTE DISPLAYATTRIBUTEPROPERTY DISPLAYCHANGE diff --git a/src/cascadia/WindowsTerminal/AppHost.cpp b/src/cascadia/WindowsTerminal/AppHost.cpp index 3168011a491..6a0e0ed698d 100644 --- a/src/cascadia/WindowsTerminal/AppHost.cpp +++ b/src/cascadia/WindowsTerminal/AppHost.cpp @@ -535,23 +535,19 @@ void AppHost::LastTabClosed(const winrt::Windows::Foundation::IInspectable& /*se { _windowLogic.ClearPersistedWindowState(); } - - // If the user closes the last tab, in the last window, _by closing the tab_ - // (not by closing the whole window), we need to manually persist an empty - // window state here. That will cause the terminal to re-open with the usual - // settings (not the persisted state) - if (args.ClearPersistedState() && - _windowManager.GetNumberOfPeasants() == 1) - { - _windowLogic.ClearPersistedWindowState(); - } - // Remove ourself from the list of peasants so that we aren't included in // any future requests. This will also mean we block until any existing // event handler finishes. _windowManager.SignalClose(_peasant); - PostQuitMessage(0); + if (Utils::IsWindows11()) + { + PostQuitMessage(0); + } + else + { + PostMessageW(_window->GetInteropHandle(), WM_REFRIGERATE, 0, 0); + } } LaunchPosition AppHost::_GetWindowLaunchPosition() diff --git a/src/cascadia/WindowsTerminal/AppHost.h b/src/cascadia/WindowsTerminal/AppHost.h index 992a4402874..60b96103124 100644 --- a/src/cascadia/WindowsTerminal/AppHost.h +++ b/src/cascadia/WindowsTerminal/AppHost.h @@ -11,6 +11,8 @@ class AppHost : public std::enable_shared_from_this { public: + static constexpr DWORD WM_REFRIGERATE = WM_APP + 0; + AppHost(const winrt::TerminalApp::AppLogic& logic, winrt::Microsoft::Terminal::Remoting::WindowRequestedArgs args, const winrt::Microsoft::Terminal::Remoting::WindowManager& manager, diff --git a/src/cascadia/WindowsTerminal/NonClientIslandWindow.cpp b/src/cascadia/WindowsTerminal/NonClientIslandWindow.cpp index ee538bfa71a..bab65132266 100644 --- a/src/cascadia/WindowsTerminal/NonClientIslandWindow.cpp +++ b/src/cascadia/WindowsTerminal/NonClientIslandWindow.cpp @@ -26,6 +26,11 @@ NonClientIslandWindow::NonClientIslandWindow(const ElementTheme& requestedTheme) { } +NonClientIslandWindow::~NonClientIslandWindow() +{ + Close(); +} + void NonClientIslandWindow::Close() { // Avoid further callbacks into XAML/WinUI-land after we've Close()d the DesktopWindowXamlSource diff --git a/src/cascadia/WindowsTerminal/NonClientIslandWindow.h b/src/cascadia/WindowsTerminal/NonClientIslandWindow.h index d173ad68c2d..b2cbb5a8a29 100644 --- a/src/cascadia/WindowsTerminal/NonClientIslandWindow.h +++ b/src/cascadia/WindowsTerminal/NonClientIslandWindow.h @@ -30,6 +30,7 @@ class NonClientIslandWindow : public IslandWindow static constexpr const int topBorderVisibleHeight = 1; NonClientIslandWindow(const winrt::Windows::UI::Xaml::ElementTheme& requestedTheme) noexcept; + ~NonClientIslandWindow() override; void Refrigerate() noexcept override; diff --git a/src/cascadia/WindowsTerminal/WindowThread.cpp b/src/cascadia/WindowsTerminal/WindowThread.cpp index 2c70741ad1c..bafe6ef97e5 100644 --- a/src/cascadia/WindowsTerminal/WindowThread.cpp +++ b/src/cascadia/WindowsTerminal/WindowThread.cpp @@ -4,6 +4,8 @@ #include "pch.h" #include "WindowThread.h" +using namespace winrt::Microsoft::Terminal::Remoting; + WindowThread::WindowThread(winrt::TerminalApp::AppLogic logic, winrt::Microsoft::Terminal::Remoting::WindowRequestedArgs args, winrt::Microsoft::Terminal::Remoting::WindowManager manager, @@ -81,17 +83,6 @@ void WindowThread::RundownForExit() _pumpRemainingXamlMessages(); } -void WindowThread::ThrowAway() -{ - // raise the signal to unblock KeepWarm. We won't have a host, so we'll drop - // out of the message loop to eventually RundownForExit. - // - // This should only be called when the app is fully quitting. After this is - // called on any thread, on win10, we won't be able to call into XAML - // anymore. - _microwaveBuzzer.notify_one(); -} - // Method Description: // - Check if we should keep this window alive, to try it's message loop again. // If we were refrigerated for later, then this will block the thread on the @@ -108,29 +99,24 @@ bool WindowThread::KeepWarm() return true; } - // If we're refrigerated, then wait on the microwave signal, which will be - // raised when we get re-heated by another thread to reactivate us. - - if (_warmWindow != nullptr) + // Even when the _host has been destroyed the HWND will continue receiving messages, in particular WM_DISPATCHNOTIFY at least once a second. This is important to Windows as it keeps your room warm. + MSG msg; + for (;;) { - std::unique_lock lock(_microwave); - _microwaveBuzzer.wait(lock); - - // If ThrowAway() was called, then the buzzer will be signalled without - // setting a new _host. In that case, the app is quitting, for real. We - // just want to exit with false. - const bool reheated = _host != nullptr; - if (reheated) + if (!GetMessageW(&msg, nullptr, 0, 0)) + { + return false; + } + // We're using a single window message (WM_REFRIGERATE) to indicate both + // state transitions. In this case, the window is actually being woken up. + if (msg.message == AppHost::WM_REFRIGERATE) { _UpdateSettingsRequestedToken = _host->UpdateSettingsRequested([this]() { _UpdateSettingsRequestedHandlers(); }); // Re-initialize the host here, on the window thread _host->Initialize(); + return true; } - return reheated; - } - else - { - return false; + DispatchMessageW(&msg); } } @@ -154,24 +140,22 @@ void WindowThread::Refrigerate() // Method Description: // - "Reheat" this thread for reuse. We'll build a new AppHost, and pass in the -// existing window to it. We'll then trigger the _microwaveBuzzer, so KeepWarm -// (which is on the UI thread) will get unblocked, and we can initialize this -// window. -void WindowThread::Microwave( - winrt::Microsoft::Terminal::Remoting::WindowRequestedArgs args, - winrt::Microsoft::Terminal::Remoting::Peasant peasant) +// existing window to it. We'll then wake up the thread stuck in KeepWarm(). +void WindowThread::Microwave(WindowRequestedArgs args, Peasant peasant) { + const auto hwnd = _warmWindow->GetInteropHandle(); + _peasant = std::move(peasant); _args = std::move(args); - _host = std::make_shared<::AppHost>(_appLogic, - _args, - _manager, - _peasant, - std::move(_warmWindow)); + _host = std::make_shared(_appLogic, + _args, + _manager, + _peasant, + std::move(_warmWindow)); // raise the signal to unblock KeepWarm and start the window message loop again. - _microwaveBuzzer.notify_one(); + PostMessageW(hwnd, AppHost::WM_REFRIGERATE, 0, 0); } winrt::TerminalApp::TerminalWindow WindowThread::Logic() @@ -198,6 +182,15 @@ int WindowThread::_messagePump() while (GetMessageW(&message, nullptr, 0, 0)) { + // We're using a single window message (WM_REFRIGERATE) to indicate both + // state transitions. In this case, the window is actually being refrigerated. + // This will break us out of our main message loop we'll eventually start + // the loop in WindowThread::KeepWarm to await a call to Microwave(). + if (message.message == AppHost::WM_REFRIGERATE) + { + break; + } + // GH#638 (Pressing F7 brings up both the history AND a caret browsing message) // The Xaml input stack doesn't allow an application to suppress the "caret browsing" // dialog experience triggered when you press F7. Official recommendation from the Xaml diff --git a/src/cascadia/WindowsTerminal/WindowThread.h b/src/cascadia/WindowsTerminal/WindowThread.h index a1af2db6500..c536e4297aa 100644 --- a/src/cascadia/WindowsTerminal/WindowThread.h +++ b/src/cascadia/WindowsTerminal/WindowThread.h @@ -22,7 +22,6 @@ class WindowThread : public std::enable_shared_from_this void Microwave( winrt::Microsoft::Terminal::Remoting::WindowRequestedArgs args, winrt::Microsoft::Terminal::Remoting::Peasant peasant); - void ThrowAway(); uint64_t PeasantID(); @@ -43,8 +42,6 @@ class WindowThread : public std::enable_shared_from_this winrt::event_token _UpdateSettingsRequestedToken; std::unique_ptr<::IslandWindow> _warmWindow{ nullptr }; - std::mutex _microwave; - std::condition_variable _microwaveBuzzer; int _messagePump(); void _pumpRemainingXamlMessages();