diff --git a/.github/actions/spelling/expect/expect.txt b/.github/actions/spelling/expect/expect.txt index 305c7dd90c2..3aee8c02726 100644 --- a/.github/actions/spelling/expect/expect.txt +++ b/.github/actions/spelling/expect/expect.txt @@ -495,6 +495,7 @@ DSBUFFERDESC DSBVOLUME dsm dsound +DSRCPR DSSCL DSwap DTo diff --git a/src/host/VtInputThread.cpp b/src/host/VtInputThread.cpp index 3c359a3c8e5..13d5466884f 100644 --- a/src/host/VtInputThread.cpp +++ b/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(); - auto engine = std::make_unique(std::move(dispatch), inheritCursor); + auto engine = std::make_unique(std::move(dispatch)); _pInputStateMachine = std::make_unique(std::move(engine)); } @@ -185,8 +185,14 @@ void VtInputThread::_InputThread() return S_OK; } +void VtInputThread::CaptureNextCursorPositionReport() const noexcept +{ + auto& engine = static_cast(_pInputStateMachine->Engine()); + engine.CaptureNextCursorPositionReport(); +} + til::enumset VtInputThread::WaitUntilDA1(DWORD timeout) const noexcept { - const auto& engine = static_cast(_pInputStateMachine->Engine()); + auto& engine = static_cast(_pInputStateMachine->Engine()); return engine.WaitUntilDA1(timeout); } diff --git a/src/host/VtInputThread.hpp b/src/host/VtInputThread.hpp index 50405b5a255..09c0f891375 100644 --- a/src/host/VtInputThread.hpp +++ b/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 WaitUntilDA1(DWORD timeout) const noexcept; private: diff --git a/src/host/VtIo.cpp b/src/host/VtIo.cpp index e2891ab7c60..626a702c06e 100644 --- a/src/host/VtIo.cpp +++ b/src/host/VtIo.cpp @@ -155,7 +155,7 @@ bool VtIo::IsUsingVt() const { if (IsValidHandle(_hInput.get())) { - _pVtInputThread = std::make_unique(std::move(_hInput), _lookingForCursorPosition); + _pVtInputThread = std::make_unique(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"; diff --git a/src/host/VtIo.hpp b/src/host/VtIo.hpp index f2164a68c40..7330bdaad42 100644 --- a/src/host/VtIo.hpp +++ b/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; diff --git a/src/host/screenInfo.cpp b/src/host/screenInfo.cpp index 25fb0ed14a8..4b638ac38b6 100644 --- a/src/host/screenInfo.cpp +++ b/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. diff --git a/src/host/screenInfo.hpp b/src/host/screenInfo.hpp index db3322d1a38..6237d77e23a 100644 --- a/src/host/screenInfo.hpp +++ b/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 _deferredPtyResize{ std::nullopt }; + std::atomic _conptyCursorPositionMayBeWrong = false; static void _handleDeferredResize(SCREEN_INFORMATION& siMain); diff --git a/src/server/ApiDispatchers.cpp b/src/server/ApiDispatchers.cpp index be9a2fae855..efa5f65783f 100644 --- a/src/server/ApiDispatchers.cpp +++ b/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; diff --git a/src/terminal/adapter/InteractDispatch.cpp b/src/terminal/adapter/InteractDispatch.cpp index 8ed76b9d7c7..99566acf1ca 100644 --- a/src/terminal/adapter/InteractDispatch.cpp +++ b/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: diff --git a/src/terminal/parser/InputStateMachineEngine.cpp b/src/terminal/parser/InputStateMachineEngine.cpp index c2a2079b702..cc9571b7578 100644 --- a/src/terminal/parser/InputStateMachineEngine.cpp +++ b/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 pDispatch, const bool lookingForDSR) : +InputStateMachineEngine::InputStateMachineEngine(std::unique_ptr pDispatch) : _pDispatch(std::move(pDispatch)), - _lookingForDSR(lookingForDSR), _doubleClickTime(std::chrono::milliseconds(GetDoubleClickTime())) { THROW_HR_IF_NULL(E_INVALIDARG, _pDispatch.get()); } -til::enumset InputStateMachineEngine::WaitUntilDA1(DWORD timeout) const noexcept +void InputStateMachineEngine::CaptureNextCursorPositionReport() noexcept +{ + _captureNextCursorPositionReport.store(true, std::memory_order_relaxed); +} + +til::enumset InputStateMachineEngine::WaitUntilDA1(DWORD timeout) noexcept { uint64_t val = 0; @@ -118,6 +122,10 @@ til::enumset 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::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; diff --git a/src/terminal/parser/InputStateMachineEngine.hpp b/src/terminal/parser/InputStateMachineEngine.hpp index 1b81f17f927..dc05a6d6766 100644 --- a/src/terminal/parser/InputStateMachineEngine.hpp +++ b/src/terminal/parser/InputStateMachineEngine.hpp @@ -161,9 +161,10 @@ namespace Microsoft::Console::VirtualTerminal class InputStateMachineEngine : public IStateMachineEngine { public: - InputStateMachineEngine(std::unique_ptr pDispatch, const bool lookingForDSR = false); + InputStateMachineEngine(std::unique_ptr pDispatch); - til::enumset WaitUntilDA1(DWORD timeout) const noexcept; + void CaptureNextCursorPositionReport() noexcept; + til::enumset WaitUntilDA1(DWORD timeout) noexcept; bool EncounteredWin32InputModeSequence() const noexcept override; @@ -191,7 +192,7 @@ namespace Microsoft::Console::VirtualTerminal private: const std::unique_ptr _pDispatch; std::atomic _deviceAttributes{ 0 }; - bool _lookingForDSR = false; + std::atomic _captureNextCursorPositionReport{ false }; bool _encounteredWin32InputModeSequence = false; bool _expectingStringTerminator = false; DWORD _mouseButtonState = 0; diff --git a/src/terminal/parser/ut_parser/InputEngineTest.cpp b/src/terminal/parser/ut_parser/InputEngineTest.cpp index 4cee7091d4d..6c7baa9bdc9 100644 --- a/src/terminal/parser/ut_parser/InputEngineTest.cpp +++ b/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(pfn, &testState); - VERIFY_IS_NOT_NULL(dispatch.get()); - auto inputEngine = std::make_unique(std::move(dispatch), true); - VERIFY_IS_NOT_NULL(inputEngine.get()); + auto inputEngine = std::make_unique(std::move(dispatch)); + inputEngine->CaptureNextCursorPositionReport(); auto _stateMachine = std::make_unique(std::move(inputEngine)); - VERIFY_IS_NOT_NULL(_stateMachine); testState._stateMachine = _stateMachine.get(); Log::Comment(NoThrowString().Format(