From 76793b1e3fd303d681016d019407c509e91575aa Mon Sep 17 00:00:00 2001 From: Leon Liang Date: Thu, 5 Aug 2021 10:05:21 -0700 Subject: [PATCH] [DefApp] Move from Monarch multi instance servers to Peasant single instance servers (#10823) - Monarch no longer sets itself up as a `CTerminalHandoff` multi instance server by default - In fact, `CTerminalHandoff` will only ever be a single instance server - When COM needs a `CTerminalHandoff`, it launches `wt.exe -embedding`, which gets picked up by the Monarch and then gets handed off to itself/peasant depending on user settings. - Peasant now recognizes the `-embedding` commandline and will start a `CTerminalHandoff` single instance listener, and receives the connection into a new tab. Closes #10358 --- .../TerminalApp/AppActionHandlers.cpp | 4 +- src/cascadia/TerminalApp/AppLogic.cpp | 5 ++ src/cascadia/TerminalApp/TabManagement.cpp | 6 +- src/cascadia/TerminalApp/TerminalPage.cpp | 69 ++++++++++--------- src/cascadia/TerminalApp/TerminalPage.h | 4 +- .../TerminalConnection/CTerminalHandoff.cpp | 27 ++++++-- .../TerminalConnection/CTerminalHandoff.h | 6 +- src/cascadia/WindowsTerminal/AppHost.cpp | 3 - 8 files changed, 74 insertions(+), 50 deletions(-) diff --git a/src/cascadia/TerminalApp/AppActionHandlers.cpp b/src/cascadia/TerminalApp/AppActionHandlers.cpp index 60401122c..f88017e1e 100644 --- a/src/cascadia/TerminalApp/AppActionHandlers.cpp +++ b/src/cascadia/TerminalApp/AppActionHandlers.cpp @@ -264,12 +264,12 @@ namespace winrt::TerminalApp::implementation { if (args == nullptr) { - _OpenNewTab(nullptr); + LOG_IF_FAILED(_OpenNewTab(nullptr)); args.Handled(true); } else if (const auto& realArgs = args.ActionArgs().try_as()) { - _OpenNewTab(realArgs.TerminalArgs()); + LOG_IF_FAILED(_OpenNewTab(realArgs.TerminalArgs())); args.Handled(true); } } diff --git a/src/cascadia/TerminalApp/AppLogic.cpp b/src/cascadia/TerminalApp/AppLogic.cpp index 441fbd4de..e9350ef95 100644 --- a/src/cascadia/TerminalApp/AppLogic.cpp +++ b/src/cascadia/TerminalApp/AppLogic.cpp @@ -1229,6 +1229,11 @@ namespace winrt::TerminalApp::implementation auto actions = winrt::single_threaded_vector(std::move(appArgs.GetStartupActions())); _root->ProcessStartupActions(actions, false, cwd); + + if (appArgs.IsHandoffListener()) + { + _root->SetInboundListener(true); + } } // Return the result of parsing with commandline, though it may or may not be used. return result; diff --git a/src/cascadia/TerminalApp/TabManagement.cpp b/src/cascadia/TerminalApp/TabManagement.cpp index b93f50cab..dd9e9dc96 100644 --- a/src/cascadia/TerminalApp/TabManagement.cpp +++ b/src/cascadia/TerminalApp/TabManagement.cpp @@ -56,7 +56,7 @@ namespace winrt::TerminalApp::implementation // - existingConnection: An optional connection that is already established to a PTY // for this tab to host instead of creating one. // If not defined, the tab will create the connection. - void TerminalPage::_OpenNewTab(const NewTerminalArgs& newTerminalArgs, winrt::Microsoft::Terminal::TerminalConnection::ITerminalConnection existingConnection) + HRESULT TerminalPage::_OpenNewTab(const NewTerminalArgs& newTerminalArgs, winrt::Microsoft::Terminal::TerminalConnection::ITerminalConnection existingConnection) try { const auto profileGuid{ _settings.GetProfileForArgs(newTerminalArgs) }; @@ -89,8 +89,10 @@ namespace winrt::TerminalApp::implementation TraceLoggingWideString(schemeName.data(), "SchemeName", "Color scheme set in the settings"), TraceLoggingKeyword(MICROSOFT_KEYWORD_MEASURES), TelemetryPrivacyDataTag(PDT_ProductAndServicePerformance)); + + return S_OK; } - CATCH_LOG(); + CATCH_RETURN(); // Method Description: // - Creates a new tab with the given settings. If the tab bar is not being diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index 96e5f38e0..3bbf27eab 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -19,6 +19,8 @@ #include "RenameWindowRequestedArgs.g.cpp" #include "../inc/WindowingBehavior.h" +#include + using namespace winrt; using namespace winrt::Windows::Foundation::Collections; using namespace winrt::Windows::UI::Xaml; @@ -348,34 +350,12 @@ namespace winrt::TerminalApp::implementation winrt::Microsoft::Terminal::TerminalConnection::ConptyConnection::StartInboundListener(); } // If we failed to start the listener, it will throw. - // We should fail fast here or the Terminal will be in a very strange state. - // We only start the listener if the Terminal was started with the COM server - // `-Embedding` flag and we make no tabs as a result. - // Therefore, if the listener cannot start itself up to make that tab with - // the inbound connection that caused the COM activation in the first place... - // we would be left with an empty terminal frame with no tabs. - // Instead, crash out so COM sees the server die and things unwind - // without a weird empty frame window. + // We don't want to fail fast here because if a peasant has some trouble with + // starting the listener, we don't want it to crash and take all its tabs down + // with it. catch (...) { - // However, we cannot always fail fast because of MSFT:33501832. Sometimes the COM catalog - // tears the state between old and new versions and fails here for that reason. - // As we're always becoming an inbound server in the monarch, even when COM didn't strictly - // ask us yet...we might just crash always. - // Instead... we're going to differentiate. If COM started us... we will fail fast - // so it sees the process die and falls back. - // If we were just starting normally as a Monarch and opportunistically listening for - // inbound connections... then we'll just log the failure and move on assuming - // the version state is torn and will fix itself whenever the packaging upgrade - // tasks decide to clean up. - if (_isEmbeddingInboundListener) - { - FAIL_FAST_CAUGHT_EXCEPTION(); - } - else - { - LOG_CAUGHT_EXCEPTION(); - } + LOG_CAUGHT_EXCEPTION(); } } } @@ -759,7 +739,7 @@ namespace winrt::TerminalApp::implementation } else { - this->_OpenNewTab(newTerminalArgs); + LOG_IF_FAILED(this->_OpenNewTab(newTerminalArgs)); } } @@ -2405,13 +2385,38 @@ namespace winrt::TerminalApp::implementation return _isAlwaysOnTop; } - void TerminalPage::_OnNewConnection(winrt::Microsoft::Terminal::TerminalConnection::ITerminalConnection connection) + HRESULT TerminalPage::_OnNewConnection(winrt::Microsoft::Terminal::TerminalConnection::ITerminalConnection connection) { - // TODO: GH 9458 will give us more context so we can try to choose a better profile. - _OpenNewTab(nullptr, connection); + // We need to be on the UI thread in order for _OpenNewTab to run successfully. + // HasThreadAccess will return true if we're currently on a UI thread and false otherwise. + // When we're on a COM thread, we'll need to dispatch the calls to the UI thread + // and wait on it hence the locking mechanism. + if (Dispatcher().HasThreadAccess()) + { + // TODO: GH 9458 will give us more context so we can try to choose a better profile. + auto hr = _OpenNewTab(nullptr, connection); - // Request a summon of this window to the foreground - _SummonWindowRequestedHandlers(*this, nullptr); + // Request a summon of this window to the foreground + _SummonWindowRequestedHandlers(*this, nullptr); + + return hr; + } + else + { + til::latch latch{ 1 }; + HRESULT finalVal = S_OK; + + Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [&]() { + finalVal = _OpenNewTab(nullptr, connection); + + _SummonWindowRequestedHandlers(*this, nullptr); + + latch.count_down(); + }); + + latch.wait(); + return finalVal; + } } // Method Description: diff --git a/src/cascadia/TerminalApp/TerminalPage.h b/src/cascadia/TerminalApp/TerminalPage.h index d6476b5ad..bb1a6f13e 100644 --- a/src/cascadia/TerminalApp/TerminalPage.h +++ b/src/cascadia/TerminalApp/TerminalPage.h @@ -188,7 +188,7 @@ namespace winrt::TerminalApp::implementation void _CreateNewTabFlyout(); void _OpenNewTabDropdown(); - void _OpenNewTab(const Microsoft::Terminal::Settings::Model::NewTerminalArgs& newTerminalArgs, winrt::Microsoft::Terminal::TerminalConnection::ITerminalConnection existingConnection = nullptr); + HRESULT _OpenNewTab(const Microsoft::Terminal::Settings::Model::NewTerminalArgs& newTerminalArgs, winrt::Microsoft::Terminal::TerminalConnection::ITerminalConnection existingConnection = nullptr); void _CreateNewTabFromSettings(GUID profileGuid, const Microsoft::Terminal::Settings::Model::TerminalSettingsCreateResult& settings, winrt::Microsoft::Terminal::TerminalConnection::ITerminalConnection existingConnection = nullptr); winrt::Microsoft::Terminal::TerminalConnection::ITerminalConnection _CreateConnectionFromSettings(GUID profileGuid, Microsoft::Terminal::Settings::Model::TerminalSettings settings); @@ -344,7 +344,7 @@ namespace winrt::TerminalApp::implementation winrt::Microsoft::Terminal::Settings::Model::Command _lastPreviewedCommand{ nullptr }; winrt::Microsoft::Terminal::Settings::Model::TerminalSettings _originalSettings{ nullptr }; - void _OnNewConnection(winrt::Microsoft::Terminal::TerminalConnection::ITerminalConnection connection); + HRESULT _OnNewConnection(winrt::Microsoft::Terminal::TerminalConnection::ITerminalConnection connection); void _HandleToggleInboundPty(const IInspectable& sender, const Microsoft::Terminal::Settings::Model::ActionEventArgs& args); void _WindowRenamerActionClick(const IInspectable& sender, const IInspectable& eventArgs); diff --git a/src/cascadia/TerminalConnection/CTerminalHandoff.cpp b/src/cascadia/TerminalConnection/CTerminalHandoff.cpp index 0138921fd..5023a8188 100644 --- a/src/cascadia/TerminalConnection/CTerminalHandoff.cpp +++ b/src/cascadia/TerminalConnection/CTerminalHandoff.cpp @@ -11,6 +11,8 @@ using namespace Microsoft::WRL; static NewHandoffFunction _pfnHandoff = nullptr; // The registration ID of the class object for clean up later static DWORD g_cTerminalHandoffRegistration = 0; +// Mutex so we only do start/stop/establish one at a time. +static std::shared_mutex _mtx; // Routine Description: // - Starts listening for TerminalHandoff requests by registering @@ -19,9 +21,11 @@ static DWORD g_cTerminalHandoffRegistration = 0; // - pfnHandoff - Function to callback when a handoff is received // Return Value: // - S_OK, E_NOT_VALID_STATE (start called when already started) or relevant COM registration error. -HRESULT CTerminalHandoff::s_StartListening(NewHandoffFunction pfnHandoff) noexcept +HRESULT CTerminalHandoff::s_StartListening(NewHandoffFunction pfnHandoff) try { + std::unique_lock lock{ _mtx }; + RETURN_HR_IF(E_NOT_VALID_STATE, _pfnHandoff != nullptr); const auto classFactory = Make>(); @@ -31,7 +35,7 @@ try ComPtr unk; RETURN_IF_FAILED(classFactory.As(&unk)); - RETURN_IF_FAILED(CoRegisterClassObject(__uuidof(CTerminalHandoff), unk.Get(), CLSCTX_LOCAL_SERVER, REGCLS_MULTIPLEUSE, &g_cTerminalHandoffRegistration)); + RETURN_IF_FAILED(CoRegisterClassObject(__uuidof(CTerminalHandoff), unk.Get(), CLSCTX_LOCAL_SERVER, REGCLS_SINGLEUSE, &g_cTerminalHandoffRegistration)); _pfnHandoff = pfnHandoff; @@ -46,8 +50,10 @@ CATCH_RETURN() // - // Return Value: // - S_OK, E_NOT_VALID_STATE (stop called when not started), or relevant COM class revoke error -HRESULT CTerminalHandoff::s_StopListening() noexcept +HRESULT CTerminalHandoff::s_StopListening() { + std::unique_lock lock{ _mtx }; + RETURN_HR_IF_NULL(E_NOT_VALID_STATE, _pfnHandoff); _pfnHandoff = nullptr; @@ -91,10 +97,19 @@ static HRESULT _duplicateHandle(const HANDLE in, HANDLE& out) noexcept // - E_NOT_VALID_STATE if a event handler is not registered before calling. `::DuplicateHandle` // error codes if we cannot manage to make our own copy of handles to retain. Or S_OK/error // from the registered handler event function. -HRESULT CTerminalHandoff::EstablishPtyHandoff(HANDLE in, HANDLE out, HANDLE signal, HANDLE ref, HANDLE server, HANDLE client) noexcept +HRESULT CTerminalHandoff::EstablishPtyHandoff(HANDLE in, HANDLE out, HANDLE signal, HANDLE ref, HANDLE server, HANDLE client) { + // Stash a local copy of _pfnHandoff before we stop listening. + auto localPfnHandoff = _pfnHandoff; + + // Because we are REGCLS_SINGLEUSE... we need to `CoRevokeClassObject` after we handle this ONE call. + // COM does not automatically clean that up for us. We must do it. + s_StopListening(); + + std::unique_lock lock{ _mtx }; + // Report an error if no one registered a handoff function before calling this. - RETURN_HR_IF_NULL(E_NOT_VALID_STATE, _pfnHandoff); + RETURN_HR_IF_NULL(E_NOT_VALID_STATE, localPfnHandoff); // Duplicate the handles from what we received. // The contract with COM specifies that any HANDLEs we receive from the caller belong @@ -108,5 +123,5 @@ HRESULT CTerminalHandoff::EstablishPtyHandoff(HANDLE in, HANDLE out, HANDLE sign RETURN_IF_FAILED(_duplicateHandle(client, client)); // Call registered handler from when we started listening. - return _pfnHandoff(in, out, signal, ref, server, client); + return localPfnHandoff(in, out, signal, ref, server, client); } diff --git a/src/cascadia/TerminalConnection/CTerminalHandoff.h b/src/cascadia/TerminalConnection/CTerminalHandoff.h index 2e173e0a3..2ba644f41 100644 --- a/src/cascadia/TerminalConnection/CTerminalHandoff.h +++ b/src/cascadia/TerminalConnection/CTerminalHandoff.h @@ -37,12 +37,12 @@ struct __declspec(uuid(__CLSID_CTerminalHandoff)) HANDLE signal, HANDLE ref, HANDLE server, - HANDLE client) noexcept override; + HANDLE client) override; #pragma endregion - static HRESULT s_StartListening(NewHandoffFunction pfnHandoff) noexcept; - static HRESULT s_StopListening() noexcept; + static HRESULT s_StartListening(NewHandoffFunction pfnHandoff); + static HRESULT s_StopListening(); }; // Disable warnings from the CoCreatableClass macro as the value it provides for diff --git a/src/cascadia/WindowsTerminal/AppHost.cpp b/src/cascadia/WindowsTerminal/AppHost.cpp index 53e21ccdd..092fe8eae 100644 --- a/src/cascadia/WindowsTerminal/AppHost.cpp +++ b/src/cascadia/WindowsTerminal/AppHost.cpp @@ -656,9 +656,6 @@ void AppHost::_BecomeMonarch(const winrt::Windows::Foundation::IInspectable& /*s const winrt::Windows::Foundation::IInspectable& /*args*/) { _setupGlobalHotkeys(); - - // The monarch is just going to be THE listener for inbound connections. - _listenForInboundConnections(); } void AppHost::_listenForInboundConnections()