Repository: microsoft/terminal (language: cpp)

Issue:
ED2 inserts loads of blank lines in scrollback
### Windows Terminal version

1.19.3172.0

### Windows build number

10.0.19045.3930

### Other Software

_No response_

### Steps to reproduce

1. Open a WSL bash shell in Windows Terminal.
2. Execute the following set of commands:
   
   ```bash
   cd /bin
   ls -la
   printf "\e[3J"
   ls -la
   printf "\e[3J"
   printf "\e[2J"
   ```
4. Scroll up to view the scrollback buffer.


### Expected Behavior

You should see the last lines of the `/bin` directory listing and the two `printf` commands.

### Actual Behavior

The buffer immediately above the visible viewport is blank, and you have to scroll back for pages to find the previous content.

Reproducing this might be a bit tricky, because if I replace the `ls -la` with something like `seq 1000`, the bug isn't triggered. There must be something about the directory listing that's a factor, and hopefully it's not specific to my directory content. And in case it makes any difference, my screen size is 80x24.

It's also worth noting that I can reproduce this in a recent version of OpenConsole, but not in my inbox ConHost (Windows 10), so it looks like it might be a regression.


Discussion / hints:
It seems that this issue occurs as soon as a line wrap occurs (in my case when the window is <98 columns wide).
FYI, it looks like this broke in commit 74748394c17c168843b511dd837268445e5dfd6c (PR #15701), although I'm not sure why.

Edit: After that commit, the `GetLastNonSpaceCharacter` call in `AdaptDispatch::_EraseAll` started returning the wrong coordinates, which explains the results we're seeing. However, I didn't see any significant changes to the function in that commit, so I'm assuming the issue is deeper than that.
I found that the culprit is the change to `ROW::MeasureRight` which now uses the `_wrapForced` member to calculate the row width. That's problematic, because `AdeptDispatch::_EraseScrollback` uses `_FillRect` which does not reset the `_wrapForced` member. That row is then considered non-empty which causes the large number of empty lines.

I'm not sure if I'm seeing this correctly but I believe that's a preexisting bug in `_EraseScrollback` that simply got uncovered now, right?

I'm working on a change now that replaces the `_FillRect` call with some code that scrolls the viewport to the absolute start of the `TextBuffer` so that we can `MEM_DECOMMIT` the remaining backing buffer efficiently.
> That's problematic, because `AdeptDispatch::_EraseScrollback` uses `_FillRect` which does not reset the `_wrapForced` member.

I think that issue may have been introduced by PR #15541. The original `_FillRect` implementation used a `TextBuffer::WriteLine` call with the `wrap` parameter set to false, so that would previously have reset `_wrapForced`.

> I'm working on a change now that replaces the `_FillRect` call with some code that scrolls the viewport to the absolute start of the `TextBuffer` so that we can `MEM_DECOMMIT` the remaining backing buffer efficiently.

That sounds like a good idea, but I think that still leaves a bug in the `_EraseAll` implementation, which possibly has existed from the start. That call to `GetLastNonSpaceCharacter` should really be limited to the current VT page. Otherwise if there happened to be content in the "scroll forward" area (which I think could happen if you're mixing legacy console API calls with VT output), then an `ED2` sequence could still end up panning too far down.
Should `_FillRect` set the wrap flag again? Considering delayed EOL wraps it shouldn't do that I think?

I'm not entirely sure I understand why `_EraseAll` calls `GetLastNonSpaceCharacter` so I may be unqualified to fix it. I would've assumed it would simply move the current `_api.GetViewport()` up by one `viewport.height`...
Ah, I think I can see now why it does it that way. Gnome Terminal scrolls just enough to move the current content into the scrollback (i.e. 3 lines of 3 lines are visible). What I find somewhat curious is that a second `\e[2J` will however scroll the buffer by an entire viewport height. Is that a bug in Gnome Terminal?

I believe the proper way to fix `_EraseAll` is to stop treating `_virtualBottom` as the literal bottom of the viewport, but rather as the last line that was ever written. The bottom of the viewport would then be `std::max(_virtualBottom, viewport.height)`. This would also simplify some stuff in `TextBuffer` so it may be worthwhile to store the value there instead of in `SCREEN_INFORMATION`. I think this is better than `GetLastNonSpaceCharacter` because I get the feeling that the reliance on "whitespace = empty" throughout various parts of the code base makes our behavior appear unpredictable/inconsistent at times.

For now I'll submit just the `_EraseScrollback` as this will fix the `_wrapForced` bug and make the above work. But I'm curious what you think about my previous comment regarding `_FillRect`: Should it affect the `_wrapForced` flag again? Or are there other parts of `AdaptDispatch` that may be broken?

Fix patch:
diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp
index 528d8fbf4d3..9d8193eeb6a 100644
--- a/src/buffer/out/textBuffer.cpp
+++ b/src/buffer/out/textBuffer.cpp
@@ -126,6 +126,8 @@ void TextBuffer::_reserve(til::size screenBufferSize, const TextAttribute& defau
 // The compiler doesn't understand the likelihood of our branches. (PGO does, but that's imperfect.)
 __declspec(noinline) void TextBuffer::_commit(const std::byte* row)
 {
+    assert(row >= _commitWatermark);
+
     const auto rowEnd = row + _bufferRowStride;
     const auto remaining = gsl::narrow_cast<uintptr_t>(_bufferEnd - _commitWatermark);
     const auto minimum = gsl::narrow_cast<uintptr_t>(rowEnd - _commitWatermark);
@@ -146,7 +148,7 @@ void TextBuffer::_decommit() noexcept
     _commitWatermark = _buffer.get();
 }
 
-// Constructs ROWs up to (excluding) the ROW pointed to by `until`.
+// Constructs ROWs between [_commitWatermark,until).
 void TextBuffer::_construct(const std::byte* until) noexcept
 {
     for (; _commitWatermark < until; _commitWatermark += _bufferRowStride)
@@ -158,8 +160,7 @@ void TextBuffer::_construct(const std::byte* until) noexcept
     }
 }
 
-// Destroys all previously constructed ROWs.
-// Be careful! This doesn't reset any of the members, in particular the _commitWatermark.
+// Destructs ROWs between [_buffer,_commitWatermark).
 void TextBuffer::_destroy() const noexcept
 {
     for (auto it = _buffer.get(); it < _commitWatermark; it += _bufferRowStride)
@@ -168,9 +169,8 @@ void TextBuffer::_destroy() const noexcept
     }
 }
 
-// This function is "direct" because it trusts the caller to properly wrap the "offset"
-// parameter modulo the _height of the buffer, etc. But keep in mind that a offset=0
-// is the GetScratchpadRow() and not the GetRowByOffset(0). That one is offset=1.
+// This function is "direct" because it trusts the caller to properly
+// wrap the "offset" parameter modulo the _height of the buffer.
 ROW& TextBuffer::_getRowByOffsetDirect(size_t offset)
 {
     const auto row = _buffer.get() + _bufferRowStride * offset;
@@ -184,6 +184,7 @@ ROW& TextBuffer::_getRowByOffsetDirect(size_t offset)
     return *reinterpret_cast<ROW*>(row);
 }
 
+// See GetRowByOffset().
 ROW& TextBuffer::_getRow(til::CoordType y) const
 {
     // Rows are stored circularly, so the index you ask for is offset by the start position and mod the total of rows.
@@ -197,6 +198,7 @@ ROW& TextBuffer::_getRow(til::CoordType y) const
     }
 
     // We add 1 to the row offset, because row "0" is the one returned by GetScratchpadRow().
+    // See GetScratchpadRow() for more explanation.
 #pragma warning(suppress : 26492) // Don't use const_cast to cast away const or volatile (type.3).
     return const_cast<TextBuffer*>(this)->_getRowByOffsetDirect(gsl::narrow_cast<size_t>(offset) + 1);
 }
