Skip to content

Commit

Permalink
Refrigerate our threads for later reuse (#15424)
Browse files Browse the repository at this point in the history
This is my proposed solution to #15384.

Basically, the issue is that we cannot ever close a
`DesktopWindowXamlSource` ("DWXS"). If we do, then any other thread that
tries to access XAML metadata will explode, which happens frequently. A
DWXS is inextricably linked to an HWND. That means we have to not only
reuse DWXS's, but the HWNDs themselves. XAML also isn't agile, so we've
got to keep the `thread` that the DWXS was started on alive as well.

To do this, we're going to introduce the ability to "refrigerate" and
"reheat" window threads.
* A window thread is "**hot**" if it's actively got a window, and is
pumping window messages, and generally, is a normal thing.
* When a window is closed, we need to "**refrigerate**" it's
`WindowThread` and `IslandWindow`. `WindowEmperor` will take care of
tracking the threads that are refrigerated.
* When a new window is requested, the Emperor first try to
"**reheat**"/"**microwave**" a refrigerated thread. When a thread gets
reheated, we'll create a new AppHost (and `TerminalWindow`/`Page`), and
we'll use the _existing_ `IslandWindow` for that instance.

<sub>The metaphor is obviously ridiculous, but _you get it_ so who
cares.</sub>

In this way, we'll keep all the windows we've ever created around in
memory, for later reuse. This means that the leak goes from (~12MB x
number of windows closed) to (~12MB x maximum number of simultaneously
open Terminal windows). It's still not good.

We won't do this on Windows 11, because the bug that is the fundamental
premise of this issue is fixed already in the OS.




I'm not 100% confident in this yet. 
* [x] There's still a d3d leak of some sort on exit in debug builds.
(maybe #15306 related)
* havent seen this in a while. Must have been a issue in an earlier
revision.
* [x] I need to validate more on Windows 11
* [x] **BAD**: Closing the last tab on Windows 11 doesn't close the
window
* [x] **BAD**: Closing a window on Windows 11 doesn't close the window -
it just closes the one tab item and keeps on choochin'
* [x] **BAD**: Close last tab, open new one, attempt to close window -
ALL windows go \*poof\*. Cause of course. No break into post-mortem
either.
* [x] more comments
* [ ] maybe a diagram
* [x] Restoring windows is at the wrong place entirely? I once reopened
the Terminal with two persisted windows, and it created one at 0,0
* [x] Remaining code TODO!s: 0 (?)
* [ ] "warm-reloading" `useTabsInTitlebar` (change while terminal is
running after closing a window, open a new one) REALLY doesn't work.
Obviously restores the last kind of window. Yike.

is all about #15384

closes #15410 along the way. Might fork that fix off.
  • Loading branch information
zadjii-msft authored Jul 19, 2023
1 parent 6a10ea5 commit 7010626
Show file tree
Hide file tree
Showing 13 changed files with 492 additions and 124 deletions.
121 changes: 84 additions & 37 deletions src/cascadia/WindowsTerminal/AppHost.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ constexpr const auto FrameUpdateInterval = std::chrono::milliseconds(16);
AppHost::AppHost(const winrt::TerminalApp::AppLogic& logic,
winrt::Microsoft::Terminal::Remoting::WindowRequestedArgs args,
const Remoting::WindowManager& manager,
const Remoting::Peasant& peasant) noexcept :
const Remoting::Peasant& peasant,
std::unique_ptr<IslandWindow> window) noexcept :
_appLogic{ logic },
_windowLogic{ nullptr }, // don't make one, we're going to take a ref on app's
_windowManager{ manager },
Expand All @@ -48,13 +49,22 @@ AppHost::AppHost(const winrt::TerminalApp::AppLogic& logic,

// _HandleCommandlineArgs will create a _windowLogic
_useNonClientArea = _windowLogic.GetShowTabsInTitlebar();
if (_useNonClientArea)

const bool isWarmStart = window != nullptr;
if (isWarmStart)
{
_window = std::make_unique<NonClientIslandWindow>(_windowLogic.GetRequestedTheme());
_window = std::move(window);
}
else
{
_window = std::make_unique<IslandWindow>();
if (_useNonClientArea)
{
_window = std::make_unique<NonClientIslandWindow>(_windowLogic.GetRequestedTheme());
}
else
{
_window = std::make_unique<IslandWindow>();
}
}

// Update our own internal state tracking if we're in quake mode or not.
Expand All @@ -69,14 +79,10 @@ AppHost::AppHost(const winrt::TerminalApp::AppLogic& logic,
std::placeholders::_2);
_window->SetCreateCallback(pfn);

_window->MouseScrolled({ this, &AppHost::_WindowMouseWheeled });
_window->WindowActivated({ this, &AppHost::_WindowActivated });
_window->WindowMoved({ this, &AppHost::_WindowMoved });

_window->ShouldExitFullscreen({ &_windowLogic, &winrt::TerminalApp::TerminalWindow::RequestExitFullscreen });

_window->SetAlwaysOnTop(_windowLogic.GetInitialAlwaysOnTop());
_window->SetAutoHideWindow(_windowLogic.AutoHideWindow());
_windowCallbacks.MouseScrolled = _window->MouseScrolled({ this, &AppHost::_WindowMouseWheeled });
_windowCallbacks.WindowActivated = _window->WindowActivated({ this, &AppHost::_WindowActivated });
_windowCallbacks.WindowMoved = _window->WindowMoved({ this, &AppHost::_WindowMoved });
_windowCallbacks.ShouldExitFullscreen = _window->ShouldExitFullscreen({ &_windowLogic, &winrt::TerminalApp::TerminalWindow::RequestExitFullscreen });

_window->MakeWindow();
}
Expand Down Expand Up @@ -286,6 +292,14 @@ void AppHost::Initialize()
_windowLogic.SetTitleBarContent({ this, &AppHost::_UpdateTitleBarContent });
}

// These call APIs that are reentrant on the window message loop. If
// you call them in the ctor, we might deadlock. The ctor for AppHost isn't
// always called on the window thread - for reheated windows, it could be
// called on a random COM thread.

_window->SetAlwaysOnTop(_windowLogic.GetInitialAlwaysOnTop());
_window->SetAutoHideWindow(_windowLogic.AutoHideWindow());

// MORE EVENT HANDLERS HERE!
// MAKE SURE THEY ARE ALL:
// * winrt::auto_revoke
Expand All @@ -295,13 +309,14 @@ void AppHost::Initialize()
// tearing down, after we've nulled out the window, during the dtor. That
// can cause unexpected AV's everywhere.
//
// _window callbacks don't need to be treated this way, because:
// * IslandWindow isn't a WinRT type (so it doesn't have neat revokers like this)
// * This particular bug scenario applies when we've already freed the window.
// _window callbacks are a little special:
// * IslandWindow isn't a WinRT type (so it doesn't have neat revokers like
// this), so instead they go in their own special helper struct.
// * they all need to be manually revoked in _revokeWindowCallbacks.

// Register the 'X' button of the window for a warning experience of multiple
// tabs opened, this is consistent with Alt+F4 closing
_window->WindowCloseButtonClicked([this]() {
_windowCallbacks.WindowCloseButtonClicked = _window->WindowCloseButtonClicked([this]() {
_CloseRequested(nullptr, nullptr);
});
// If the user requests a close in another way handle the same as if the 'X'
Expand All @@ -310,11 +325,11 @@ void AppHost::Initialize()

// Add an event handler to plumb clicks in the titlebar area down to the
// application layer.
_window->DragRegionClicked([this]() { _windowLogic.TitlebarClicked(); });
_windowCallbacks.DragRegionClicked = _window->DragRegionClicked([this]() { _windowLogic.TitlebarClicked(); });

_window->WindowVisibilityChanged([this](bool showOrHide) { _windowLogic.WindowVisibilityChanged(showOrHide); });
_windowCallbacks.WindowVisibilityChanged = _window->WindowVisibilityChanged([this](bool showOrHide) { _windowLogic.WindowVisibilityChanged(showOrHide); });

_window->UpdateSettingsRequested({ this, &AppHost::_requestUpdateSettings });
_windowCallbacks.UpdateSettingsRequested = _window->UpdateSettingsRequested({ this, &AppHost::_requestUpdateSettings });

_revokers.Initialized = _windowLogic.Initialized(winrt::auto_revoke, { this, &AppHost::_WindowInitializedHandler });
_revokers.RequestedThemeChanged = _windowLogic.RequestedThemeChanged(winrt::auto_revoke, { this, &AppHost::_UpdateTheme });
Expand All @@ -325,14 +340,14 @@ void AppHost::Initialize()
_revokers.SystemMenuChangeRequested = _windowLogic.SystemMenuChangeRequested(winrt::auto_revoke, { this, &AppHost::_SystemMenuChangeRequested });
_revokers.ChangeMaximizeRequested = _windowLogic.ChangeMaximizeRequested(winrt::auto_revoke, { this, &AppHost::_ChangeMaximizeRequested });

_window->MaximizeChanged([this](bool newMaximize) {
_windowCallbacks.MaximizeChanged = _window->MaximizeChanged([this](bool newMaximize) {
if (_windowLogic)
{
_windowLogic.Maximized(newMaximize);
}
});

_window->AutomaticShutdownRequested([this]() {
_windowCallbacks.AutomaticShutdownRequested = _window->AutomaticShutdownRequested([this]() {
// Raised when the OS is beginning an update of the app. We will quit,
// to save our state, before the OS manually kills us.
Remoting::WindowManager::RequestQuitAll(_peasant);
Expand Down Expand Up @@ -428,6 +443,9 @@ void AppHost::Close()
_frameTimer.Tick(_frameTimerToken);
}
_showHideWindowThrottler.reset();

_revokeWindowCallbacks();

_window->Close();

if (_windowLogic)
Expand All @@ -437,6 +455,50 @@ void AppHost::Close()
}
}

void AppHost::_revokeWindowCallbacks()
{
// You'll recall, IslandWindow isn't a WinRT type so it can't have auto-revokers.
//
// Instead, we need to manually remove our callbacks we registered on the window object.
_window->MouseScrolled(_windowCallbacks.MouseScrolled);
_window->WindowActivated(_windowCallbacks.WindowActivated);
_window->WindowMoved(_windowCallbacks.WindowMoved);
_window->ShouldExitFullscreen(_windowCallbacks.ShouldExitFullscreen);
_window->WindowCloseButtonClicked(_windowCallbacks.WindowCloseButtonClicked);
_window->DragRegionClicked(_windowCallbacks.DragRegionClicked);
_window->WindowVisibilityChanged(_windowCallbacks.WindowVisibilityChanged);
_window->UpdateSettingsRequested(_windowCallbacks.UpdateSettingsRequested);
_window->MaximizeChanged(_windowCallbacks.MaximizeChanged);
_window->AutomaticShutdownRequested(_windowCallbacks.AutomaticShutdownRequested);
}

// revoke our callbacks, discard our XAML content (TerminalWindow &
// TerminalPage), and hand back our IslandWindow. This does _not_ close the XAML
// island for this thread. We should not be re-used after this, and our caller
// can destruct us like they normally would during a close. The returned
// IslandWindow will retain ownership of the DesktopWindowXamlSource, for later
// reuse.
[[nodiscard]] std::unique_ptr<IslandWindow> AppHost::Refrigerate()
{
// After calling _window->Close() we should avoid creating more WinUI related actions.
// I suspect WinUI wouldn't like that very much. As such unregister all event handlers first.
_revokers = {};
_showHideWindowThrottler.reset();

_revokeWindowCallbacks();

// DO NOT CLOSE THE WINDOW
_window->Refrigerate();

if (_windowLogic)
{
_windowLogic.DismissDialog();
_windowLogic = nullptr;
}

return std::move(_window);
}

// Method Description:
// - Called every time when the active tab's title changes. We'll also fire off
// a window message so we can update the window's title on the main thread,
Expand Down Expand Up @@ -1049,22 +1111,7 @@ static bool _isActuallyDarkTheme(const auto requestedTheme)
// Windows 10, so that we don't even get that spew
void _frameColorHelper(const HWND h, const COLORREF color)
{
static const bool isWindows11 = []() {
OSVERSIONINFOEXW osver{};
osver.dwOSVersionInfoSize = sizeof(osver);
osver.dwBuildNumber = 22000;

DWORDLONG dwlConditionMask = 0;
VER_SET_CONDITION(dwlConditionMask, VER_BUILDNUMBER, VER_GREATER_EQUAL);

if (VerifyVersionInfoW(&osver, VER_BUILDNUMBER, dwlConditionMask) != FALSE)
{
return true;
}
return false;
}();

if (isWindows11)
if (Utils::IsWindows11())
{
LOG_IF_FAILED(DwmSetWindowAttribute(h, DWMWA_BORDER_COLOR, &color, sizeof(color)));
}
Expand Down
28 changes: 27 additions & 1 deletion src/cascadia/WindowsTerminal/AppHost.h
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,16 @@ class AppHost
AppHost(const winrt::TerminalApp::AppLogic& logic,
winrt::Microsoft::Terminal::Remoting::WindowRequestedArgs args,
const winrt::Microsoft::Terminal::Remoting::WindowManager& manager,
const winrt::Microsoft::Terminal::Remoting::Peasant& peasant) noexcept;
const winrt::Microsoft::Terminal::Remoting::Peasant& peasant,
std::unique_ptr<IslandWindow> window = nullptr) noexcept;

void AppTitleChanged(const winrt::Windows::Foundation::IInspectable& sender, winrt::hstring newTitle);
void LastTabClosed(const winrt::Windows::Foundation::IInspectable& sender, const winrt::TerminalApp::LastTabClosedEventArgs& args);
void Initialize();
void Close();

[[nodiscard]] std::unique_ptr<IslandWindow> Refrigerate();

bool OnDirectKeyEvent(const uint32_t vkey, const uint8_t scanCode, const bool down);
void SetTaskbarProgress(const winrt::Windows::Foundation::IInspectable& sender, const winrt::Windows::Foundation::IInspectable& args);

Expand Down Expand Up @@ -58,6 +62,8 @@ class AppHost

void _preInit();

void _revokeWindowCallbacks();

void _HandleCommandlineArgs(const winrt::Microsoft::Terminal::Remoting::WindowRequestedArgs& args);
void _HandleSessionRestore(const bool startedForContent);

Expand Down Expand Up @@ -202,4 +208,24 @@ class AppHost
winrt::Microsoft::Terminal::Remoting::WindowManager::QuitAllRequested_revoker QuitAllRequested;
winrt::Microsoft::Terminal::Remoting::Peasant::SendContentRequested_revoker SendContentRequested;
} _revokers{};

// our IslandWindow is not a WinRT type. It can't make auto_revokers like
// the above. We also need to make sure to unregister ourself from the
// window when we refrigerate the window thread so that the window can later
// be re-used.
struct WindowRevokers
{
winrt::event_token MouseScrolled;
winrt::event_token WindowActivated;
winrt::event_token WindowMoved;
winrt::event_token ShouldExitFullscreen;
winrt::event_token WindowCloseButtonClicked;
winrt::event_token DragRegionClicked;
winrt::event_token WindowVisibilityChanged;
winrt::event_token UpdateSettingsRequested;
winrt::event_token MaximizeChanged;
winrt::event_token AutomaticShutdownRequested;
// LOAD BEARING!!
//If you add events here, make sure they're revoked in AppHost::_revokeWindowCallbacks
} _windowCallbacks{};
};
6 changes: 5 additions & 1 deletion src/cascadia/WindowsTerminal/BaseWindow.h
Original file line number Diff line number Diff line change
Expand Up @@ -210,13 +210,17 @@ class BaseWindow

bool _minimized = false;

void _setupUserData()
{
SetWindowLongPtr(_window.get(), GWLP_USERDATA, reinterpret_cast<LONG_PTR>(this));
}
// Method Description:
// - This method is called when the window receives the WM_NCCREATE message.
// Return Value:
// - The value returned from the window proc.
[[nodiscard]] virtual LRESULT OnNcCreate(WPARAM wParam, LPARAM lParam) noexcept
{
SetWindowLongPtr(_window.get(), GWLP_USERDATA, reinterpret_cast<LONG_PTR>(this));
_setupUserData();

EnableNonClientDpiScaling(_window.get());
_currentDpi = GetDpiForWindow(_window.get());
Expand Down
Loading

0 comments on commit 7010626

Please sign in to comment.