Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pump the message queue on frozen windows #16588

Merged
merged 6 commits into from
Jan 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/actions/spelling/expect/expect.txt
Original file line number Diff line number Diff line change
Expand Up @@ -481,6 +481,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
Copy link
Member

Choose a reason for hiding this comment

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

Very sus that this is just gone now - why? Is this related?

Copy link
Member Author

Choose a reason for hiding this comment

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

If you expand the surrounding code in the diff viewer you'll notice that this block of code is a duplicate! I think it might have been an incorrect merge in the past.

Copy link
Member

Choose a reason for hiding this comment

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

oh what the heck

// 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()
Copy link
Member

Choose a reason for hiding this comment

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

Just added redundancy? I don't think we've got any code that was removed that was calling Close before, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, it's terrible... This change fixes a bug and you might already know what I mean when I say: IslandWindow::Close is a virtual function which the ~IslandWindow destructor calls. NonClientIslandWindow overloads Close. boom

But in my opinion this sorely belongs into the "only obvious in hindsight" category of arcane C++ footnukes (can't have enough of those in a language!), so let me explain in more detail:

  • No matter whether you define a destructor or not, C++ will define it's own "real-destructor", because that one is responsible for destroying non-trival class members (like std::strings and such). So, while NonClientIslandWindow doesn't have an explicit destructor, it gets one anyways which virtually overloads the IslandWindow destructor.
  • With inheritance, destructors are called in reverse starting from child classes up to parent classes. In other words, first ~NonClientIslandWindow is called and then ~IslandWindow.

In short, when ~IslandWindow gets called and it tries to call the virtual function Close, the child class NonClientIslandWindow is already destroyed, which means that the vtable pointer now points to IslandWindow, which means that IslandWindow::Close is called and not NonClientIslandWindow::Close. The above change fixes this.

{
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
9 changes: 0 additions & 9 deletions src/cascadia/WindowsTerminal/WindowEmperor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -584,15 +584,6 @@ LRESULT WindowEmperor::_messageHandler(UINT const message, WPARAM const wParam,
// we'll undoubtedly crash.
winrt::fire_and_forget WindowEmperor::_close()
{
{
auto fridge{ _oldThreads.lock() };
for (auto& window : *fridge)
{
window->ThrowAway();
}
fridge->clear();
}

// Important! Switch back to the main thread for the emperor. That way, the
// quit will go to the emperor's message pump.
co_await wil::resume_foreground(_dispatcher);
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)
Copy link
Member

Choose a reason for hiding this comment

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

fwiw mike referred to the act of waking back up as Microwaving; refrigerating is when it goes down to sleep

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see - you have repurposed it to be a single flag. Transition between the states. etc

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I wasn't sure what to name it... "refrigerate" has the benefit that it only comes up in this context in the project, so it should be easier to find related code.

lhecker marked this conversation as resolved.
Show resolved Hide resolved
{
_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)
lhecker marked this conversation as resolved.
Show resolved Hide resolved
{
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
Loading