From d38bb906ecd5dc232796a77b512f2b73cf9bfbe4 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Thu, 21 Sep 2023 23:55:06 +0200 Subject: [PATCH] Replace WinRT clipboard API with Win32 for pasting (#15360) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Win32 API is significantly faster than the WinRT one, in the order of around 300-1000x depending on the CPU and CPU load. This might slightly improve the situation around #15315, but I suspect that it requires many more fixes. For instance, we don't really have a single text input "queue" into which we write. Multiple routines that `resume_background` just to `WriteFile` into the input pipe are thus racing against each other, contributing to the laggy feeling. I also fear that the modern Windows text stack might be inherently RPC based too, producing worse lag with rising CPU load. This might fix #14323 ## Validation Steps Performed * Paste text from Edge ✅ * Paste text from Notepad ✅ * Right click the address bar in Explorer, choose "Copy address", paste text into WT ✅ --------- Co-authored-by: Dustin L. Howett --- .github/actions/spelling/allow/allow.txt | 1 + .github/actions/spelling/allow/apis.txt | 1 + .github/actions/spelling/allow/microsoft.txt | 3 +- src/cascadia/TerminalApp/TerminalPage.cpp | 110 ++++++++++++------ .../TerminalControl/ControlInteractivity.cpp | 20 +--- .../TerminalControl/ControlInteractivity.h | 1 - src/cascadia/TerminalControl/EventArgs.h | 4 +- 7 files changed, 84 insertions(+), 56 deletions(-) diff --git a/.github/actions/spelling/allow/allow.txt b/.github/actions/spelling/allow/allow.txt index 20999b3a54e..28c46840207 100644 --- a/.github/actions/spelling/allow/allow.txt +++ b/.github/actions/spelling/allow/allow.txt @@ -13,6 +13,7 @@ clickable clig CMMI copyable +Counterintuitively CtrlDToClose cybersecurity dalet diff --git a/.github/actions/spelling/allow/apis.txt b/.github/actions/spelling/allow/apis.txt index e58df6e202b..b25346b9ce1 100644 --- a/.github/actions/spelling/allow/apis.txt +++ b/.github/actions/spelling/allow/apis.txt @@ -226,6 +226,7 @@ Vcpp Viewbox virtualalloc vsnwprintf +wcsnlen wcsstr wcstoui WDJ diff --git a/.github/actions/spelling/allow/microsoft.txt b/.github/actions/spelling/allow/microsoft.txt index b79f49c3781..80335179b32 100644 --- a/.github/actions/spelling/allow/microsoft.txt +++ b/.github/actions/spelling/allow/microsoft.txt @@ -59,12 +59,13 @@ PRIINFO propkey pscustomobject QWORD +rdpclip regedit resfiles robocopy SACLs -segoe sdkddkver +segoe Shobjidl sid Skype diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index 45b7f34ab66..64dd32c41f4 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -2640,62 +2640,91 @@ namespace winrt::TerminalApp::implementation // - Does some of this in a background thread, as to not hang/crash the UI thread. // Arguments: // - eventArgs: the PasteFromClipboard event sent from the TermControl - fire_and_forget TerminalPage::_PasteFromClipboardHandler(const IInspectable /*sender*/, - const PasteFromClipboardEventArgs eventArgs) + fire_and_forget TerminalPage::_PasteFromClipboardHandler(const IInspectable /*sender*/, const PasteFromClipboardEventArgs eventArgs) + try { - const auto data = Clipboard::GetContent(); + // The old Win32 clipboard API as used below is somewhere in the order of 300-1000x faster than + // the WinRT one on average, depending on CPU load. Don't use the WinRT clipboard API if you can. + const auto weakThis = get_weak(); + const auto dispatcher = Dispatcher(); + const auto globalSettings = _settings.GlobalSettings(); + winrt::hstring text; - // This will switch the execution of the function to a background (not - // UI) thread. This is IMPORTANT, because the getting the clipboard data - // will crash on the UI thread, because the main thread is a STA. + // GetClipboardData might block for up to 30s for delay-rendered contents. co_await winrt::resume_background(); - try { - hstring text = L""; - if (data.Contains(StandardDataFormats::Text())) - { - text = co_await data.GetTextAsync(); - } - // Windows Explorer's "Copy address" menu item stores a StorageItem in the clipboard, and no text. - else if (data.Contains(StandardDataFormats::StorageItems())) + // According to various reports on the internet, OpenClipboard might + // fail to acquire the internal lock, for instance due to rdpclip.exe. + for (int attempts = 1;;) { - auto items = co_await data.GetStorageItemsAsync(); - if (items.Size() > 0) + if (OpenClipboard(nullptr)) { - auto item = items.GetAt(0); - text = item.Path(); + break; } - } - if (_settings.GlobalSettings().TrimPaste()) - { - text = { Utils::TrimPaste(text) }; - if (text.empty()) + if (attempts > 5) { - // Text is all white space, nothing to paste co_return; } + + attempts++; + Sleep(10 * attempts); } - // If the requesting terminal is in bracketed paste mode, then we don't need to warn about a multi-line paste. - auto warnMultiLine = _settings.GlobalSettings().WarnAboutMultiLinePaste() && - !eventArgs.BracketedPasteEnabled(); - if (warnMultiLine) + const auto clipboardCleanup = wil::scope_exit([]() { + CloseClipboard(); + }); + + const auto data = GetClipboardData(CF_UNICODETEXT); + if (!data) + { + co_return; + } + + const auto str = static_cast(GlobalLock(data)); + if (!str) { - const auto isNewLineLambda = [](auto c) { return c == L'\n' || c == L'\r'; }; - const auto hasNewLine = std::find_if(text.cbegin(), text.cend(), isNewLineLambda) != text.cend(); - warnMultiLine = hasNewLine; + co_return; } - constexpr const std::size_t minimumSizeForWarning = 1024 * 5; // 5 KiB - const auto warnLargeText = text.size() > minimumSizeForWarning && - _settings.GlobalSettings().WarnAboutLargePaste(); + const auto dataCleanup = wil::scope_exit([&]() { + GlobalUnlock(data); + }); + + const auto maxLength = GlobalSize(data) / sizeof(wchar_t); + const auto length = wcsnlen(str, maxLength); + text = winrt::hstring{ str, gsl::narrow_cast(length) }; + } - if (warnMultiLine || warnLargeText) + if (globalSettings.TrimPaste()) + { + text = { Utils::TrimPaste(text) }; + if (text.empty()) { - co_await wil::resume_foreground(Dispatcher()); + // Text is all white space, nothing to paste + co_return; + } + } + // If the requesting terminal is in bracketed paste mode, then we don't need to warn about a multi-line paste. + auto warnMultiLine = globalSettings.WarnAboutMultiLinePaste() && !eventArgs.BracketedPasteEnabled(); + if (warnMultiLine) + { + const auto isNewLineLambda = [](auto c) { return c == L'\n' || c == L'\r'; }; + const auto hasNewLine = std::find_if(text.cbegin(), text.cend(), isNewLineLambda) != text.cend(); + warnMultiLine = hasNewLine; + } + + constexpr const std::size_t minimumSizeForWarning = 1024 * 5; // 5 KiB + const auto warnLargeText = text.size() > minimumSizeForWarning && globalSettings.WarnAboutLargePaste(); + + if (warnMultiLine || warnLargeText) + { + co_await wil::resume_foreground(dispatcher); + + if (const auto strongThis = weakThis.get()) + { // We have to initialize the dialog here to be able to change the text of the text block within it FindName(L"MultiLinePasteDialog").try_as(); ClipboardText().Text(text); @@ -2723,10 +2752,15 @@ namespace winrt::TerminalApp::implementation } } - eventArgs.HandleClipboardData(text); + co_await winrt::resume_background(); } - CATCH_LOG(); + + // This will end up calling ConptyConnection::WriteInput which calls WriteFile which may block for + // an indefinite amount of time. Avoid freezes and deadlocks by running this on a background thread. + assert(!dispatcher.HasThreadAccess()); + eventArgs.HandleClipboardData(std::move(text)); } + CATCH_LOG(); void TerminalPage::_OpenHyperlinkHandler(const IInspectable /*sender*/, const Microsoft::Terminal::Control::OpenHyperlinkEventArgs eventArgs) { diff --git a/src/cascadia/TerminalControl/ControlInteractivity.cpp b/src/cascadia/TerminalControl/ControlInteractivity.cpp index d0b74edfaa2..a97a700162a 100644 --- a/src/cascadia/TerminalControl/ControlInteractivity.cpp +++ b/src/cascadia/TerminalControl/ControlInteractivity.cpp @@ -224,22 +224,14 @@ namespace winrt::Microsoft::Terminal::Control::implementation // - Initiate a paste operation. void ControlInteractivity::RequestPasteTextFromClipboard() { - // attach ControlInteractivity::_sendPastedTextToConnection() as the - // clipboardDataHandler. This is called when the clipboard data is - // loaded. - auto clipboardDataHandler = std::bind(&ControlInteractivity::_sendPastedTextToConnection, this, std::placeholders::_1); - auto pasteArgs = winrt::make_self(clipboardDataHandler, _core->BracketedPasteEnabled()); + auto args = winrt::make( + [core = _core](const winrt::hstring& wstr) { + core->PasteText(wstr); + }, + _core->BracketedPasteEnabled()); // send paste event up to TermApp - _PasteFromClipboardHandlers(*this, *pasteArgs); - } - - // Method Description: - // - Pre-process text pasted (presumably from the clipboard) - // before sending it over the terminal's connection. - void ControlInteractivity::_sendPastedTextToConnection(std::wstring_view wstr) - { - _core->PasteText(winrt::hstring{ wstr }); + _PasteFromClipboardHandlers(*this, std::move(args)); } void ControlInteractivity::PointerPressed(Control::MouseButtonState buttonState, diff --git a/src/cascadia/TerminalControl/ControlInteractivity.h b/src/cascadia/TerminalControl/ControlInteractivity.h index dee2fe9beed..cafd8972c63 100644 --- a/src/cascadia/TerminalControl/ControlInteractivity.h +++ b/src/cascadia/TerminalControl/ControlInteractivity.h @@ -155,7 +155,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation bool _canSendVTMouseInput(const ::Microsoft::Terminal::Core::ControlKeyStates modifiers); bool _shouldSendAlternateScroll(const ::Microsoft::Terminal::Core::ControlKeyStates modifiers, const int32_t delta); - void _sendPastedTextToConnection(std::wstring_view wstr); til::point _getTerminalPosition(const til::point pixelPosition); bool _sendMouseEventHelper(const til::point terminalPosition, diff --git a/src/cascadia/TerminalControl/EventArgs.h b/src/cascadia/TerminalControl/EventArgs.h index 12632871fdb..1c135cdf4f6 100644 --- a/src/cascadia/TerminalControl/EventArgs.h +++ b/src/cascadia/TerminalControl/EventArgs.h @@ -86,7 +86,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation struct PasteFromClipboardEventArgs : public PasteFromClipboardEventArgsT { public: - PasteFromClipboardEventArgs(std::function clipboardDataHandler, bool bracketedPasteEnabled) : + PasteFromClipboardEventArgs(std::function clipboardDataHandler, bool bracketedPasteEnabled) : m_clipboardDataHandler(clipboardDataHandler), _BracketedPasteEnabled{ bracketedPasteEnabled } {} @@ -98,7 +98,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation WINRT_PROPERTY(bool, BracketedPasteEnabled, false); private: - std::function m_clipboardDataHandler; + std::function m_clipboardDataHandler; }; struct OpenHyperlinkEventArgs : public OpenHyperlinkEventArgsT