From 77215d9d77b99b48d1ee8302736178f2ec9f3a77 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 18 May 2022 17:25:06 -0500 Subject: [PATCH] Fix `ShowWindow(GetConsoleWindow())` (#13118) A bad merge, that actually revealed a horrible bug. There was a secret conflict between the code in #12526 and #12515. 69b77ca was a bad merge that hid just how bad the issue was. Fixing the one line `nullptr`->`this` in `InteractivityFactory` resulted in a window that would flash uncontrollably, as it minimized and restored itself in a loop. Great. This can seemingly be fixed by making sure that the conpty window is initially created with the owner already set, rather than relying on a `SetParent` call in post. This does pose some complications for the #1256 future we're approaching. However, this is a blocking bug _now_, and we can figure out the tearout/`SetParent` thing in post. * fixes #13066. * Tested with the script in that issue. * Window doesn't flash uncontrollably. * `gci | ogv` still works right * I work here. * Opening a new tab doesn't spontaneously cause the window to minimize * Restoring from minimized doesn't yeet focus to an invisible window * Opening a new tab doesn't yeet focus to an invisible window * There _is_ a viable way to call `GetAncestor` s.t. it returns the Terminal's hwnd in Terminal, and the console's in Conhost The `SW_SHOWNOACTIVATE` change is also quite load bearing. With just `SW_NORMAL`, the pseudo window (which is invisible!) gets activated whenever the terminal window is restored from minimized. That's BAD. There's actually more to this as well. Calling `SetParent` on a window that is `WS_VISIBLE` will cause the OS to hide the window, make it a _child_ window, then call `SW_SHOW` on the window to re-show it. `SW_SHOW`, however, will cause the OS to also set that window as the _foreground_ window, which would result in the pty's hwnd stealing the foreground away from the owning terminal window. That's bad. `SetWindowLongPtr` seems to do the job of changing who the window owner is, without all the other side effects of reparenting the window. Without `SetParent`, however, the pty HWND is no longer a descendant of the Terminal HWND, so that means `GA_ROOT` can no longer be used to find the owner's hwnd. For even more insanity, without `WS_POPUP`, none of the values of `GetAncestor` will actually get the terminal HWND. So, now we also need `WS_POPUP` on the pty hwnd. To get at the Terminal hwnd, you'll need ```c++ GetAncestor(GetConsoleWindow(), GA_ROOTOWNER) ``` --- src/cascadia/TerminalApp/TerminalPage.cpp | 5 ++- .../TerminalConnection/ConptyConnection.cpp | 8 +++-- .../TerminalConnection/ConptyConnection.h | 2 +- src/cascadia/TerminalControl/ControlCore.cpp | 16 ++++++--- src/host/PtySignalInputThread.cpp | 36 +++++++++++++++---- src/host/PtySignalInputThread.hpp | 1 + src/host/VtIo.cpp | 26 +++++++++++--- src/host/VtIo.hpp | 2 ++ src/host/outputStream.cpp | 2 +- .../base/InteractivityFactory.cpp | 12 +++++-- src/interactivity/win32/windowio.cpp | 15 ++++++-- src/terminal/adapter/InteractDispatch.cpp | 18 +++++++--- 12 files changed, 110 insertions(+), 33 deletions(-) diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index 05ed799d3cf..30fcab0cc5b 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -2431,7 +2431,10 @@ namespace winrt::TerminalApp::implementation TermControl term{ settings.DefaultSettings(), settings.UnfocusedSettings(), connection }; // GH#12515: ConPTY assumes it's hidden at the start. If we're not, let it know now. - term.WindowVisibilityChanged(_visible); + if (_visible) + { + term.WindowVisibilityChanged(_visible); + } if (_hostingHwnd.has_value()) { diff --git a/src/cascadia/TerminalConnection/ConptyConnection.cpp b/src/cascadia/TerminalConnection/ConptyConnection.cpp index caf2d8e9016..3e816e2c39c 100644 --- a/src/cascadia/TerminalConnection/ConptyConnection.cpp +++ b/src/cascadia/TerminalConnection/ConptyConnection.cpp @@ -311,13 +311,17 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation THROW_IF_FAILED(_CreatePseudoConsoleAndPipes(dimensions, flags, &_inPipe, &_outPipe, &_hPC)); - // GH#12515: The conpty assumes it's hidden at the start. If we're visible, let it know now. - THROW_IF_FAILED(ConptyShowHidePseudoConsole(_hPC.get(), _initialVisibility)); if (_initialParentHwnd != 0) { THROW_IF_FAILED(ConptyReparentPseudoConsole(_hPC.get(), reinterpret_cast(_initialParentHwnd))); } + // GH#12515: The conpty assumes it's hidden at the start. If we're visible, let it know now. + if (_initialVisibility) + { + THROW_IF_FAILED(ConptyShowHidePseudoConsole(_hPC.get(), _initialVisibility)); + } + THROW_IF_FAILED(_LaunchAttachedClient()); } // But if it was an inbound handoff... attempt to synchronize the size of it with what our connection diff --git a/src/cascadia/TerminalConnection/ConptyConnection.h b/src/cascadia/TerminalConnection/ConptyConnection.h index d2bfe4736da..5e20b11d4bc 100644 --- a/src/cascadia/TerminalConnection/ConptyConnection.h +++ b/src/cascadia/TerminalConnection/ConptyConnection.h @@ -73,7 +73,7 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation hstring _commandline{}; hstring _startingDirectory{}; hstring _startingTitle{}; - bool _initialVisibility{ false }; + bool _initialVisibility{ true }; Windows::Foundation::Collections::ValueSet _environment{ nullptr }; guid _guid{}; // A unique session identifier for connected client hstring _clientName{}; // The name of the process hosted by this ConPTY connection (as of launch). diff --git a/src/cascadia/TerminalControl/ControlCore.cpp b/src/cascadia/TerminalControl/ControlCore.cpp index 82d2a69224c..0f648fd0a49 100644 --- a/src/cascadia/TerminalControl/ControlCore.cpp +++ b/src/cascadia/TerminalControl/ControlCore.cpp @@ -1196,8 +1196,11 @@ namespace winrt::Microsoft::Terminal::Control::implementation void ControlCore::_terminalShowWindowChanged(bool showOrHide) { - auto showWindow = winrt::make_self(showOrHide); - _ShowWindowChangedHandlers(*this, *showWindow); + if (_initializedTerminal) + { + auto showWindow = winrt::make_self(showOrHide); + _ShowWindowChangedHandlers(*this, *showWindow); + } } bool ControlCore::HasSelection() const @@ -1703,10 +1706,13 @@ namespace winrt::Microsoft::Terminal::Control::implementation // - void ControlCore::WindowVisibilityChanged(const bool showOrHide) { - // show is true, hide is false - if (auto conpty{ _connection.try_as() }) + if (_initializedTerminal) { - conpty.ShowHide(showOrHide); + // show is true, hide is false + if (auto conpty{ _connection.try_as() }) + { + conpty.ShowHide(showOrHide); + } } } diff --git a/src/host/PtySignalInputThread.cpp b/src/host/PtySignalInputThread.cpp index 9f3bd98c2cc..935354ebf6e 100644 --- a/src/host/PtySignalInputThread.cpp +++ b/src/host/PtySignalInputThread.cpp @@ -75,11 +75,20 @@ void PtySignalInputThread::ConnectConsole() noexcept _DoShowHide(_initialShowHide->show); } - // If we were given a owner HWND, then manually start the pseudo window now. - if (_earlyReparent) - { - _DoSetWindowParent(*_earlyReparent); - } + // We should have successfully used the _earlyReparent message in CreatePseudoWindow. +} + +// Method Description: +// - Create our pseudo window. We're doing this here, instead of in +// ConnectConsole, because the window is created in +// ConsoleInputThreadProcWin32, before ConnectConsole is first called. Doing +// this here ensures that the window is first created with the initial owner +// set up (if so specified). +// - Refer to GH#13066 for details. +void PtySignalInputThread::CreatePseudoWindow() +{ + HWND owner = _earlyReparent.has_value() ? reinterpret_cast((*_earlyReparent).handle) : HWND_DESKTOP; + ServiceLocator::LocatePseudoWindow(owner); } // Method Description: @@ -227,15 +236,28 @@ void PtySignalInputThread::_DoShowHide(const bool show) // - void PtySignalInputThread::_DoSetWindowParent(const SetParentData& data) { + const auto owner{ reinterpret_cast(data.handle) }; // This will initialize s_interactivityFactory for us. It will also // conveniently return 0 when we're on OneCore. // // If the window hasn't been created yet, by some other call to // LocatePseudoWindow, then this will also initialize the owner of the // window. - if (const auto pseudoHwnd{ ServiceLocator::LocatePseudoWindow(reinterpret_cast(data.handle)) }) + if (const auto pseudoHwnd{ ServiceLocator::LocatePseudoWindow(owner) }) { - LOG_LAST_ERROR_IF_NULL(::SetParent(pseudoHwnd, reinterpret_cast(data.handle))); + // DO NOT USE SetParent HERE! + // + // Calling SetParent on a window that is WS_VISIBLE will cause the OS to + // hide the window, make it a _child_ window, then call SW_SHOW on the + // window to re-show it. SW_SHOW, however, will cause the OS to also set + // that window as the _foreground_ window, which would result in the + // pty's hwnd stealing the foreground away from the owning terminal + // window. That's bad. + // + // SetWindowLongPtr seems to do the job of changing who the window owner + // is, without all the other side effects of reparenting the window. + // See #13066 + ::SetWindowLongPtr(pseudoHwnd, GWLP_HWNDPARENT, reinterpret_cast(owner)); } } diff --git a/src/host/PtySignalInputThread.hpp b/src/host/PtySignalInputThread.hpp index 1ac12f44f33..f9f4268bb9c 100644 --- a/src/host/PtySignalInputThread.hpp +++ b/src/host/PtySignalInputThread.hpp @@ -33,6 +33,7 @@ namespace Microsoft::Console PtySignalInputThread& operator=(const PtySignalInputThread&) = delete; void ConnectConsole() noexcept; + void CreatePseudoWindow(); private: enum class PtySignal : unsigned short diff --git a/src/host/VtIo.cpp b/src/host/VtIo.cpp index a01f5680bf9..adae77d3faa 100644 --- a/src/host/VtIo.cpp +++ b/src/host/VtIo.cpp @@ -300,18 +300,34 @@ bool VtIo::IsUsingVt() const if (_pPtySignalInputThread) { - // IMPORTANT! Start the pseudo window on this thread. This thread has a - // message pump. If you DON'T, then a DPI change in the owning hwnd will - // cause us to get a dpi change as well, which we'll never deque and - // handle, effectively HANGING THE OWNER HWND. + // Let the signal thread know that the console is connected. // - // Let the signal thread know that the console is connected + // By this point, the pseudo window should have already been created, by + // ConsoleInputThreadProcWin32. That thread has a message pump, which is + // needed to ensure that DPI change messages to the owning terminal + // window don't end up hanging because the pty didn't also process it. _pPtySignalInputThread->ConnectConsole(); } return S_OK; } +// Method Description: +// - Create our pseudo window. This is exclusively called by +// ConsoleInputThreadProcWin32 on the console input thread. +// * It needs to be called on that thread, before any other calls to +// LocatePseudoWindow, to make sure that the input thread is the HWND's +// message thread. +// * It needs to be plumbed through the signal thread, because the signal +// thread knows if someone should be marked as the window's owner. It's +// VERY IMPORTANT that any initial owners are set up when the window is +// first created. +// - Refer to GH#13066 for details. +void VtIo::CreatePseudoWindow() +{ + _pPtySignalInputThread->CreatePseudoWindow(); +} + // Method Description: // - Create and start the signal thread. The signal thread can be created // independent of the i/o threads, and doesn't require a client first diff --git a/src/host/VtIo.hpp b/src/host/VtIo.hpp index 3abba39c184..39af4466bb3 100644 --- a/src/host/VtIo.hpp +++ b/src/host/VtIo.hpp @@ -52,6 +52,8 @@ namespace Microsoft::Console::VirtualTerminal [[nodiscard]] HRESULT ManuallyClearScrollback() const noexcept; + void CreatePseudoWindow(); + private: // After CreateIoHandlers is called, these will be invalid. wil::unique_hfile _hInput; diff --git a/src/host/outputStream.cpp b/src/host/outputStream.cpp index 4074139d3a2..82f2d871aa3 100644 --- a/src/host/outputStream.cpp +++ b/src/host/outputStream.cpp @@ -285,7 +285,7 @@ void ConhostInternalGetSet::ShowWindow(bool showOrHide) { const auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); const auto hwnd = gci.IsInVtIoMode() ? ServiceLocator::LocatePseudoWindow() : ServiceLocator::LocateConsoleWindow()->GetWindowHandle(); - ::ShowWindow(hwnd, showOrHide ? SW_NORMAL : SW_MINIMIZE); + ::ShowWindow(hwnd, showOrHide ? SW_SHOWNOACTIVATE : SW_MINIMIZE); } // Routine Description: diff --git a/src/interactivity/base/InteractivityFactory.cpp b/src/interactivity/base/InteractivityFactory.cpp index a4ec388128e..c86df116e4f 100644 --- a/src/interactivity/base/InteractivityFactory.cpp +++ b/src/interactivity/base/InteractivityFactory.cpp @@ -320,8 +320,14 @@ using namespace Microsoft::Console::Interactivity; // as far as the difference between parent/child and owner/owned // windows). Evan K said we should do it this way, and he // definitely knows. - const auto windowStyle = WS_OVERLAPPEDWINDOW; - const auto exStyles = WS_EX_TOOLWINDOW | WS_EX_TRANSPARENT | WS_EX_LAYERED; + // + // GH#13066: Load-bearing: Make sure to set WS_POPUP. If you + // don't, then GetAncestor(GetConsoleWindow(), GA_ROOTOWNER) + // will return the console handle again, not the owning + // terminal's handle. It's not entirely clear why, but WS_POPUP + // is absolutely vital for this to work correctly. + const auto windowStyle = WS_OVERLAPPEDWINDOW | WS_POPUP; + const auto exStyles = WS_EX_TOOLWINDOW | WS_EX_TRANSPARENT | WS_EX_LAYERED | WS_EX_NOACTIVATE; // Attempt to create window. hwnd = CreateWindowExW(exStyles, @@ -335,7 +341,7 @@ using namespace Microsoft::Console::Interactivity; owner, nullptr, nullptr, - nullptr); + this); if (hwnd == nullptr) { diff --git a/src/interactivity/win32/windowio.cpp b/src/interactivity/win32/windowio.cpp index 86a51e09ca6..7816d8ff398 100644 --- a/src/interactivity/win32/windowio.cpp +++ b/src/interactivity/win32/windowio.cpp @@ -1036,9 +1036,18 @@ DWORD WINAPI ConsoleInputThreadProcWin32(LPVOID /*lpParameter*/) // If we are headless (because we're a pseudo console), we // will still need a window handle in the win32 environment // in case anyone sends messages at that HWND (vim.exe is an example.) - // We have to CreateWindow on the same thread that will pump the messages - // which is this thread. - ServiceLocator::LocatePseudoWindow(); + // + // IMPORTANT! We have to CreateWindow on the same thread that will pump + // the messages, which is this thread. If you DON'T, then a DPI change + // in the owning hwnd will cause us to get a dpi change as well, which + // we'll never deque and handle, effectively HANGING THE OWNER HWND. + // ServiceLocator::LocatePseudoWindow(); + // + // Instead of just calling LocatePseudoWindow, make sure to go through + // VtIo's CreatePseudoWindow, which will make sure that the window is + // successfully created with the owner configured when the window is + // first created. See GH#13066 for details. + ServiceLocator::LocateGlobals().getConsoleInformation().GetVtIo()->CreatePseudoWindow(); } UnlockConsole(); diff --git a/src/terminal/adapter/InteractDispatch.cpp b/src/terminal/adapter/InteractDispatch.cpp index 8da0f121a54..303f8145d15 100644 --- a/src/terminal/adapter/InteractDispatch.cpp +++ b/src/terminal/adapter/InteractDispatch.cpp @@ -199,11 +199,19 @@ bool InteractDispatch::FocusChanged(const bool focused) const { // They want focus, we found a pseudo hwnd. - // Note: ::GetParent(pseudoHwnd) will return 0. GetAncestor works though. - // GA_PARENT and GA_ROOT seemingly return the same thing for - // Terminal. We're going with GA_ROOT since it seems - // semantically more correct here. - if (const auto ownerHwnd{ ::GetAncestor(pseudoHwnd, GA_ROOT) }) + // BODGY + // + // This needs to be GA_ROOTOWNER here. Not GA_ROOT, GA_PARENT, + // or GetParent. The ConPTY hwnd is an owned, top-level, popup, + // non-parented window. It does not have a parent set. It does + // have an owner set. It is not a WS_CHILD window. This + // combination of things allows us to find the owning window + // with GA_ROOTOWNER. GA_ROOT will get us ourselves, and + // GA_PARENT will return the desktop HWND. + // + // See GH#13066 + + if (const auto ownerHwnd{ ::GetAncestor(pseudoHwnd, GA_ROOTOWNER) }) { // We have an owner from a previous call to ReparentWindow