@@ -238,6 +240,9 @@ ROW& TextBuffer::GetScratchpadRow()
 // Returns a row filled with whitespace and the given attributes, for you to freely use.
 ROW& TextBuffer::GetScratchpadRow(const TextAttribute& attributes)
 {
+    // The scratchpad row is mapped to the underlying index 0, whereas all regular rows are mapped to
+    // index 1 and up. We do it this way instead of the other way around (scratchpad row at index _height),
+    // because that would force us to MEM_COMMIT the entire buffer whenever this function is called.
     auto& r = _getRowByOffsetDirect(0);
     r.Reset(attributes);
     return r;
@@ -902,15 +907,14 @@ til::point TextBuffer::GetLastNonSpaceCharacter(const Viewport* viewOptional) co
 
     // If the X coordinate turns out to be -1, the row was empty, we need to search backwards for the real end of text.
     const auto viewportTop = viewport.Top();
-    auto fDoBackUp = (coordEndOfText.x < 0 && coordEndOfText.y > viewportTop); // this row is empty, and we're not at the top
-    while (fDoBackUp)
+
+    // while (this row is empty, and we're not at the top)
+    while (coordEndOfText.x < 0 && coordEndOfText.y > viewportTop)
     {
         coordEndOfText.y--;
         const auto& backupRow = GetRowByOffset(coordEndOfText.y);
         // We need to back up to the previous row if this line is empty, AND there are more rows
-
         coordEndOfText.x = backupRow.MeasureRight() - 1;
-        fDoBackUp = (coordEndOfText.x < 0 && coordEndOfText.y > viewportTop);
     }
 
     // don't allow negative results
@@ -1146,6 +1150,39 @@ void TextBuffer::Reset() noexcept
     _initialAttributes = _currentAttributes;
 }
 
