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

(cherry picked from commit 5d2fa47)
Service-Card-Id: 91642478
Service-Version: 1.18
  • Loading branch information
lhecker authored and DHowett committed Jan 29, 2024
1 parent dd595d7 commit 8ac494b
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 @@ -505,6 +505,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 @@ -524,23 +524,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

1 comment on commit 8ac494b

@github-actions
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@check-spelling-bot Report

🔴 Please review

See the 📜action log for details.

Unrecognized words (3)

atvis
LASTEXITCODE
wcsnlen

Previously acknowledged words that are now absent ACCESSTOKEN BUILDURI COLLECTIONURI countof dhandler dmp etcoreapp fileurl HKLM homeglyphs IXMP lastexitcode listproperties logissue minimizeall preinstalled procs SOURCEBRANCH suiteless TEAMPROJECT testbuildplatform testdlls testmode testnameprefix testtimeout untests xxyyzz :arrow_right:
To accept ✔️ these unrecognized words as correct and remove the previously acknowledged and now absent words, run the following commands

... in a clone of the [email protected]:microsoft/terminal.git repository
on the release-1.18 branch (ℹ️ how do I use this?):

curl -s -S -L 'https://raw.githubusercontent.com/check-spelling/check-spelling/v0.0.21/apply.pl' |
perl - 'https://github.com/microsoft/terminal/actions/runs/7703497054/attempts/1'
Pattern suggestions ✂️ (1)

You could add these patterns to .github/actions/spelling/patterns/8ac494b8c113fd27d929931fbfb159c7632dee36.txt:

# Automatically suggested patterns
# hit-count: 1 file-count: 1
# Punycode
\bxn--[-0-9a-z]+

Warnings (1)

See the 📜action log for details.

ℹ️ Warnings Count
ℹ️ candidate-pattern 1

See ℹ️ Event descriptions for more information.

✏️ Contributor please read this

By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.

⚠️ The command is written for posix shells. If it doesn't work for you, you can manually add (one word per line) / remove items to expect.txt and the excludes.txt files.

If the listed items are:

  • ... misspelled, then please correct them instead of using the command.
  • ... names, please add them to .github/actions/spelling/allow/names.txt.
  • ... APIs, you can add them to a file in .github/actions/spelling/allow/.
  • ... just things you're using, please add them to an appropriate file in .github/actions/spelling/expect/.
  • ... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in .github/actions/spelling/patterns/.

See the README.md in each directory for more information.

🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉

If the flagged items are 🤯 false positives

If items relate to a ...

  • binary file (or some other file you wouldn't want to check at all).

    Please add a file path to the excludes.txt file matching the containing file.

    File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

    ^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).

  • well-formed pattern.

    If you can write a pattern that would match it,
    try adding it to the patterns.txt file.

    Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.

    Note that patterns can't match multiline strings.

Please sign in to comment.