DCS sequences split across multiple "packets" can be corrupted by conpty

[j4james]

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:

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.

[j4james]

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".

[j4james]

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.

[j4james]

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:

terminal/src/renderer/vt/paint.cpp

Lines 32 to 36 in 378b659

	// If there's nothing to do, quick return
	auto somethingToDo = _invalidMap.any() ||
	_scrollDelta != til::point{ 0, 0 } ||
	_cursorMoved ||
	_titleChanged;

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:

terminal/src/renderer/vt/XtermEngine.cpp

Lines 38 to 40 in 378b659

	[[nodiscard]] HRESULT XtermEngine::StartPaint() noexcept
	{
	RETURN_IF_FAILED(VtEngine::StartPaint());

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 Issues with cursor invalidation in GDI #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.

[j4james]

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.