+void TextBuffer::ClearScrollback(const til::CoordType start, const til::CoordType height)
+{
+    if (start <= 0)
+    {
+        return;
+    }
+
+    if (height <= 0)
+    {
+        _decommit();
+        return;
+    }
+
+    // Our goal is to move the viewport to the absolute start of the underlying memory buffer so that we can
+    // MEM_DECOMMIT the remaining memory. _firstRow is used to make the TextBuffer behave like a circular buffer.
+    // The start parameter is relative to the _firstRow. The trick to get the content to the absolute start
+    // is to simply add _firstRow ourselves and then reset it to 0. This causes ScrollRows() to write into
+    // the absolute start while reading from relative coordinates. This works because GetRowByOffset()
+    // operates modulo the buffer height and so the possibly-too-large startAbsolute won't be an issue.
+    const auto startAbsolute = _firstRow + start;
+    _firstRow = 0;
+    ScrollRows(startAbsolute, height, -startAbsolute);
+
+    const auto end = _estimateOffsetOfLastCommittedRow();
+    for (auto y = height; y <= end; ++y)
+    {
+        GetMutableRowByOffset(y).Reset(_initialAttributes);
+    }
+
+    ScrollMarks(-start);
+    ClearMarksInRange(til::point{ 0, height }, til::point{ _width, _height });
+}
+
 // Routine Description:
 // - This is the legacy screen resize with minimal changes
 // Arguments:
diff --git a/src/buffer/out/textBuffer.hpp b/src/buffer/out/textBuffer.hpp
index 8ba16f97e75..39cc9fe9a59 100644
--- a/src/buffer/out/textBuffer.hpp
+++ b/src/buffer/out/textBuffer.hpp
@@ -194,6 +194,7 @@ class TextBuffer final
     til::point BufferToScreenPosition(const til::point position) const;
 
     void Reset() noexcept;
+    void ClearScrollback(const til::CoordType start, const til::CoordType height);
 
     void ResizeTraditional(const til::size newSize);
 
diff --git a/src/host/ut_host/ScreenBufferTests.cpp b/src/host/ut_host/ScreenBufferTests.cpp
index d6687e19bd6..98a6f09b2e5 100644
--- a/src/host/ut_host/ScreenBufferTests.cpp
+++ b/src/host/ut_host/ScreenBufferTests.cpp
@@ -4515,6 +4515,7 @@ void ScreenBufferTests::EraseScrollbackTests()
     auto& si = gci.GetActiveOutputBuffer().GetActiveBuffer();
     auto& stateMachine = si.GetStateMachine();
     const auto& cursor = si.GetTextBuffer().GetCursor();
+    const auto initialAttributes = si.GetAttributes();
     WI_SetFlag(si.OutputMode, ENABLE_VIRTUAL_TERMINAL_PROCESSING);
 
     const auto bufferWidth = si.GetBufferSize().Width();
@@ -4571,7 +4572,7 @@ void ScreenBufferTests::EraseScrollbackTests()
     }
 
     Log::Comment(L"The rest of the buffer should be cleared with default attributes.");
-    VERIFY_IS_TRUE(_ValidateLinesContain(viewportLine, bufferHeight, L' ', TextAttribute{}));
+    VERIFY_IS_TRUE(_ValidateLinesContain(viewportLine, bufferHeight, L' ', initialAttributes));
 }
 
 void ScreenBufferTests::EraseTests()
diff --git a/src/terminal/adapter/adaptDispatch.cpp b/src/terminal/adapter/adaptDispatch.cpp
index b6e07b77ffb..fb504c20200 100644
--- a/src/terminal/adapter/adaptDispatch.cpp
+++ b/src/terminal/adapter/adaptDispatch.cpp
@@ -3200,18 +3200,7 @@ bool AdaptDispatch::_EraseScrollback()
     auto& cursor = textBuffer.GetCursor();
     const auto row = cursor.GetPosition().y;
 
-    // Clear all the marks below the new viewport position.
-    textBuffer.ClearMarksInRange(til::point{ 0, height },
-                                 til::point{ bufferSize.width, bufferSize.height });
-    // Then scroll all the remaining marks up. This will trim ones that are now "outside" the buffer
-    textBuffer.ScrollMarks(-top);
-
-    // Scroll the viewport content to the top of the buffer.
-    textBuffer.ScrollRows(top, height, -top);
-    // Clear everything after the viewport.
-    _FillRect(textBuffer, { 0, height, bufferSize.width, bufferSize.height }, whitespace, {});
-    // Also reset the line rendition for all of the cleared rows.
-    textBuffer.ResetLineRenditionRange(height, bufferSize.height);
+    textBuffer.ClearScrollback(top, height);
     // Move the viewport
     _api.SetViewportPosition({ viewport.left, 0 });
     // Move the cursor to the same relative location.


Test patch: