From df6c43fc69a021a26624be65361854853950b0bb Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 9 Jun 2022 18:12:26 -0500 Subject: [PATCH] Make sure foreground access works for DefTerm (#13247) See also: #12799, the origin of much of this. This change evolved over multiple phases. When we create a defterm connection in `TerminalPage::_OnNewConnection`, we don't have the hosting HWND yet, so the tab gets created without one. We'll later get called with the owner, in `Initialize`. To remedy this, we need to: * In `Initialize`, make sure to update any existing controls with the new owner. * In `ControlCore`, actually propogate the new owner down to the connection DefTerm launches don't actually request focus mode, so the Terminal never sends them focus events. We need those focus events so that the console can request foreground rights. To remedy this, we need to: * pass `--win32input` to the commandline used to initialize OpenConsole in ConPTY mode. We request focus events at the same time we request win32-input-mode. * I also added `--resizeQuirk`, because _by all accounts that should be there_. Resizing in defterm windows should be _wacky_ without it, and I'm a little surprised we haven't seen any bugs due to this yet. `ConsoleSetForeground` expects a `HANDLE` to the process we want to give foreground rights to. The problem is, the wire format we used _also_ decided that a HANDLE value was a good idea. It's not. If we pass the literal value of the HANDLE to the process from OpenConsole to conhost, so conhost can call that API, the value that conhost uses there will most likely be an invalid handle. The HANDLE's value is its value in _OpenConsole_, not in conhost. To remedy this, we need to: * Just not forward `ConsoleSetForeground`. Turns out, we _can_ just call that in OpenConsole safely. There's no validation. So just instantiate a static version of the Win32 version of ConsoleControl, just to use for SetForeground. (thanks Dustin) * [x] Tested manually - Win+R `powershell`, `notepad` spawns on top. Closes #13211 (cherry picked from commit 2a7dd8b73075fffd3d1220842ae381f536f7c35b) Service-Card-Id: 83026847 Service-Version: 1.14 --- src/cascadia/TerminalApp/TerminalPage.cpp | 23 +++++++++++++++++++ src/cascadia/TerminalControl/ControlCore.cpp | 21 +++++++++++++++-- src/cascadia/TerminalControl/ControlCore.h | 8 +++---- src/host/ConsoleArguments.cpp | 6 +++++ src/host/input.cpp | 2 +- src/host/srvinit.cpp | 7 +++++- src/inc/HostSignals.hpp | 8 +++---- .../base/HostSignalInputThread.cpp | 8 ++++--- .../base/RemoteConsoleControl.cpp | 14 +++++------ .../base/RemoteConsoleControl.hpp | 5 +++- src/interactivity/inc/IConsoleControl.hpp | 2 +- src/interactivity/onecore/ConsoleControl.cpp | 4 ++-- src/interactivity/onecore/ConsoleControl.hpp | 2 +- src/interactivity/win32/ConsoleControl.cpp | 4 ++-- src/interactivity/win32/ConsoleControl.hpp | 2 +- src/server/ProcessList.cpp | 2 +- 16 files changed, 86 insertions(+), 32 deletions(-) diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index fdec1392530..1e011e71efa 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -69,6 +69,29 @@ namespace winrt::TerminalApp::implementation // - see GH#2988 HRESULT TerminalPage::Initialize(HWND hwnd) { + if (!_hostingHwnd.has_value()) + { + // GH#13211 - if we haven't yet set the owning hwnd, reparent all the controls now. + for (const auto& tab : _tabs) + { + if (auto terminalTab{ _GetTerminalTabImpl(tab) }) + { + terminalTab->GetRootPane()->WalkTree([&](auto&& pane) { + if (const auto& term{ pane->GetTerminalControl() }) + { + term.OwningHwnd(reinterpret_cast(hwnd)); + } + }); + } + // We don't need to worry about resetting the owning hwnd for the + // SUI here. GH#13211 only repros for a defterm connection, where + // the tab is spawned before the window is created. It's not + // possible to make a SUI tab like that, before the window is + // created. The SUI could be spawned as a part of a window restore, + // but that would still work fine. The window would be created + // before restoring previous tabs in that scenario. + } + } _hostingHwnd = hwnd; return S_OK; } diff --git a/src/cascadia/TerminalControl/ControlCore.cpp b/src/cascadia/TerminalControl/ControlCore.cpp index d40a9207623..a4a087ca1fb 100644 --- a/src/cascadia/TerminalControl/ControlCore.cpp +++ b/src/cascadia/TerminalControl/ControlCore.cpp @@ -264,11 +264,11 @@ namespace winrt::Microsoft::Terminal::Control::implementation const auto height = vp.Height(); _connection.Resize(height, width); - if (_OwningHwnd != 0) + if (_owningHwnd != 0) { if (auto conpty{ _connection.try_as() }) { - conpty.ReparentWindow(_OwningHwnd); + conpty.ReparentWindow(_owningHwnd); } } @@ -1764,4 +1764,21 @@ namespace winrt::Microsoft::Terminal::Control::implementation // transparency, or our acrylic, or our image. return Opacity() < 1.0f || UseAcrylic() || !_settings->BackgroundImage().empty() || _settings->UseBackgroundImageForWindow(); } + + uint64_t ControlCore::OwningHwnd() + { + return _owningHwnd; + } + + void ControlCore::OwningHwnd(uint64_t owner) + { + if (owner != _owningHwnd && _connection) + { + if (auto conpty{ _connection.try_as() }) + { + conpty.ReparentWindow(owner); + } + } + _owningHwnd = owner; + } } diff --git a/src/cascadia/TerminalControl/ControlCore.h b/src/cascadia/TerminalControl/ControlCore.h index dfa3d445a95..894c4178944 100644 --- a/src/cascadia/TerminalControl/ControlCore.h +++ b/src/cascadia/TerminalControl/ControlCore.h @@ -176,10 +176,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation void WindowVisibilityChanged(const bool showOrHide); - // TODO:GH#1256 - When a tab can be torn out or otherwise reparented to - // another window, this value will need a custom setter, so that we can - // also update the connection. - WINRT_PROPERTY(uint64_t, OwningHwnd, 0); + uint64_t OwningHwnd(); + void OwningHwnd(uint64_t owner); RUNTIME_SETTING(double, Opacity, _settings->Opacity()); RUNTIME_SETTING(bool, UseAcrylic, _settings->UseAcrylic()); @@ -249,6 +247,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation double _panelHeight{ 0 }; double _compositionScale{ 0 }; + uint64_t _owningHwnd{ 0 }; + winrt::Windows::System::DispatcherQueue _dispatcher{ nullptr }; std::shared_ptr> _tsfTryRedrawCanvas; std::shared_ptr> _updatePatternLocations; diff --git a/src/host/ConsoleArguments.cpp b/src/host/ConsoleArguments.cpp index f89bf249732..2541cc8ab82 100644 --- a/src/host/ConsoleArguments.cpp +++ b/src/host/ConsoleArguments.cpp @@ -25,6 +25,12 @@ const std::wstring_view ConsoleArguments::FEATURE_ARG = L"--feature"; const std::wstring_view ConsoleArguments::FEATURE_PTY_ARG = L"pty"; const std::wstring_view ConsoleArguments::COM_SERVER_ARG = L"-Embedding"; const std::wstring_view ConsoleArguments::PASSTHROUGH_ARG = L"--passthrough"; +// NOTE: Thinking about adding more commandline args that control conpty, for +// the Terminal? Make sure you add them to the commandline in +// ConsoleEstablishHandoff. We use that to initialize the ConsoleArguments for a +// defterm handoff s.t. they behave like a conpty connection that was started by +// the terminal. If you forget them there, the conpty won't obey them, only for +// defterm. std::wstring EscapeArgument(std::wstring_view ac) { diff --git a/src/host/input.cpp b/src/host/input.cpp index a689a9454c7..e49dee1a8be 100644 --- a/src/host/input.cpp +++ b/src/host/input.cpp @@ -326,7 +326,7 @@ void ProcessCtrlEvents() if (NT_SUCCESS(Status)) { Status = ServiceLocator::LocateConsoleControl() - ->EndTask((HANDLE)rgProcessHandleList[i].dwProcessID, + ->EndTask(rgProcessHandleList[i].dwProcessID, EventType, CtrlFlags); if (rgProcessHandleList[i].hProcess == nullptr) diff --git a/src/host/srvinit.cpp b/src/host/srvinit.cpp index f963999341a..a04648812cf 100644 --- a/src/host/srvinit.cpp +++ b/src/host/srvinit.cpp @@ -536,7 +536,12 @@ try outPipeTheirSide.reset(); signalPipeTheirSide.reset(); - const auto commandLine = fmt::format(FMT_COMPILE(L" --headless --signal {:#x}"), (int64_t)signalPipeOurSide.release()); + // GH#13211 - Make sure we request win32input mode and that the terminal + // obeys the resizing quirk. Otherwise, defterm connections to the Terminal + // are going to have weird resizing, and aren't going to send full fidelity + // input messages. + const auto commandLine = fmt::format(FMT_COMPILE(L" --headless --resizeQuirk --win32input --signal {:#x}"), + (int64_t)signalPipeOurSide.release()); ConsoleArguments consoleArgs(commandLine, inPipeOurSide.release(), outPipeOurSide.release()); RETURN_IF_FAILED(consoleArgs.ParseCommandline()); diff --git a/src/inc/HostSignals.hpp b/src/inc/HostSignals.hpp index 05e1d0f3f72..ec29e540646 100644 --- a/src/inc/HostSignals.hpp +++ b/src/inc/HostSignals.hpp @@ -13,21 +13,21 @@ namespace Microsoft::Console struct HostSignalNotifyAppData { uint32_t sizeInBytes; - uint32_t processId; + uint32_t processId; // THIS IS A PID }; struct HostSignalSetForegroundData { uint32_t sizeInBytes; - uint32_t processId; + uint32_t processId; // THIS IS A HANDLE, NOT A PID bool isForeground; }; struct HostSignalEndTaskData { uint32_t sizeInBytes; - uint32_t processId; + uint32_t processId; // THIS IS A PID uint32_t eventType; uint32_t ctrlFlags; }; -}; \ No newline at end of file +}; diff --git a/src/interactivity/base/HostSignalInputThread.cpp b/src/interactivity/base/HostSignalInputThread.cpp index 397131b08e4..bed01bbd013 100644 --- a/src/interactivity/base/HostSignalInputThread.cpp +++ b/src/interactivity/base/HostSignalInputThread.cpp @@ -94,9 +94,11 @@ T HostSignalInputThread::_ReceiveTypedPacket() } case HostSignals::SetForeground: { - auto msg = _ReceiveTypedPacket(); + _ReceiveTypedPacket(); - LOG_IF_NTSTATUS_FAILED(ServiceLocator::LocateConsoleControl()->SetForeground(ULongToHandle(msg.processId), msg.isForeground)); + // GH#13211 - This honestly shouldn't be called by OpenConsole + // anymore, but it's possible that a much older version of + // OpenConsole is still calling this. Just do nothing. break; } @@ -104,7 +106,7 @@ T HostSignalInputThread::_ReceiveTypedPacket() { auto msg = _ReceiveTypedPacket(); - LOG_IF_NTSTATUS_FAILED(ServiceLocator::LocateConsoleControl()->EndTask(ULongToHandle(msg.processId), msg.eventType, msg.ctrlFlags)); + LOG_IF_NTSTATUS_FAILED(ServiceLocator::LocateConsoleControl()->EndTask(msg.processId, msg.eventType, msg.ctrlFlags)); break; } diff --git a/src/interactivity/base/RemoteConsoleControl.cpp b/src/interactivity/base/RemoteConsoleControl.cpp index 21b8155a79f..7cd03e36317 100644 --- a/src/interactivity/base/RemoteConsoleControl.cpp +++ b/src/interactivity/base/RemoteConsoleControl.cpp @@ -57,19 +57,17 @@ template [[nodiscard]] NTSTATUS RemoteConsoleControl::SetForeground(_In_ HANDLE hProcess, _In_ BOOL fForeground) { - HostSignalSetForegroundData data{}; - data.sizeInBytes = sizeof(data); - data.processId = HandleToULong(hProcess); - data.isForeground = fForeground; - - return _SendTypedPacket(_pipe.get(), HostSignals::SetForeground, data); + // GH#13211 - Apparently this API doesn't need to be forwarded to conhost at + // all. Instead, just perform the ConsoleControl operation here, in proc. + // This lets us avoid all sorts of strange handle duplicating weirdness. + return _control.SetForeground(hProcess, fForeground); } -[[nodiscard]] NTSTATUS RemoteConsoleControl::EndTask(_In_ HANDLE hProcessId, _In_ DWORD dwEventType, _In_ ULONG ulCtrlFlags) +[[nodiscard]] NTSTATUS RemoteConsoleControl::EndTask(_In_ DWORD dwProcessId, _In_ DWORD dwEventType, _In_ ULONG ulCtrlFlags) { HostSignalEndTaskData data{}; data.sizeInBytes = sizeof(data); - data.processId = HandleToULong(hProcessId); + data.processId = dwProcessId; data.eventType = dwEventType; data.ctrlFlags = ulCtrlFlags; diff --git a/src/interactivity/base/RemoteConsoleControl.hpp b/src/interactivity/base/RemoteConsoleControl.hpp index 52de8db1eb3..b922b9beaa6 100644 --- a/src/interactivity/base/RemoteConsoleControl.hpp +++ b/src/interactivity/base/RemoteConsoleControl.hpp @@ -14,6 +14,7 @@ Author(s): #pragma once #include "../inc/IConsoleControl.hpp" +#include "../win32/ConsoleControl.hpp" namespace Microsoft::Console::Interactivity { @@ -25,9 +26,11 @@ namespace Microsoft::Console::Interactivity // IConsoleControl Members [[nodiscard]] NTSTATUS NotifyConsoleApplication(_In_ DWORD dwProcessId); [[nodiscard]] NTSTATUS SetForeground(_In_ HANDLE hProcess, _In_ BOOL fForeground); - [[nodiscard]] NTSTATUS EndTask(_In_ HANDLE hProcessId, _In_ DWORD dwEventType, _In_ ULONG ulCtrlFlags); + [[nodiscard]] NTSTATUS EndTask(_In_ DWORD dwProcessId, _In_ DWORD dwEventType, _In_ ULONG ulCtrlFlags); private: wil::unique_handle _pipe; + + Win32::ConsoleControl _control; }; } diff --git a/src/interactivity/inc/IConsoleControl.hpp b/src/interactivity/inc/IConsoleControl.hpp index 02d83e1b97e..7129cff68ff 100644 --- a/src/interactivity/inc/IConsoleControl.hpp +++ b/src/interactivity/inc/IConsoleControl.hpp @@ -23,6 +23,6 @@ namespace Microsoft::Console::Interactivity virtual ~IConsoleControl() = default; [[nodiscard]] virtual NTSTATUS NotifyConsoleApplication(DWORD dwProcessId) = 0; [[nodiscard]] virtual NTSTATUS SetForeground(HANDLE hProcess, BOOL fForeground) = 0; - [[nodiscard]] virtual NTSTATUS EndTask(HANDLE hProcessId, DWORD dwEventType, ULONG ulCtrlFlags) = 0; + [[nodiscard]] virtual NTSTATUS EndTask(DWORD dwProcessId, DWORD dwEventType, ULONG ulCtrlFlags) = 0; }; } diff --git a/src/interactivity/onecore/ConsoleControl.cpp b/src/interactivity/onecore/ConsoleControl.cpp index 662a2b6b1e9..248d02b3ac8 100644 --- a/src/interactivity/onecore/ConsoleControl.cpp +++ b/src/interactivity/onecore/ConsoleControl.cpp @@ -22,13 +22,13 @@ using namespace Microsoft::Console::Interactivity::OneCore; return STATUS_SUCCESS; } -[[nodiscard]] NTSTATUS ConsoleControl::EndTask(_In_ HANDLE hProcessId, _In_ DWORD dwEventType, _In_ ULONG ulCtrlFlags) +[[nodiscard]] NTSTATUS ConsoleControl::EndTask(_In_ DWORD dwProcessId, _In_ DWORD dwEventType, _In_ ULONG ulCtrlFlags) { USER_API_MSG m{}; const auto a = &m.u.EndTask; RtlZeroMemory(a, sizeof(*a)); - a->ProcessId = hProcessId; + a->ProcessId = ULongToHandle(dwProcessId); // This is actually a PID, even though the struct expects a HANDLE. a->ConsoleEventCode = dwEventType; a->ConsoleFlags = ulCtrlFlags; diff --git a/src/interactivity/onecore/ConsoleControl.hpp b/src/interactivity/onecore/ConsoleControl.hpp index 9b406a475f4..4771c6c2220 100644 --- a/src/interactivity/onecore/ConsoleControl.hpp +++ b/src/interactivity/onecore/ConsoleControl.hpp @@ -26,6 +26,6 @@ namespace Microsoft::Console::Interactivity::OneCore // IConsoleControl Members [[nodiscard]] NTSTATUS NotifyConsoleApplication(_In_ DWORD dwProcessId) noexcept override; [[nodiscard]] NTSTATUS SetForeground(_In_ HANDLE hProcess, _In_ BOOL fForeground) noexcept override; - [[nodiscard]] NTSTATUS EndTask(_In_ HANDLE hProcessId, _In_ DWORD dwEventType, _In_ ULONG ulCtrlFlags) override; + [[nodiscard]] NTSTATUS EndTask(_In_ DWORD dwProcessId, _In_ DWORD dwEventType, _In_ ULONG ulCtrlFlags) override; }; } diff --git a/src/interactivity/win32/ConsoleControl.cpp b/src/interactivity/win32/ConsoleControl.cpp index 4cea4e3857e..78e66110df2 100644 --- a/src/interactivity/win32/ConsoleControl.cpp +++ b/src/interactivity/win32/ConsoleControl.cpp @@ -37,12 +37,12 @@ using namespace Microsoft::Console::Interactivity::Win32; sizeof(Flags)); } -[[nodiscard]] NTSTATUS ConsoleControl::EndTask(_In_ HANDLE hProcessId, _In_ DWORD dwEventType, _In_ ULONG ulCtrlFlags) +[[nodiscard]] NTSTATUS ConsoleControl::EndTask(_In_ DWORD dwProcessId, _In_ DWORD dwEventType, _In_ ULONG ulCtrlFlags) { auto pConsoleWindow = ServiceLocator::LocateConsoleWindow(); CONSOLEENDTASK ConsoleEndTaskParams; - ConsoleEndTaskParams.ProcessId = hProcessId; + ConsoleEndTaskParams.ProcessId = ULongToHandle(dwProcessId); // This is actually a PID, even though the struct expects a HANDLE. ConsoleEndTaskParams.ConsoleEventCode = dwEventType; ConsoleEndTaskParams.ConsoleFlags = ulCtrlFlags; ConsoleEndTaskParams.hwnd = pConsoleWindow == nullptr ? nullptr : pConsoleWindow->GetWindowHandle(); diff --git a/src/interactivity/win32/ConsoleControl.hpp b/src/interactivity/win32/ConsoleControl.hpp index 9a8ab927fc0..fdd490a528a 100644 --- a/src/interactivity/win32/ConsoleControl.hpp +++ b/src/interactivity/win32/ConsoleControl.hpp @@ -47,7 +47,7 @@ namespace Microsoft::Console::Interactivity::Win32 // IConsoleControl Members [[nodiscard]] NTSTATUS NotifyConsoleApplication(_In_ DWORD dwProcessId); [[nodiscard]] NTSTATUS SetForeground(_In_ HANDLE hProcess, _In_ BOOL fForeground); - [[nodiscard]] NTSTATUS EndTask(_In_ HANDLE hProcessId, _In_ DWORD dwEventType, _In_ ULONG ulCtrlFlags); + [[nodiscard]] NTSTATUS EndTask(_In_ DWORD dwProcessId, _In_ DWORD dwEventType, _In_ ULONG ulCtrlFlags); // Public Members [[nodiscard]] NTSTATUS Control(_In_ ConsoleControl::ControlType ConsoleCommand, diff --git a/src/server/ProcessList.cpp b/src/server/ProcessList.cpp index f852db7728e..ab9c529a76d 100644 --- a/src/server/ProcessList.cpp +++ b/src/server/ProcessList.cpp @@ -305,7 +305,7 @@ void ConsoleProcessList::ModifyConsoleProcessFocus(const bool fForeground) { const auto pProcessHandle = *it; - if (pProcessHandle->_hProcess != nullptr) + if (pProcessHandle->_hProcess) { _ModifyProcessForegroundRights(pProcessHandle->_hProcess.get(), fForeground); }