Skip to content

Commit

Permalink
Pump the message queue on frozen windows (#16588)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
lhecker committed Jan 29, 2024
1 parent 63bfdb2 commit 5d2fa47
Show file tree
Hide file tree
Showing 7 changed files with 50 additions and 55 deletions.
1 change: 1 addition & 0 deletions .github/actions/spelling/expect/expect.txt
Original file line number Diff line number Diff line change
Expand Up @@ -482,6 +482,7 @@ directio
DIRECTX
DISABLEDELAYEDEXPANSION
DISABLENOSCROLL
DISPATCHNOTIFY
DISPLAYATTRIBUTE
DISPLAYATTRIBUTEPROPERTY
DISPLAYCHANGE
Expand Down
20 changes: 8 additions & 12 deletions src/cascadia/WindowsTerminal/AppHost.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
2 changes: 2 additions & 0 deletions src/cascadia/WindowsTerminal/AppHost.h
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
class AppHost : public std::enable_shared_from_this<AppHost>
{
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,
Expand Down
5 changes: 5 additions & 0 deletions src/cascadia/WindowsTerminal/NonClientIslandWindow.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/WindowsTerminal/NonClientIslandWindow.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
73 changes: 33 additions & 40 deletions src/cascadia/WindowsTerminal/WindowThread.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand All @@ -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);
}
}

Expand All @@ -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<AppHost>(_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()
Expand All @@ -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
Expand Down
3 changes: 0 additions & 3 deletions src/cascadia/WindowsTerminal/WindowThread.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ class WindowThread : public std::enable_shared_from_this<WindowThread>
void Microwave(
winrt::Microsoft::Terminal::Remoting::WindowRequestedArgs args,
winrt::Microsoft::Terminal::Remoting::Peasant peasant);
void ThrowAway();

uint64_t PeasantID();

Expand All @@ -43,8 +42,6 @@ class WindowThread : public std::enable_shared_from_this<WindowThread>
winrt::event_token _UpdateSettingsRequestedToken;

std::unique_ptr<::IslandWindow> _warmWindow{ nullptr };
std::mutex _microwave;
std::condition_variable _microwaveBuzzer;

int _messagePump();
void _pumpRemainingXamlMessages();
Expand Down

0 comments on commit 5d2fa47

Please sign in to comment.