From 741633ef7ae964a5765914dc181b8eccbec58d68 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Mon, 18 Sep 2023 17:53:24 +0200 Subject: [PATCH] ConPTY: Avoid WINDOW_BUFFER_SIZE_EVENT when the viewport moves (#15935) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `SetConsoleWindowInfoImpl` calls `PostUpdateWindowSize`, which emits a `CM_SET_WINDOW_SIZE` event, which causes `_InternalSetWindowSize` to be called, which calls `ScreenBufferSizeChange` which then finally emits a `WINDOW_BUFFER_SIZE_EVENT` event into the client input buffer. This messes up applications like which make use of `WINDOW_BUFFER_SIZE_EVENT` to perform potentially lossy operations. In case of SSH this results in a resize (SIGWINCH) of the server-side screen which similarly may result in a response by the shell, etc. Since that happens over networks and is async, and because our conhost VT viewport implementation appears to have a number of subtle bugs, this results in duplicate output lines (sometimes hundreds). Under Windows Terminal this issue is not as apparent, since ConPTY has no viewport that can be moved and no scrollback. It only appears as an issue if a terminal application reacts poorly to the SIGWINCH event. Closes #15769 ## Validation Steps Performed * Set a breakpoint in `SynthesizeWindowBufferSizeEvent` * Launch WSL and cause the viewport to move down No calls to `SynthesizeWindowBufferSizeEvent` ✅ * Execute `tput reset` Input line moves to row 0 ✅ --- .github/actions/spelling/expect/expect.txt | 1 + src/host/outputStream.cpp | 8 +++++--- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/.github/actions/spelling/expect/expect.txt b/.github/actions/spelling/expect/expect.txt index 2826e125a02..af587b42b67 100644 --- a/.github/actions/spelling/expect/expect.txt +++ b/.github/actions/spelling/expect/expect.txt @@ -1908,6 +1908,7 @@ touchpad Tpp Tpqrst tprivapi +tput tracelog tracelogging traceloggingprovider diff --git a/src/host/outputStream.cpp b/src/host/outputStream.cpp index 222868aad7a..a48e189f00d 100644 --- a/src/host/outputStream.cpp +++ b/src/host/outputStream.cpp @@ -95,9 +95,11 @@ til::rect ConhostInternalGetSet::GetViewport() const void ConhostInternalGetSet::SetViewportPosition(const til::point position) { auto& info = _io.GetActiveOutputBuffer(); - const auto dimensions = info.GetViewport().Dimensions(); - const auto windowRect = til::rect{ position, dimensions }.to_inclusive_rect(); - THROW_IF_FAILED(ServiceLocator::LocateGlobals().api->SetConsoleWindowInfoImpl(info, true, windowRect)); + THROW_IF_FAILED(info.SetViewportOrigin(true, position, false)); + // SetViewportOrigin() only updates the virtual bottom (the bottom coordinate of the area + // in the text buffer a VT client writes its output into) when it's moving downwards. + // But this function is meant to truly move the viewport no matter what. Otherwise `tput reset` breaks. + info.UpdateBottom(); } // Method Description: