Fetch the terminal cursor position after a resize

.github/actions/spelling/expect/expect.txt

@@ -495,6 +495,7 @@ DSBUFFERDESC
 DSBVOLUME
 dsm
 dsound
+DSRCPR
 DSSCL
 DSwap
 DTo

src/host/VtInputThread.cpp

@@ -21,13 +21,13 @@ using namespace Microsoft::Console::VirtualTerminal;
 // - hPipe - a handle to the file representing the read end of the VT pipe.
 // - inheritCursor - a bool indicating if the state machine should expect a
 //      cursor positioning sequence. See MSFT:15681311.
-VtInputThread::VtInputThread(_In_ wil::unique_hfile hPipe, const bool inheritCursor) :
+VtInputThread::VtInputThread(_In_ wil::unique_hfile hPipe) :
     _hFile{ std::move(hPipe) }
 {
     THROW_HR_IF(E_HANDLE, _hFile.get() == INVALID_HANDLE_VALUE);
 
     auto dispatch = std::make_unique<InteractDispatch>();
-    auto engine = std::make_unique<InputStateMachineEngine>(std::move(dispatch), inheritCursor);
+    auto engine = std::make_unique<InputStateMachineEngine>(std::move(dispatch));
     _pInputStateMachine = std::make_unique<StateMachine>(std::move(engine));
 }
 
@@ -185,8 +185,14 @@ void VtInputThread::_InputThread()
     return S_OK;
 }
 
+void VtInputThread::CaptureNextCursorPositionReport() const noexcept
+{
+    auto& engine = static_cast<InputStateMachineEngine&>(_pInputStateMachine->Engine());
+    engine.CaptureNextCursorPositionReport();
+}
+
 til::enumset<DeviceAttribute, uint64_t> VtInputThread::WaitUntilDA1(DWORD timeout) const noexcept
 {
-    const auto& engine = static_cast<InputStateMachineEngine&>(_pInputStateMachine->Engine());
+    auto& engine = static_cast<InputStateMachineEngine&>(_pInputStateMachine->Engine());
     return engine.WaitUntilDA1(timeout);
 }

src/host/VtInputThread.hpp

@@ -26,9 +26,10 @@ namespace Microsoft::Console
     class VtInputThread
     {
     public:
-        VtInputThread(_In_ wil::unique_hfile hPipe, const bool inheritCursor);
+        explicit VtInputThread(_In_ wil::unique_hfile hPipe);
 
         [[nodiscard]] HRESULT Start();
+        void CaptureNextCursorPositionReport() const noexcept;
         til::enumset<VirtualTerminal::DeviceAttribute, uint64_t> WaitUntilDA1(DWORD timeout) const noexcept;
 
     private:

src/host/VtIo.cpp

@@ -155,7 +155,7 @@ bool VtIo::IsUsingVt() const
     {
         if (IsValidHandle(_hInput.get()))
         {
-            _pVtInputThread = std::make_unique<VtInputThread>(std::move(_hInput), _lookingForCursorPosition);
+            _pVtInputThread = std::make_unique<VtInputThread>(std::move(_hInput));
         }
     }
     CATCH_RETURN();
@@ -177,7 +177,7 @@ bool VtIo::IsUsingVt() const
             // wait for the DA1 response below and effectively wait for both.
             if (_lookingForCursorPosition)
             {
-                writer.WriteUTF8("\x1b[6n"); // Cursor Position Report (DSR CPR)
+                writer.WriteDSRCPR();
             }
 
             // GH#4999 - Send a sequence to the connected terminal to request
@@ -720,6 +720,19 @@ void VtIo::Writer::WriteASB(bool enabled) const
     _io->_back.append(&buf[0], std::size(buf) - 1);
 }
 
+// DSR CPR: Cursor Position Report
+bool VtIo::Writer::WriteDSRCPR() const
+{
+    if (!_io->_pVtInputThread)
+    {
+        return false;
+    }
+
+    _io->_back.append("\x1b[6n");
+    _io->_pVtInputThread->CaptureNextCursorPositionReport();
+    return true;
+}
+
 void VtIo::Writer::WriteWindowVisibility(bool visible) const
 {
     char buf[] = "\x1b[1t";

src/host/VtIo.hpp

@@ -40,6 +40,7 @@ namespace Microsoft::Console::VirtualTerminal
             void WriteSGR1006(bool enabled) const;
             void WriteDECAWM(bool enabled) const;
             void WriteASB(bool enabled) const;
+            bool WriteDSRCPR() const;
             void WriteWindowVisibility(bool visible) const;
             void WriteWindowTitle(std::wstring_view title) const;
             void WriteAttributes(const TextAttribute& attributes) const;

src/host/screenInfo.cpp

@@ -1404,6 +1404,8 @@ NT_CATCH_RETURN()
 
     if (SUCCEEDED_NTSTATUS(status))
     {
+        SetConptyCursorPositionMayBeWrong();
+
         // Fire off an event to let accessibility apps know the layout has changed.
         if (IsActiveScreenBuffer())
         {
@@ -1422,6 +1424,86 @@ NT_CATCH_RETURN()
     return status;
 }
 
+// If we're ConPTY, our copy of the buffer may be out of sync with the terminal,
+// because our VT, resize reflow, etc., implementation may be different.
+// For some operations we set a flag to indicate the cursor position may be wrong.
+// For GetConsoleScreenBufferInfo(Ex) we then fetch the latest position from the terminal.
+// This fixes some of the most glaring out of sync issues. See GH#18725.
+bool SCREEN_INFORMATION::ConptyCursorPositionMayBeWrong() const noexcept
+{
+    return _conptyCursorPositionMayBeWrong.load(std::memory_order_relaxed);
+}
+
+// This should be called whenever we do something that may desynchronize
+// our cursor position from the terminal's, e.g. a buffer resize.
+// See ConptyCursorPositionMayBeWrong().
+void SCREEN_INFORMATION::SetConptyCursorPositionMayBeWrong() noexcept
+{
+    auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
+    assert(gci.IsConsoleLocked());
+
+    if (gci.IsInVtIoMode())
+    {
+        _conptyCursorPositionMayBeWrong.store(true, std::memory_order_relaxed);
+    }
+}
+
+// This should be called whenever we've synchronized our cursor position again.
+// See ConptyCursorPositionMayBeWrong().
+void SCREEN_INFORMATION::ResetConptyCursorPositionMayBeWrong() noexcept
+{
+    _conptyCursorPositionMayBeWrong.store(false, std::memory_order_relaxed);
+    til::atomic_notify_all(_conptyCursorPositionMayBeWrong);
+}
+
+// Call this to synchronously wait until the ConPTY cursor position
+// is known to be correct again. To do so, this emits a DSR CPR sequence
+// and waits for a response from the terminal.
+// See ConptyCursorPositionMayBeWrong().
+void SCREEN_INFORMATION::WaitForConptyCursorPositionToBeSynchronized() noexcept
+{
+    if (!_conptyCursorPositionMayBeWrong.load(std::memory_order_relaxed))
+    {
+        return;
+    }
+
+    auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
+
+    {
+        gci.LockConsole();
+        const auto exit = wil::scope_exit([&] { gci.UnlockConsole(); });
+        auto writer = gci.GetVtWriterForBuffer(this);
+
+        if (!writer || !writer.WriteDSRCPR())
+        {
+            _conptyCursorPositionMayBeWrong.store(false, std::memory_order_relaxed);
+            return;
+        }
+
+        writer.Submit();
+    }
+
+    // If you were to hold the lock, the ConPTY input thread couldn't
+    // process any input and thus couldn't update the cursor position.
+    assert(!gci.IsConsoleLocked());
+
+    for (;;)
+    {
+        if (!_conptyCursorPositionMayBeWrong.load(std::memory_order::relaxed))
+        {
+            break;
+        }
+
+        // atomic_wait() returns false when the timeout expires.
+        // Technically we should decrement the timeout with each iteration,
+        // but I suspect infinite spurious wake-ups are a theoretical problem.
+        if (!til::atomic_wait(_conptyCursorPositionMayBeWrong, true, 500))
+        {
+            break;
+        }
+    }
+}
+
 // Routine Description:
 // - Given a rectangle containing screen buffer coordinates (character-level positioning, not pixel)
 //   This method will trim the rectangle to ensure it is within the buffer.

src/host/screenInfo.hpp

@@ -222,6 +222,11 @@ class SCREEN_INFORMATION : public ConsoleObjectHeader, public Microsoft::Console
     [[nodiscard]] NTSTATUS ResizeWithReflow(const til::size coordnewScreenSize);
     [[nodiscard]] NTSTATUS ResizeTraditional(const til::size coordNewScreenSize);
 
+    bool ConptyCursorPositionMayBeWrong() const noexcept;
+    void SetConptyCursorPositionMayBeWrong() noexcept;
+    void ResetConptyCursorPositionMayBeWrong() noexcept;
+    void WaitForConptyCursorPositionToBeSynchronized() noexcept;
+
 private:
     SCREEN_INFORMATION(_In_ Microsoft::Console::Interactivity::IWindowMetrics* pMetrics,
                        const TextAttribute popupAttributes,
@@ -280,6 +285,7 @@ class SCREEN_INFORMATION : public ConsoleObjectHeader, public Microsoft::Console
     til::CoordType _virtualBottom;
 
     std::optional<til::size> _deferredPtyResize{ std::nullopt };
+    std::atomic<bool> _conptyCursorPositionMayBeWrong = false;
 
     static void _handleDeferredResize(SCREEN_INFORMATION& siMain);
 

src/server/ApiDispatchers.cpp

@@ -584,6 +584,12 @@ constexpr T saturate(auto val)
     SCREEN_INFORMATION* pObj;
     RETURN_IF_FAILED(pObjectHandle->GetScreenBuffer(GENERIC_READ, &pObj));
 
+    // See ConptyCursorPositionMayBeWrong() for details.
+    if (pObj->ConptyCursorPositionMayBeWrong())
+    {
+        pObj->WaitForConptyCursorPositionToBeSynchronized();
+    }
+
     m->_pApiRoutines->GetConsoleScreenBufferInfoExImpl(*pObj, ex);
 
     a->FullscreenSupported = !!ex.bFullscreenSupported;

src/terminal/adapter/InteractDispatch.cpp

@@ -149,6 +149,9 @@ void InteractDispatch::WindowManipulation(const DispatchTypes::WindowManipulatio
 // - col: The column to move the cursor to.
 void InteractDispatch::MoveCursor(const VTInt row, const VTInt col)
 {
+    const auto& api = ServiceLocator::LocateGlobals().api;
+    auto& info = ServiceLocator::LocateGlobals().getConsoleInformation().GetActiveOutputBuffer();
+
     // First retrieve some information about the buffer
     const auto viewport = _api.GetBufferAndViewport().viewport;
 
@@ -159,9 +162,11 @@ void InteractDispatch::MoveCursor(const VTInt row, const VTInt col)
     coordCursor.x = std::clamp(coordCursor.x, viewport.left, viewport.right);
 
     // Finally, attempt to set the adjusted cursor position back into the console.
-    const auto api = gsl::not_null{ ServiceLocator::LocateGlobals().api };
-    auto& info = ServiceLocator::LocateGlobals().getConsoleInformation().GetActiveOutputBuffer();
     LOG_IF_FAILED(api->SetConsoleCursorPositionImpl(info, coordCursor));
+
+    // Unblock any callers inside SCREEN_INFORMATION::WaitForConptyCursorPositionToBeSynchronized().
+    // The cursor position has now been updated to the terminal's.
+    info.ResetConptyCursorPositionMayBeWrong();
 }
 
 // Routine Description:

src/terminal/parser/InputStateMachineEngine.cpp

@@ -89,15 +89,19 @@ static bool operator==(const Ss3ToVkey& pair, const Ss3ActionCodes code) noexcep
     return pair.action == code;
 }
 
-InputStateMachineEngine::InputStateMachineEngine(std::unique_ptr<IInteractDispatch> pDispatch, const bool lookingForDSR) :
+InputStateMachineEngine::InputStateMachineEngine(std::unique_ptr<IInteractDispatch> pDispatch) :
     _pDispatch(std::move(pDispatch)),
-    _lookingForDSR(lookingForDSR),
     _doubleClickTime(std::chrono::milliseconds(GetDoubleClickTime()))
 {
     THROW_HR_IF_NULL(E_INVALIDARG, _pDispatch.get());
 }
 
-til::enumset<DeviceAttribute, uint64_t> InputStateMachineEngine::WaitUntilDA1(DWORD timeout) const noexcept
+void InputStateMachineEngine::CaptureNextCursorPositionReport() noexcept
+{
+    _captureNextCursorPositionReport.store(true, std::memory_order_relaxed);
+}
+
+til::enumset<DeviceAttribute, uint64_t> InputStateMachineEngine::WaitUntilDA1(DWORD timeout) noexcept
 {
     uint64_t val = 0;
 
@@ -118,6 +122,10 @@ til::enumset<DeviceAttribute, uint64_t> InputStateMachineEngine::WaitUntilDA1(DW
         }
     }
 
+    // VtIo first sends a DSR CPR and then a DA1 request.
+    // If we encountered a DA1 response here, the DSR request is definitely done now.
+    _captureNextCursorPositionReport.store(false, std::memory_order_relaxed);
+
     return til::enumset<DeviceAttribute, uint64_t>::from_bits(val);
 }
 
@@ -413,12 +421,9 @@ bool InputStateMachineEngine::ActionCsiDispatch(const VTID id, const VTParameter
         // The F3 case is special - it shares a code with the DeviceStatusResponse.
         // If we're looking for that response, then do that, and break out.
         // Else, fall though to the _GetCursorKeysModifierState handler.
-        if (_lookingForDSR)
+        if (_captureNextCursorPositionReport.exchange(false, std::memory_order_relaxed))
         {
             _pDispatch->MoveCursor(parameters.at(0), parameters.at(1));
-            // Right now we're only looking for on initial cursor
-            //      position response. After that, only look for F3.
-            _lookingForDSR = false;
             return true;
         }
         // Heuristic: If the hosting terminal used the win32 input mode, chances are high
@@ -491,10 +496,6 @@ bool InputStateMachineEngine::ActionCsiDispatch(const VTID id, const VTParameter
 
             _deviceAttributes.fetch_or(attributes.bits(), std::memory_order_relaxed);
             til::atomic_notify_all(_deviceAttributes);
-
-            // VtIo first sends a DSR CPR and then a DA1 request.
-            // If we encountered a DA1 response here, the DSR request is definitely done now.
-            _lookingForDSR = false;
             return true;
         }
         return false;

src/terminal/parser/InputStateMachineEngine.hpp

@@ -161,9 +161,10 @@ namespace Microsoft::Console::VirtualTerminal
     class InputStateMachineEngine : public IStateMachineEngine
     {
     public:
-        InputStateMachineEngine(std::unique_ptr<IInteractDispatch> pDispatch, const bool lookingForDSR = false);
+        InputStateMachineEngine(std::unique_ptr<IInteractDispatch> pDispatch);
 
-        til::enumset<DeviceAttribute, uint64_t> WaitUntilDA1(DWORD timeout) const noexcept;
+        void CaptureNextCursorPositionReport() noexcept;
+        til::enumset<DeviceAttribute, uint64_t> WaitUntilDA1(DWORD timeout) noexcept;
 
         bool EncounteredWin32InputModeSequence() const noexcept override;
 
@@ -191,7 +192,7 @@ namespace Microsoft::Console::VirtualTerminal
     private:
         const std::unique_ptr<IInteractDispatch> _pDispatch;
         std::atomic<uint64_t> _deviceAttributes{ 0 };
-        bool _lookingForDSR = false;
+        std::atomic<bool> _captureNextCursorPositionReport{ false };
         bool _encounteredWin32InputModeSequence = false;
         bool _expectingStringTerminator = false;
         DWORD _mouseButtonState = 0;

src/terminal/parser/ut_parser/InputEngineTest.cpp

@@ -679,11 +679,9 @@ void InputEngineTest::CursorPositioningTest()
     auto pfn = std::bind(&TestState::TestInputCallback, &testState, std::placeholders::_1);
 
     auto dispatch = std::make_unique<TestInteractDispatch>(pfn, &testState);
-    VERIFY_IS_NOT_NULL(dispatch.get());
-    auto inputEngine = std::make_unique<InputStateMachineEngine>(std::move(dispatch), true);
-    VERIFY_IS_NOT_NULL(inputEngine.get());
+    auto inputEngine = std::make_unique<InputStateMachineEngine>(std::move(dispatch));
+    inputEngine->CaptureNextCursorPositionReport();
     auto _stateMachine = std::make_unique<StateMachine>(std::move(inputEngine));
-    VERIFY_IS_NOT_NULL(_stateMachine);
     testState._stateMachine = _stateMachine.get();
 
     Log::Comment(NoThrowString().Format(