Refrigerate our threads for later reuse

src/cascadia/WindowsTerminal/AppHost.cpp

@@ -31,7 +31,8 @@ static constexpr short KeyPressed{ gsl::narrow_cast<short>(0x8000) };
 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 },
@@ -44,13 +45,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.
@@ -65,16 +75,15 @@ 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();
+    if (!isWarmStart)
+    {
+        _window->MakeWindow();
+    }
 }
 
 bool AppHost::OnDirectKeyEvent(const uint32_t vkey, const uint8_t scanCode, const bool down)
@@ -282,6 +291,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
@@ -291,13 +308,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 .
 
     // 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'
@@ -306,11 +324,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 });
@@ -321,14 +339,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);
@@ -420,6 +438,9 @@ void AppHost::Close()
     // I suspect WinUI wouldn't like that very much. As such unregister all event handlers first.
     _revokers = {};
     _showHideWindowThrottler.reset();
+
+    _revokeWindowCallbacks();
+
     _window->Close();
 
     if (_windowLogic)
@@ -429,6 +450,47 @@ void AppHost::Close()
     }
 }
 
+void AppHost::_revokeWindowCallbacks()
+{
+    _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();
+
+    // DO NOT CLOSE THE WINDOW
+    _window->Refrigerate();
+
+    _revokeWindowCallbacks();
+
+    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,

src/cascadia/WindowsTerminal/AppHost.h

@@ -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);
 
@@ -55,6 +59,8 @@ class AppHost
 
     void _preInit();
 
+    void _revokeWindowCallbacks();
+
     void _HandleCommandlineArgs(const winrt::Microsoft::Terminal::Remoting::WindowRequestedArgs& args);
     void _HandleSessionRestore(const bool startedForContent);
 
@@ -194,4 +200,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{};
 };

src/cascadia/WindowsTerminal/BaseWindow.h

@@ -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());

src/cascadia/WindowsTerminal/IslandWindow.cpp

@@ -42,46 +42,6 @@ IslandWindow::~IslandWindow()
 
 void IslandWindow::Close()
 {
-    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)
-    {
-        // BODGY
-        //  ____   ____  _____   _______     __
-        // |  _ \ / __ \|  __ \ / ____\ \   / /
-        // | |_) | |  | | |  | | |  __ \ \_/ /
-        // |  _ <| |  | | |  | | | |_ | \   /
-        // | |_) | |__| | |__| | |__| |  | |
-        // |____/ \____/|_____/ \_____|  |_|
-        //
-        // There's a bug in Windows 10 where closing a DesktopWindowXamlSource
-        // on any thread will free an internal static resource that's used by
-        // XAML for the entire process. This would result in closing window
-        // essentially causing the entire app to crash.
-        //
-        // To avoid this, leak the XAML island. We only need to leak this on
-        // Windows 10, since the bug is fixed in Windows 11.
-        //
-        // See GH #15384, MSFT:32109540
-        auto a{ _source };
-        winrt::detach_abi(_source);
-
-        // </BODGY>
-    }
-
     // GH#15454: Unset the user data for the window. This will prevent future
     // callbacks that come onto our window message loop from being sent to the
     // IslandWindow (or other derived class's) implementation.
@@ -99,6 +59,27 @@ void IslandWindow::Close()
     }
 }
 
+// Clear out any state that might be associated with this app instance, so that
+// we can later re-use this HWND for another instance.
+//
+// This doesn't actually close out our HWND or DesktopWindowXamlSource, but it
+// will remove all our content, and SW_HIDE the window, so it isn't accessible.
+void IslandWindow::Refrigerate() noexcept
+{
+    // Similar to in Close - unset our HWND's user data. We'll re-set this when
+    // we get re-heated, so that while we're refrigerated, we won't have
+    // unexpected callbacks into us while we don't have content.
+    //
+    // This pointer will get re-set in _warmInitialize
+    SetWindowLongPtr(_window.get(), GWLP_USERDATA, 0);
+
+    _pfnCreateCallback = nullptr;
+    _pfnSnapDimensionCallback = nullptr;
+
+    _rootGrid.Children().Clear();
+    ShowWindow(_window.get(), SW_HIDE);
+}
+
 HWND IslandWindow::GetInteropHandle() const
 {
     return _interopWindowHandle;
@@ -345,7 +326,30 @@ LRESULT IslandWindow::_OnMoving(const WPARAM /*wParam*/, const LPARAM lParam)
     return false;
 }
 
-void IslandWindow::Initialize()
+// return true if this was a "cold" initialize, that didn't start XAML before.
+bool IslandWindow::Initialize()
+{
+    if (!_source)
+    {
+        _coldInitialize();
+        return true;
+    }
+    else
+    {
+        // This was a "warm" initialize - we've already got an HWND, but we need
+        // to move it to the new correct place, new size, and reset any leftover
+        // runtime state.
+        _warmInitialize();
+    }
+    return false;
+}
+
+// Method Description:
+// - Start this window for the first time. This will instantiate our XAML
+//   island, set up our root grid, and initialize some other members that only
+//   need to be initialized once.
+// - This should only be called once.
+void IslandWindow::_coldInitialize()
 {
     _source = DesktopWindowXamlSource{};
 
@@ -378,9 +382,32 @@ void IslandWindow::Initialize()
     // We don't really care if this failed or not.
     TerminalTrySetTransparentBackground(true);
 }
+void IslandWindow::_warmInitialize()
+{
+    // re-add the pointer to us to our HWND's user data, so that we can start
+    // getting window proc callbacks again.
+    _setupUserData();
+
+    // Manually ask how we want to be created.
+    if (_pfnCreateCallback)
+    {
+        til::rect rc{ GetWindowRect() };
+        _pfnCreateCallback(_window.get(), rc);
+    }
+
+    // Don't call IslandWindow::OnSize - that will set the Width/Height members
+    // of the _rootGrid. However, NonClientIslandWindow doesn't use those! If you set them, here,
+    // the contents of the window will never resize.
+    UpdateWindow(_window.get());
+    ForceResize();
+}
 
 void IslandWindow::OnSize(const UINT width, const UINT height)
 {
+    // NOTE: This _isn't_ called by NonClientIslandWindow::OnSize. The
+    // NonClientIslandWindow has very different logic for positioning the
+    // DesktopWindowXamlSource inside it's HWND.
+
     // update the interop window size
     SetWindowPos(_interopWindowHandle, nullptr, 0, 0, width, height, SWP_SHOWWINDOW | SWP_NOACTIVATE);