Repository: microsoft/terminal (language: cpp)

Issue:
DCS sequences split across multiple "packets" can be corrupted by conpty
### Windows Terminal version

1.20.10822.0

### Windows build number

10.0.19045.4291

### Other Software

_No response_

### Steps to reproduce

Run the following python script:

```py
import sys
import time

sys.stdout.write('\033[37;40m\033[2J')
sys.stdout.write('\033P2$p0;2;50;0;0/')
sys.stdout.flush()
time.sleep(0.1)
sys.stdout.write('0;2;0;0;0\033\\')
```


### Expected Behavior

It should clear the screen and briefly make the background color red. It works as expected in OpenConsole.

### Actual Behavior

In Windows Terminal, the background is left permanently red, and you can see the second half of the DCS sequence output on the screen: `0;2;0;0;0`.

Looking at the debug tap, it appears that conpty is inserting an SGR reset sequence in the middle of the DCS stream (where the script sleeps for a bit). This seems similar to issue #16079, but that test case is now working correctly on the main branch, while this is still broken.

I haven't had a chance to git bisect it, so I'm not sure if it's a regression, but I wouldn't be surprised if it was always broken in some way (although possibly not obviously so). There may also be an element of timing involved, but I can reproduce the problem consistently with the script above. 


Discussion / hints:
Hi I'm an AI powered bot that finds similar issues based off the issue title.

Please view the issues below to see if they solve your problem, and if the issue describes your problem please consider closing this one and thumbs upping the other issue to help us prioritize it. Thank you!

### Open similar issues:

- [DCS sequences are no longer passed through in real time (#17111)](https://github.com/microsoft/terminal/issues/17111),  similarity score: 0.86
- [Conpty "pass-through" doesn't maintain order of operations (#8698)](https://github.com/microsoft/terminal/issues/8698),  similarity score: 0.81

### Closed similar issues:
- [Escape sequences behave strangely over conpty when VirtualTerminalLevel is set (#1965)](https://github.com/microsoft/terminal/issues/1965),  similarity score: 0.78
- [ConPTY not emitting OSC sequences longer than 256 characters (Windows 10) (#15551)](https://github.com/microsoft/terminal/issues/15551),  similarity score: 0.75

> Note: You can give me feedback by thumbs upping or thumbs downing this comment.
BTW coincidentally I noticed something weird today and I'm not sure if it's related to the issues you've found (edit: it's quite unlikely though). Try running this in a new pwsh tab (or WSL with the `` ` `` replaced):
```pwsh
"a`e[100m`r`nb`e[m`r`nc"
```

Then continue executing this until the terminal starts scrolling. Once it does, entire rows are given the attribute at a time.
> Once it does, entire rows are given the attribute at a time.

This is expected behavior. When the screen scrolls, the newly revealed line is meant to be filled with the active color attributes by default. You can control that behavior with `DECECM` (Erase Color Mode). For example, in pwsh you would need to do this first: ``"`e[?117h"``. 
I figured out the problem here. When performing a DCS sequence passthrough, the first thing we do is call `Renderer::TriggerFlush` to make sure any pending VT engine paints are output before starting the actual DCS sequence. However, if there is already a pending paint waiting on the console lock in the render thread, that's still going to be executed when the lock is next free, even if there's nothing left for it to do.

So what's happening in this case is the initial part of the DCS gets passed through, then the console lock is released and the VT render engine proceeds with its paint. At this point there's nothing left for it paint, but that doesn't stop it from resetting the attributes in the "Prep Colors" step of `_PaintFrameForEngine`. That `SGR` sequence then ends up in the middle of the ongoing DCS sequence, resulting in an early termination.

As a quick hack fix, I made the `Renderer::TriggerFlush` method toggle the console lock after calling `_PaintFrame`. That gives the render thread a chance to grab the lock and flush its pending paint, but this feels like a very flaky solution. I think it would probably be better if the VT render engine could just avoid sending an `SGR` reset when there is no real need for it. But I don't know the code well enough to know if that's a workable solution.
After further investigation into why the VT render engine is sending that `SGR` reset, I've discovered that that is technically a bug.

In theory, the `VtEngine::StartPaint` method already has a check to see whether there's something to do. See here:

https://github.com/microsoft/terminal/blob/378b6594bd94cf3b27f4e309a11efe25d83de0d9/src/renderer/vt/paint.cpp#L32-L36

If not, that method returns `S_FALSE`, which should indicate to the renderer that we can skip this frame entirely. However, the derived `XtermEngine` class ignores the `S_FALSE` return value, so that information is lost. See here:

https://github.com/microsoft/terminal/blob/378b6594bd94cf3b27f4e309a11efe25d83de0d9/src/renderer/vt/XtermEngine.cpp#L38-L40

The other problem is that the `somethingToDo` calculation always evaluates to true, because the `_cursorMoved` flag is always set to true. This is the result of the PR #15500 changes described in issue #17150 - we're now calling `InvalidateCursor` on every paint, which makes the VT engine think the cursor is always moving.

In theory that could be fixed by checking whether the cursor position in the `InvalidateCursor` call is different from the current position, if it weren't for the fact that the `InvalidateCursor` call isn't actually passed the current position - it's passed the previous position. But that should be resolved by my suggested fix for #17150.

In short, I think we could fix this bug with the following:
1. Issue #17150 would need to be fixed first.
2. We would need to check for a change in position when setting the `_cursorMoved` flag in `VtEngine::InvalidateCursor`.
3. The `XtermEngine::StartPaint` method would need to check for `S_FALSE` being returned from the parent class.

This way we wouldn't need to mess with the console lock, so it seems like a safer approach. There might be edge cases that I'm missing though.
After trying this out, I found there's an extra step required. When the `XtermEngine::StartPaint` method returns `S_FALSE`, it also needs to reset the `_noFlushOnEnd` flag, otherwise the next frame may not flush when it is supposed to.

Fix patch:
diff --git a/src/renderer/vt/XtermEngine.cpp b/src/renderer/vt/XtermEngine.cpp
index 75fc3510fa6..b6d0c58a862 100644
--- a/src/renderer/vt/XtermEngine.cpp
+++ b/src/renderer/vt/XtermEngine.cpp
@@ -37,7 +37,16 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe,
 //      the pipe.
 [[nodiscard]] HRESULT XtermEngine::StartPaint() noexcept
 {
-    RETURN_IF_FAILED(VtEngine::StartPaint());
+    const auto hr = VtEngine::StartPaint();
+    if (hr != S_OK)
+    {
+        // If _noFlushOnEnd was set, and we didn't return early, it would usually
+        // have been reset in the EndPaint call. But since that's not going to
+        // happen now, we need to reset it here, otherwise we may mistakenly skip
+        // the flush on the next frame.
+        _noFlushOnEnd = false;
+        return hr;
+    }
 
     _trace.TraceLastText(_lastText);
 
@@ -83,15 +92,6 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe,
         }
     }
 
-    if (!_quickReturn)
-    {
-        if (_WillWriteSingleChar())
-        {
-            // Don't re-enable the cursor.
-            _quickReturn = true;
-        }
-    }
-
     return S_OK;
 }
 
diff --git a/src/renderer/vt/invalidate.cpp b/src/renderer/vt/invalidate.cpp
index 489a6b50bc1..942ba58df26 100644
--- a/src/renderer/vt/invalidate.cpp
+++ b/src/renderer/vt/invalidate.cpp
@@ -75,7 +75,7 @@ CATCH_RETURN();
     }
     _skipCursor = false;
 
-    _cursorMoved = true;
+    _cursorMoved = psrRegion->origin() != _lastText;
     return S_OK;
 }
 
diff --git a/src/renderer/vt/math.cpp b/src/renderer/vt/math.cpp
index d86e4644ca5..354ca0e9925 100644
--- a/src/renderer/vt/math.cpp
+++ b/src/renderer/vt/math.cpp
@@ -52,38 +52,3 @@ void VtEngine::_OrRect(_Inout_ til::inclusive_rect* const pRectExisting, const t
     pRectExisting->right = std::max(pRectExisting->right, pRectToOr->right);
     pRectExisting->bottom = std::max(pRectExisting->bottom, pRectToOr->bottom);
 }
-
-// Method Description:
-// - Returns true if the invalidated region indicates that we only need to
-//      simply print text from the current cursor position. This will prevent us
-//      from sending extra VT set-up/tear down sequences (?12h/l) when all we
-//      need to do is print more text at the current cursor position.
-// Arguments:
-// - <none>
-// Return Value:
-// - true iff only the next character is invalid
-bool VtEngine::_WillWriteSingleChar() const
-{
-    // If there is no scroll delta, return false.
-    if (til::point{ 0, 0 } != _scrollDelta)
-    {
-        return false;
-    }
-
-    // If there is more than one invalid char, return false.
-    if (!_invalidMap.one())
-    {
-        return false;
-    }
-
-    // Get the single point at which things are invalid.
-    const auto invalidPoint = _invalidMap.runs().front().origin();
-
-    // Either the next character to the right or the immediately previous
-    //      character should follow this code path
-    //      (The immediate previous character would suggest a backspace)
-    auto invalidIsNext = invalidPoint == _lastText;
-    auto invalidIsLast = invalidPoint == til::point{ _lastText.x - 1, _lastText.y };
-
-    return invalidIsNext || invalidIsLast;
-}
diff --git a/src/renderer/vt/paint.cpp b/src/renderer/vt/paint.cpp
index af08cdd5b58..1fcb5335887 100644
--- a/src/renderer/vt/paint.cpp
+++ b/src/renderer/vt/paint.cpp
@@ -20,30 +20,30 @@ using namespace Microsoft::Console::Types;
 //      HRESULT error code if painting didn't start successfully.
 [[nodiscard]] HRESULT VtEngine::StartPaint() noexcept
 {
+    // When unit testing, there may be no pipe, but we still need to paint.
     if (!_hFile)
     {
-        return S_FALSE;
+        return S_OK;
     }
 
     // If we're using line renditions, and this is a full screen paint, we can
     // potentially stop using them at the end of this frame.
     _stopUsingLineRenditions = _usingLineRenditions && _AllIsInvalid();
 
-    // If there's nothing to do, quick return
+    // If there's nothing to do, we won't need to paint.
     auto somethingToDo = _invalidMap.any() ||
                          _scrollDelta != til::point{ 0, 0 } ||
                          _cursorMoved ||
                          _titleChanged;
 
-    _quickReturn = !somethingToDo;
-    _trace.TraceStartPaint(_quickReturn,
+    _trace.TraceStartPaint(!somethingToDo,
                            _invalidMap,
                            _lastViewport.ToExclusive(),
                            _scrollDelta,
                            _cursorMoved,
                            _wrappedRow);
 
-    return _quickReturn ? S_FALSE : S_OK;
+    return somethingToDo ? S_OK : S_FALSE;
 }
 
 // Routine Description:
@@ -142,9 +142,9 @@ using namespace Microsoft::Console::Types;
         _usingLineRenditions = true;
     }
     // One simple optimization is that we can skip sending the line attributes
-    // when _quickReturn is true. That indicates that we're writing out a single
-    // character, which should preclude there being a rendition switch.
-    if (_usingLineRenditions && !_quickReturn)
+    // when we're writing out a single character, which should preclude there
+    // being a rendition switch.
+    if (_usingLineRenditions && !_invalidMap.one())
     {
         RETURN_IF_FAILED(_MoveCursor({ _lastText.x, targetRow }));
         switch (lineRendition)
diff --git a/src/renderer/vt/state.cpp b/src/renderer/vt/state.cpp
index 881fb0c2cce..f192536e3e6 100644
--- a/src/renderer/vt/state.cpp
+++ b/src/renderer/vt/state.cpp
@@ -37,7 +37,6 @@ VtEngine::VtEngine(_In_ wil::unique_hfile pipe,
     _pool(til::pmr::get_default_resource()),
     _invalidMap(initialViewport.Dimensions(), false, &_pool),
     _scrollDelta(0, 0),
-    _quickReturn(false),
     _clearedAllThisFrame(false),
     _cursorMoved(false),
     _resized(false),
diff --git a/src/renderer/vt/vtrenderer.hpp b/src/renderer/vt/vtrenderer.hpp
index 0125fe17369..b5c487fc1c0 100644
--- a/src/renderer/vt/vtrenderer.hpp
+++ b/src/renderer/vt/vtrenderer.hpp
@@ -113,7 +113,6 @@ namespace Microsoft::Console::Render
         til::point _lastText;
         til::point _scrollDelta;
 
-        bool _quickReturn;
         bool _clearedAllThisFrame;
         bool _cursorMoved;
         bool _resized;
@@ -214,8 +213,6 @@ namespace Microsoft::Console::Render
         [[nodiscard]] HRESULT _RgbUpdateDrawingBrushes(const TextAttribute& textAttributes) noexcept;
         [[nodiscard]] HRESULT _16ColorUpdateDrawingBrushes(const TextAttribute& textAttributes) noexcept;
 
-        bool _WillWriteSingleChar() const;
-
         // buffer space for these two functions to build their lines
         // so they don't have to alloc/free in a tight loop
         std::wstring _bufferLine;


Test patch: