Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using CSI # S to scroll screen buffer is incorrect and can lead to data loss #724

Closed
DHowett-MSFT opened this issue Jul 9, 2018 · 8 comments · Fixed by #790
Closed

Using CSI # S to scroll screen buffer is incorrect and can lead to data loss #724

DHowett-MSFT opened this issue Jul 9, 2018 · 8 comments · Fixed by #790

Comments

@DHowett-MSFT
Copy link

DHowett-MSFT commented Jul 9, 2018

Environment data

PS version: 5.1.17712.1000
PSReadline version: 2.0.0-beta2
os: 10.0.17712.1000 (WinBuild.160101.0800)
PS file version: 10.0.17712.1000 (WinBuild.160101.0800)

Steps to reproduce or exception report

Make sure that the prompt is at the bottom of the screen, then press enter any number of times. One should do.
PSReadline will emit ^[[1S (from ConsoleLib.cs, perhaps via ReadLine.cs).

Scrolling the viewport in this manner can:

  • destroy the user's scrollback buffer
  • force an expensive full-buffer repaint over a VT link (like ssh or pty)

cc @zadjii-msft

@lzybkr
Copy link
Member

lzybkr commented Jul 11, 2018

In your scenario, I think the call is here.

v1.2 was calling ScrollConsoleScreenBuffer, and the non-VT code for this method also calls ScrollConsoleScreenBuffer. The documentation states that this escape is equivalent.

I don't think any of the scrollback buffer is being destroyed (barring bugs) other than the part that needs to be scrolled off the screen because the buffer is full.

As for performance - is it worse than calling ScrollConsoleScreenBuffer? In this scenario, it's scrolling a single line, basically what would happen if you wrote a single newline.

@zadjii-msft
Copy link

Wait, why were you ever calling that API? In this case, aren't you effectively just newlining down from the previous output? Why would you need to call ScrollConsoleScreenBuffer to handle that? In the console since time immemorial, if you emit a newline at the bottom of the buffer, conhost will automatically discard the top lines and create new ones at the bottom. It certainly does that a lot more efficiently than ScrollConsoleScreenBuffer does. ScrollConsoleScreenBuffer needs to copy the contents of each row into another row, while the "newline at the bottom" case just reallocates a single row.

Plus, this might be getting hit more frequently (esp on *nix) where there's no "screen buffer", there's only the viewport. So the cursor is going to be hitting the bottom of the "buffer" after only ~30 lines (the height of your window) instead of after 9000 lines.

As far as "destroy the user's scrollback buffer", what we mean can be easily repro'd with the following VT sequence. You can test this out from bash with the following commandline:

printf "\033[2J\033[HThis line of text will disappear\n1\n2\n3\n4\n5\n6\n7\n8\015\033[1S"

This string:

  1. Clears the buffer (forcing existing contents into scrollback, for terminals that support one)
  2. moves the cursor to 0,0
  3. prints a line of text. This line of text is going to disappear from the terminal.
  4. Prints 8 more lines of text, each with a number
  5. Uses Scroll Up to move the viewport contents up by one row.

You can see the output here:

image

If you scroll up into the terminal's history, you'll see that the line "This line of text will disappear" does not appear in the scrollback.

image

While this example is trivial, in the real world case, this means that if you hit enter a couple of times at the prompt line after outputting a bunch of text, the history gets deleted instead of moving into the scrollback.

@lzybkr
Copy link
Member

lzybkr commented Jul 11, 2018

My memory is definitely hazy here, but I think I needed ScrollConsoleScreenBuffer because writing a newline didn't work correctly, or worked differently depending on the version of Windows.

IIRC, when the cursor was at the end of the line, writing a newline had no effect. I think the behavior had also changed between Windows versions, but I don't recall specifics.

@MVKozlov
Copy link
Contributor

@lzybkr It's here #607

@lzybkr
Copy link
Member

lzybkr commented Jul 12, 2018

@MVKozlov - are you sure? I think that issue is unrelated.

@MVKozlov
Copy link
Contributor

I doesn't look in ScrollConsoleScreenBuffer code exactly, but I remember that cursor at the end of a line - it s the problem with issue I mentioned

@lzybkr
Copy link
Member

lzybkr commented Jul 12, 2018

Ah, I see, thanks for the clarification, and sorry I haven't gotten around to fixing your issue.

@DHowett-MSFT
Copy link
Author

Ah, this also causes some weird viewport things during WSL Interop. Since it's "scrolling the viewport" instead of emitting line feeds, this happens:

psreadline

lzybkr added a commit that referenced this issue Nov 1, 2018
This hopefully fixes some weird issues in WSL and some Linux terminals.

This also works around a Windows 1809 Console bug for the Ctrl+l binding
to clear the screen by using the same escape sequence that bash uses.

Fix #724
lzybkr added a commit that referenced this issue Nov 5, 2018
This fixes some weird issues in WSL and some Linux terminals.

This also works around a Windows 1809 Console bug for the Ctrl+l binding
to clear the screen by using the same escape sequence that bash uses.

Fix #724
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants