Repository: microsoft/terminal (language: cpp)

Issue:
Hidden cursors sometimes reappear unexpectedly
### Windows Terminal version

1.18.1421.0

### Windows build number

10.0.19045.2913

### Other Software

_No response_

### Steps to reproduce

I don't have a simple test case to reproduce this yet, but what I'm doing is playing a terminal game that hides the cursor on startup.


### Expected Behavior

I shouldn't be able to see the cursor while the game is in progress.

### Actual Behavior

9 times out of 10, the cursor is visible.

Having added some logging in `AdaptDispatch`, I can see that conhost is receiving a single `DECTCEM` sequence to hide the cursor, but what Windows Terminal ends up receiving is two requests to hide the cursor, followed by another request to show it again. I'm guessing this is the result of some conpty reordering that has gone wrong.

A git bisect suggests that this might be a regression caused by PR #14677. I'm not absolutely certain of that though, because the issue doesn't always reproduce 100% of the time.

I'll try and come up with a simple test case when I have more time, but I just wanted to get the issue filed before I forget about it.








Discussion / hints:
I think this was fixed by PR #15991. Prior to commit 41f7ed73c1e62195919146f689135517aefbaf7b I could reproduce the problem quite easily, but now it seems to be fine. I'm happy for this issue to be closed.
I'll be closing this issue then. 🙂
@lhecker I've noticed that this bug is back again. I can reproduce in 1.20.10822.0 and the latest commit to main. A git bisect suggests it regressed in bdf2f6f2740168363bdba898697e28baa8d7beb7, but I suspect now that it may be timing related, and the "fix" in 41f7ed73c1e62195919146f689135517aefbaf7b and "regression" in bdf2f6f2740168363bdba898697e28baa8d7beb7 are just because the flushing affects the timing. So it's possible the bug was still there the whole time, and that commit isn't really to blame. I still don't have a simple test case, but I'm going to reopen the issue in the meantime so I don't forget about it.
Oww, that's unfortunate to hear. 😕 My series of PRs around the flushing of VtEngine really isn't particularly great, and I'm fairly dissatisfied with them. I couldn't think of an alternative approach to make it robust with any unknown escape sequence though and I kind of still can't. Maybe I'm missing something obvious.

I'm still eager to get rid of VtEngine entirely however [^1], and I'm hoping to get it all done at some point within a year from now.

[^1]: I mentioned this before elsewhere, but what I mean is my idea of an algorithmic console API -> VT translation, and by using the ConPTY `TextBuffer`s only for serving the `ReadConsole*` APIs. That'll certainly result in bugs (when it goes out of sync with the terminal), but I suspect that the total number of bugs then would still be less than the total number of bugs we have now, especially since it goes out of sync right now as well.
I finally figured out the source of the problem. It's this code:
https://github.com/microsoft/terminal/blob/378b6594bd94cf3b27f4e309a11efe25d83de0d9/src/renderer/vt/XtermEngine.cpp#L111-L117

If `_noFlushOnEnd` is set, the buffer won't necessarily be flushed at the end of every frame, so when the insert happens, we could have multiple frames pending in the buffer. This means the HideCursor sequence is inserted at the start of the wrong frame, and in my case it was being inserted _before_ the ShowCursor sequence that it should have been cancelling.

It can easily be fixed just by tracking the buffer size at the start of the frame, and using that saved offset for the insert position here.

Fix patch:
diff --git a/src/renderer/vt/XtermEngine.cpp b/src/renderer/vt/XtermEngine.cpp
index 14822351585..75fc3510fa6 100644
--- a/src/renderer/vt/XtermEngine.cpp
+++ b/src/renderer/vt/XtermEngine.cpp
@@ -45,6 +45,7 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe,
     // visible, then PaintCursor will be called, and we'll set this to true
     // during the frame.
     _nextCursorIsVisible = false;
+    _startOfFrameBufferIndex = _buffer.size();
 
     // Do not perform synchronization clearing in passthrough mode.
     // In passthrough, the terminal leads and we follow what it is
@@ -112,7 +113,7 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe,
         // by prepending a cursor off.
         if (_lastCursorIsVisible != Tribool::False)
         {
-            _buffer.insert(0, "\x1b[?25l");
+            _buffer.insert(_startOfFrameBufferIndex, "\x1b[?25l");
             _lastCursorIsVisible = Tribool::False;
         }
         // If the cursor was NOT previously visible, then that's fine! we don't
diff --git a/src/renderer/vt/state.cpp b/src/renderer/vt/state.cpp
index ea176e42f37..881fb0c2cce 100644
--- a/src/renderer/vt/state.cpp
+++ b/src/renderer/vt/state.cpp
@@ -158,6 +158,7 @@ void VtEngine::_flushImpl() noexcept
     {
         const auto fSuccess = WriteFile(_hFile.get(), _buffer.data(), gsl::narrow_cast<DWORD>(_buffer.size()), nullptr, nullptr);
         _buffer.clear();
+        _startOfFrameBufferIndex = 0;
         if (!fSuccess)
         {
             LOG_LAST_ERROR();
diff --git a/src/renderer/vt/vtrenderer.hpp b/src/renderer/vt/vtrenderer.hpp
index 0135e791169..0125fe17369 100644
--- a/src/renderer/vt/vtrenderer.hpp
+++ b/src/renderer/vt/vtrenderer.hpp
@@ -93,6 +93,7 @@ namespace Microsoft::Console::Render
     protected:
         wil::unique_hfile _hFile;
         std::string _buffer;
+        size_t _startOfFrameBufferIndex = 0;
 
         std::string _formatBuffer;
         std::string _conversionBuffer;


Test patch: