From a3c12bf3da080c29a73440ff95f2cb737e38fd91 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Fri, 30 Aug 2024 01:18:05 +0200 Subject: [PATCH] Avoid focus loops in ConPTY --- .github/actions/spelling/allow/apis.txt | 1 + src/cascadia/TerminalControl/ControlCore.cpp | 23 ++++++- src/cascadia/TerminalControl/ControlCore.h | 1 + src/host/PtySignalInputThread.cpp | 46 +------------- src/host/PtySignalInputThread.hpp | 3 - src/host/exe/Host.EXE.vcxproj | 2 +- src/host/outputStream.cpp | 11 +++- .../base/InteractivityFactory.cpp | 60 +++++++++++++++---- .../base/InteractivityFactory.hpp | 7 ++- src/interactivity/base/ServiceLocator.cpp | 36 ++++++++++- .../inc/IInteractivityFactory.hpp | 2 +- src/interactivity/inc/ServiceLocator.hpp | 4 +- src/terminal/adapter/InteractDispatch.cpp | 55 +---------------- 13 files changed, 131 insertions(+), 120 deletions(-) diff --git a/.github/actions/spelling/allow/apis.txt b/.github/actions/spelling/allow/apis.txt index b60e1b26a3..364080afcd 100644 --- a/.github/actions/spelling/allow/apis.txt +++ b/.github/actions/spelling/allow/apis.txt @@ -96,6 +96,7 @@ IGraphics IImage IInheritable IMap +imm IMonarch IObject iosfwd diff --git a/src/cascadia/TerminalControl/ControlCore.cpp b/src/cascadia/TerminalControl/ControlCore.cpp index cd4f5f58b8..6928f4ba35 100644 --- a/src/cascadia/TerminalControl/ControlCore.cpp +++ b/src/cascadia/TerminalControl/ControlCore.cpp @@ -201,6 +201,17 @@ namespace winrt::Microsoft::Terminal::Control::implementation } }); + // If you rapidly show/hide Windows Terminal, something about GotFocus()/LostFocus() gets broken. + // We'll then receive easily 10+ such calls from WinUI the next time the application is shown. + shared->focusChanged = std::make_unique>( + std::chrono::milliseconds{ 25 }, + [weakThis = get_weak()](const bool focused) { + if (const auto core{ weakThis.get() }) + { + core->_focusChanged(focused); + } + }); + // Scrollbar updates are also expensive (XAML), so we'll throttle them as well. shared->updateScrollBar = std::make_shared>( _dispatcher, @@ -2480,13 +2491,21 @@ namespace winrt::Microsoft::Terminal::Control::implementation // - This is related to work done for GH#2988. void ControlCore::GotFocus() { - _focusChanged(true); + const auto shared = _shared.lock_shared(); + if (shared->focusChanged) + { + (*shared->focusChanged)(true); + } } // See GotFocus. void ControlCore::LostFocus() { - _focusChanged(false); + const auto shared = _shared.lock_shared(); + if (shared->focusChanged) + { + (*shared->focusChanged)(false); + } } void ControlCore::_focusChanged(bool focused) diff --git a/src/cascadia/TerminalControl/ControlCore.h b/src/cascadia/TerminalControl/ControlCore.h index 9ac8e5450c..94d60fc95a 100644 --- a/src/cascadia/TerminalControl/ControlCore.h +++ b/src/cascadia/TerminalControl/ControlCore.h @@ -307,6 +307,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation struct SharedState { std::unique_ptr> outputIdle; + std::unique_ptr> focusChanged; std::shared_ptr> updateScrollBar; }; diff --git a/src/host/PtySignalInputThread.cpp b/src/host/PtySignalInputThread.cpp index 45860f9c1a..8b0570a67b 100644 --- a/src/host/PtySignalInputThread.cpp +++ b/src/host/PtySignalInputThread.cpp @@ -74,8 +74,6 @@ void PtySignalInputThread::ConnectConsole() noexcept { _DoShowHide(*_initialShowHide); } - - // We should have successfully used the _earlyReparent message in CreatePseudoWindow. } // Method Description: @@ -87,8 +85,7 @@ void PtySignalInputThread::ConnectConsole() noexcept // - Refer to GH#13066 for details. void PtySignalInputThread::CreatePseudoWindow() { - HWND owner = _earlyReparent.has_value() ? reinterpret_cast((*_earlyReparent).handle) : HWND_DESKTOP; - ServiceLocator::LocatePseudoWindow(owner); + ServiceLocator::LocatePseudoWindow(); } // Method Description: @@ -220,7 +217,7 @@ void PtySignalInputThread::_DoShowHide(const ShowHideData& data) return; } - _api.ShowWindow(data.show); + ServiceLocator::SetPseudoWindowVisibility(data.show); } // Method Description: @@ -237,44 +234,7 @@ void PtySignalInputThread::_DoSetWindowParent(const SetParentData& data) LockConsole(); auto Unlock = wil::scope_exit([&] { UnlockConsole(); }); - // If the client app hasn't yet connected, stash the new owner. - // We'll later (PtySignalInputThread::ConnectConsole) use the value - // to set up the owner of the conpty window. - if (!_consoleConnected) - { - _earlyReparent = data; - return; - } - - 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(owner) }) - { - // SetWindowLongPtrW may call back into the message handler and wait for it to finish, - // similar to SendMessageW(). If the conhost message handler is already processing and - // waiting to acquire the console lock, which we're currently holding, we'd deadlock. - // --> Release the lock now. - Unlock.reset(); - - // 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 - ::SetWindowLongPtrW(pseudoHwnd, GWLP_HWNDPARENT, reinterpret_cast(owner)); - } + ServiceLocator::SetPseudoWindowOwner(reinterpret_cast(data.handle)); } // Method Description: diff --git a/src/host/PtySignalInputThread.hpp b/src/host/PtySignalInputThread.hpp index 0534c0600f..eead316ba9 100644 --- a/src/host/PtySignalInputThread.hpp +++ b/src/host/PtySignalInputThread.hpp @@ -75,8 +75,5 @@ namespace Microsoft::Console std::optional _earlyResize; std::optional _initialShowHide; ConhostInternalGetSet _api; - - public: - std::optional _earlyReparent; }; } diff --git a/src/host/exe/Host.EXE.vcxproj b/src/host/exe/Host.EXE.vcxproj index 0bc466b548..f6151b95eb 100644 --- a/src/host/exe/Host.EXE.vcxproj +++ b/src/host/exe/Host.EXE.vcxproj @@ -86,7 +86,7 @@ true - WinMM.Lib;%(AdditionalDependencies) + winmm.lib;imm32.lib;%(AdditionalDependencies) diff --git a/src/host/outputStream.cpp b/src/host/outputStream.cpp index d88be99ef3..0913199db2 100644 --- a/src/host/outputStream.cpp +++ b/src/host/outputStream.cpp @@ -208,8 +208,14 @@ CursorType ConhostInternalGetSet::GetUserDefaultCursorStyle() const // - void ConhostInternalGetSet::ShowWindow(bool showOrHide) { - auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); - const auto hwnd = gci.IsInVtIoMode() ? ServiceLocator::LocatePseudoWindow() : ServiceLocator::LocateConsoleWindow()->GetWindowHandle(); + // ConPTY is supposed to be "transparent" to the VT application. Any VT it processes is given to the terminal. + // As such, it must not react to this "CSI 1 t" or "CSI 2 t" sequence. That's the job of the terminal. + // If the terminal encounters such a sequence, it can show/hide itself and let ConPTY know via its signal API. + const auto window = ServiceLocator::LocateConsoleWindow(); + if (!window) + { + return; + } // GH#13301 - When we send this ShowWindow message, if we send it to the // conhost HWND, it's going to need to get processed by the window message @@ -217,6 +223,7 @@ void ConhostInternalGetSet::ShowWindow(bool showOrHide) // However, ShowWindowAsync doesn't have this problem. It'll post the // message to the window thread, then immediately return, so we don't have // to worry about deadlocking. + const auto hwnd = window->GetWindowHandle(); ::ShowWindowAsync(hwnd, showOrHide ? SW_SHOWNOACTIVATE : SW_MINIMIZE); } diff --git a/src/interactivity/base/InteractivityFactory.cpp b/src/interactivity/base/InteractivityFactory.cpp index 862b4db514..8b5647bac6 100644 --- a/src/interactivity/base/InteractivityFactory.cpp +++ b/src/interactivity/base/InteractivityFactory.cpp @@ -294,7 +294,7 @@ using namespace Microsoft::Console::Interactivity; // - owner: the HWND that should be the initial owner of the pseudo window. // Return Value: // - STATUS_SUCCESS on success, otherwise an appropriate error. -[[nodiscard]] NTSTATUS InteractivityFactory::CreatePseudoWindow(HWND& hwnd, const HWND owner) +[[nodiscard]] NTSTATUS InteractivityFactory::CreatePseudoWindow(HWND& hwnd) { hwnd = nullptr; ApiLevel level; @@ -310,6 +310,11 @@ using namespace Microsoft::Console::Interactivity; { case ApiLevel::Win32: { + // We don't need an "Default IME" window for ConPTY. That's the terminal's job. + // -1, aka DWORD_MAX, tells the function to disable it for the entire process. + // Must be called before creating any window. + ImmDisableIME(DWORD_MAX); + pseudoClass.cbSize = sizeof(WNDCLASSEXW); pseudoClass.lpszClassName = PSEUDO_WINDOW_CLASS; pseudoClass.lpfnWndProc = s_PseudoWindowProc; @@ -330,19 +335,15 @@ using namespace Microsoft::Console::Interactivity; // 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, + hwnd = CreateWindowExW(WS_EX_TOOLWINDOW | WS_EX_NOACTIVATE, reinterpret_cast(windowClassAtom), nullptr, - windowStyle, + WS_POPUP, 0, 0, 0, 0, - owner, + _owner.load(std::memory_order_relaxed), nullptr, nullptr, this); @@ -375,6 +376,38 @@ using namespace Microsoft::Console::Interactivity; return status; } +void InteractivityFactory::SetOwner(HWND owner) noexcept +{ + _owner.store(owner, std::memory_order_relaxed); + + if (_pseudoConsoleWindowHwnd) + { + // 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 + ::SetWindowLongPtrW(_pseudoConsoleWindowHwnd, GWLP_HWNDPARENT, reinterpret_cast(owner)); + } +} + +void InteractivityFactory::SetVisibility(const bool isVisible) noexcept +{ + if (_pseudoConsoleWindowHwnd && IsIconic(_pseudoConsoleWindowHwnd) != static_cast(isVisible)) + { + _suppressVisibilityChange.store(true, std::memory_order_relaxed); + ShowWindow(_pseudoConsoleWindowHwnd, isVisible ? SW_SHOWNOACTIVATE : SW_MINIMIZE); + _suppressVisibilityChange.store(false, std::memory_order_relaxed); + } +} + // Method Description: // - Static window procedure for pseudo console windows // - Processes set-up on create to stow the "this" pointer to specific instantiations and routes @@ -446,7 +479,7 @@ using namespace Microsoft::Console::Interactivity; { _WritePseudoWindowCallback(false); } - break; + return 0; } // case WM_WINDOWPOSCHANGING: // As long as user32 didn't eat the `ShowWindow` call because the window state requested @@ -479,12 +512,12 @@ using namespace Microsoft::Console::Interactivity; } case WM_ACTIVATE: { - if (const auto ownerHwnd{ ::GetAncestor(hWnd, GA_ROOTOWNER) }) + if (const auto ownerHwnd = _owner.load(std::memory_order_relaxed)) { SetFocus(ownerHwnd); return 0; } - break; + return 0; } } // If we get this far, call the default window proc @@ -501,6 +534,11 @@ using namespace Microsoft::Console::Interactivity; // - void InteractivityFactory::_WritePseudoWindowCallback(bool showOrHide) { + if (_suppressVisibilityChange.load(std::memory_order_relaxed)) + { + return; + } + // IMPORTANT! // // A hosting terminal window should only "restore" itself in response to diff --git a/src/interactivity/base/InteractivityFactory.hpp b/src/interactivity/base/InteractivityFactory.hpp index 2a0b3460ee..7e9b912e85 100644 --- a/src/interactivity/base/InteractivityFactory.hpp +++ b/src/interactivity/base/InteractivityFactory.hpp @@ -27,7 +27,10 @@ namespace Microsoft::Console::Interactivity [[nodiscard]] NTSTATUS CreateAccessibilityNotifier(_Inout_ std::unique_ptr& notifier); [[nodiscard]] NTSTATUS CreateSystemConfigurationProvider(_Inout_ std::unique_ptr& provider); - [[nodiscard]] NTSTATUS CreatePseudoWindow(HWND& hwnd, const HWND owner); + [[nodiscard]] NTSTATUS CreatePseudoWindow(HWND& hwnd); + + void SetOwner(HWND owner) noexcept; + void SetVisibility(const bool isVisible) noexcept; // Wndproc [[nodiscard]] static LRESULT CALLBACK s_PseudoWindowProc(_In_ HWND hwnd, @@ -43,6 +46,8 @@ namespace Microsoft::Console::Interactivity void _WritePseudoWindowCallback(bool showOrHide); HWND _pseudoConsoleWindowHwnd{ nullptr }; + std::atomic _owner{ HWND_DESKTOP }; + std::atomic _suppressVisibilityChange{ false }; WRL::ComPtr _pPseudoConsoleUiaProvider{ nullptr }; }; } diff --git a/src/interactivity/base/ServiceLocator.cpp b/src/interactivity/base/ServiceLocator.cpp index f73182e8a5..24613d852f 100644 --- a/src/interactivity/base/ServiceLocator.cpp +++ b/src/interactivity/base/ServiceLocator.cpp @@ -322,7 +322,7 @@ Globals& ServiceLocator::LocateGlobals() // owner of the pseudo window. // Return Value: // - a reference to the pseudoconsole window. -HWND ServiceLocator::LocatePseudoWindow(const HWND owner) +HWND ServiceLocator::LocatePseudoWindow() { auto status = STATUS_SUCCESS; if (!s_pseudoWindowInitialized) @@ -335,7 +335,7 @@ HWND ServiceLocator::LocatePseudoWindow(const HWND owner) if (SUCCEEDED_NTSTATUS(status)) { HWND hwnd; - status = s_interactivityFactory->CreatePseudoWindow(hwnd, owner); + status = s_interactivityFactory->CreatePseudoWindow(hwnd); s_pseudoWindow.reset(hwnd); } @@ -345,6 +345,38 @@ HWND ServiceLocator::LocatePseudoWindow(const HWND owner) return s_pseudoWindow.get(); } +void ServiceLocator::SetPseudoWindowOwner(HWND owner) +{ + auto status = STATUS_SUCCESS; + if (!s_interactivityFactory) + { + status = ServiceLocator::LoadInteractivityFactory(); + } + + if (s_interactivityFactory) + { + static_cast(s_interactivityFactory.get())->SetOwner(owner); + } + + LOG_IF_NTSTATUS_FAILED(status); +} + +void ServiceLocator::SetPseudoWindowVisibility(bool showOrHide) +{ + auto status = STATUS_SUCCESS; + if (!s_interactivityFactory) + { + status = ServiceLocator::LoadInteractivityFactory(); + } + + if (s_interactivityFactory) + { + static_cast(s_interactivityFactory.get())->SetVisibility(showOrHide); + } + + LOG_IF_NTSTATUS_FAILED(status); +} + #pragma endregion #pragma region Private Methods diff --git a/src/interactivity/inc/IInteractivityFactory.hpp b/src/interactivity/inc/IInteractivityFactory.hpp index d974933475..99fb20f282 100644 --- a/src/interactivity/inc/IInteractivityFactory.hpp +++ b/src/interactivity/inc/IInteractivityFactory.hpp @@ -40,6 +40,6 @@ namespace Microsoft::Console::Interactivity [[nodiscard]] virtual NTSTATUS CreateAccessibilityNotifier(_Inout_ std::unique_ptr& notifier) = 0; [[nodiscard]] virtual NTSTATUS CreateSystemConfigurationProvider(_Inout_ std::unique_ptr& provider) = 0; - [[nodiscard]] virtual NTSTATUS CreatePseudoWindow(HWND& hwnd, const HWND owner) = 0; + [[nodiscard]] virtual NTSTATUS CreatePseudoWindow(HWND& hwnd) = 0; }; } diff --git a/src/interactivity/inc/ServiceLocator.hpp b/src/interactivity/inc/ServiceLocator.hpp index 0bc0131bc2..5ed052ca3a 100644 --- a/src/interactivity/inc/ServiceLocator.hpp +++ b/src/interactivity/inc/ServiceLocator.hpp @@ -84,7 +84,9 @@ namespace Microsoft::Console::Interactivity static Globals& LocateGlobals(); - static HWND LocatePseudoWindow(const HWND owner = nullptr /*HWND_DESKTOP = 0*/); + static HWND LocatePseudoWindow(); + static void SetPseudoWindowOwner(HWND owner); + static void SetPseudoWindowVisibility(bool showOrHide); ServiceLocator(const ServiceLocator&) = delete; ServiceLocator& operator=(const ServiceLocator&) = delete; diff --git a/src/terminal/adapter/InteractDispatch.cpp b/src/terminal/adapter/InteractDispatch.cpp index 1f859c6916..d9805fe959 100644 --- a/src/terminal/adapter/InteractDispatch.cpp +++ b/src/terminal/adapter/InteractDispatch.cpp @@ -162,59 +162,8 @@ void InteractDispatch::FocusChanged(const bool focused) // InteractDispatch outside ConPTY mode, but just in case... if (gci.IsInVtIoMode()) { - auto shouldActuallyFocus = false; - - // From https://github.com/microsoft/terminal/pull/12799#issuecomment-1086289552 - // Make sure that the process that's telling us it's focused, actually - // _is_ in the FG. We don't want to allow malicious.exe to say "yep I'm - // in the foreground, also, here's a popup" if it isn't actually in the - // FG. - if (focused) - { - if (const auto pseudoHwnd{ ServiceLocator::LocatePseudoWindow() }) - { - // They want focus, we found a pseudo hwnd. - - // 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 - - if (const auto currentFgWindow{ ::GetForegroundWindow() }) - { - // There is a window in the foreground (it's possible there - // isn't one) - - // Get the PID of the current FG window, and compare with our owner's PID. - DWORD currentFgPid{ 0 }; - DWORD ownerPid{ 0 }; - GetWindowThreadProcessId(currentFgWindow, ¤tFgPid); - GetWindowThreadProcessId(ownerHwnd, &ownerPid); - - if (ownerPid == currentFgPid) - { - // Huzzah, the app that owns us is actually the FG - // process. They're allowed to grand FG rights. - shouldActuallyFocus = true; - } - } - } - } - } - - WI_UpdateFlag(gci.Flags, CONSOLE_HAS_FOCUS, shouldActuallyFocus); - gci.ProcessHandleList.ModifyConsoleProcessFocus(shouldActuallyFocus); + WI_UpdateFlag(gci.Flags, CONSOLE_HAS_FOCUS, focused); + gci.ProcessHandleList.ModifyConsoleProcessFocus(focused); gci.pInputBuffer->WriteFocusEvent(focused); } // Does nothing outside of ConPTY. If there's a real HWND, then the HWND is solely in charge.