Skip to content

Commit

Permalink
ConPTY: Avoid WINDOW_BUFFER_SIZE_EVENT when the viewport moves (#15935)
Browse files Browse the repository at this point in the history
`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 ✅
  • Loading branch information
lhecker authored Sep 18, 2023
1 parent 2fa8e76 commit 741633e
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 3 deletions.
1 change: 1 addition & 0 deletions .github/actions/spelling/expect/expect.txt
Original file line number Diff line number Diff line change
Expand Up @@ -1908,6 +1908,7 @@ touchpad
Tpp
Tpqrst
tprivapi
tput
tracelog
tracelogging
traceloggingprovider
Expand Down
8 changes: 5 additions & 3 deletions src/host/outputStream.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down

0 comments on commit 741633e

Please sign in to comment.