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.

[github-actions[bot]]

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), similarity score: 0.86
• Conpty "pass-through" doesn't maintain order of operations (#8698), similarity score: 0.81

Closed similar issues:

• Escape sequences behave strangely over conpty when VirtualTerminalLevel is set (#1965), similarity score: 0.78
• ConPTY not emitting OSC sequences longer than 256 characters (Windows 10) (#15551), similarity score: 0.75

Note: You can give me feedback by thumbs upping or thumbs downing this comment.

[lhecker]

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):

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

[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:

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.

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