From 32e075956d74a081af1857b2c20b6b04b8a323d1 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Fri, 13 Aug 2021 10:56:34 -0700 Subject: [PATCH] Prevent deadlock in UIA Move API (#10937) Fixes a bug where interacting with Windows Terminal when using Narrator causes Windows Terminal to hang. `UiaTextRangeBase::Move()` locks, but later calls `UiaTextRangeBase::ExpandToEnclosingUnit()` which attempts to lock again. The workaround for this is to introduce a `_expandToEnclosingUnit()` that _does not_ lock the console. Then, `Move()` calls this new method, thus only allowing one lock to be established at a time. This bug is observed to be in v1.11.2221.0 and _not_ in v1.9.1942.0. --- src/types/UiaTextRangeBase.cpp | 136 ++++++++++++++++++--------------- src/types/UiaTextRangeBase.hpp | 2 + 2 files changed, 75 insertions(+), 63 deletions(-) diff --git a/src/types/UiaTextRangeBase.cpp b/src/types/UiaTextRangeBase.cpp index 69f531c09cc..02e8abdb639 100644 --- a/src/types/UiaTextRangeBase.cpp +++ b/src/types/UiaTextRangeBase.cpp @@ -262,59 +262,71 @@ IFACEMETHODIMP UiaTextRangeBase::ExpandToEnclosingUnit(_In_ TextUnit unit) noexc try { - const auto& buffer = _pData->GetTextBuffer(); - const auto bufferSize = _getBufferSize(); - const auto bufferEnd = bufferSize.EndExclusive(); + _expandToEnclosingUnit(unit); + UiaTracing::TextRange::ExpandToEnclosingUnit(unit, *this); + return S_OK; + } + CATCH_RETURN(); +} - if (unit == TextUnit_Character) - { - _start = buffer.GetGlyphStart(_start); - _end = buffer.GetGlyphEnd(_start); - } - else if (unit <= TextUnit_Word) - { - // expand to word - _start = buffer.GetWordStart(_start, _wordDelimiters, true); - _end = buffer.GetWordEnd(_start, _wordDelimiters, true); +// Method Description: +// - Moves _start and _end endpoints to encompass the enclosing text unit. +// (i.e. word --> enclosing word, line --> enclosing line) +// - IMPORTANT: this does _not_ lock the console +// Arguments: +// - attributeId - the UIA text attribute identifier we're expanding by +// Return Value: +// - +void UiaTextRangeBase::_expandToEnclosingUnit(TextUnit unit) +{ + const auto& buffer = _pData->GetTextBuffer(); + const auto bufferSize = _getBufferSize(); + const auto bufferEnd = bufferSize.EndExclusive(); - // GetWordEnd may return the actual end of the TextBuffer. - // If so, just set it to this value of bufferEnd - if (!bufferSize.IsInBounds(_end)) - { - _end = bufferEnd; - } + if (unit == TextUnit_Character) + { + _start = buffer.GetGlyphStart(_start); + _end = buffer.GetGlyphEnd(_start); + } + else if (unit <= TextUnit_Word) + { + // expand to word + _start = buffer.GetWordStart(_start, _wordDelimiters, true); + _end = buffer.GetWordEnd(_start, _wordDelimiters, true); + + // GetWordEnd may return the actual end of the TextBuffer. + // If so, just set it to this value of bufferEnd + if (!bufferSize.IsInBounds(_end)) + { + _end = bufferEnd; } - else if (unit <= TextUnit_Line) + } + else if (unit <= TextUnit_Line) + { + if (_start == bufferEnd) { - if (_start == bufferEnd) - { - // Special case: if we are at the bufferEnd, - // move _start back one, instead of _end forward - _start.X = 0; - _start.Y = base::ClampSub(_start.Y, 1); - _end = bufferEnd; - } - else - { - // expand to line - _start.X = 0; - _end.X = 0; - _end.Y = base::ClampAdd(_start.Y, 1); - } + // Special case: if we are at the bufferEnd, + // move _start back one, instead of _end forward + _start.X = 0; + _start.Y = base::ClampSub(_start.Y, 1); + _end = bufferEnd; } else { - // TODO GH#6986: properly handle "end of buffer" as last character - // instead of last cell - // expand to document - _start = bufferSize.Origin(); - _end = bufferSize.EndExclusive(); + // expand to line + _start.X = 0; + _end.X = 0; + _end.Y = base::ClampAdd(_start.Y, 1); } - - UiaTracing::TextRange::ExpandToEnclosingUnit(unit, *this); - return S_OK; } - CATCH_RETURN(); + else + { + // TODO GH#6986: properly handle "end of buffer" as last character + // instead of last cell + // expand to document + _start = bufferSize.Origin(); + _end = bufferSize.EndExclusive(); + } } // Method Description: @@ -994,6 +1006,7 @@ std::wstring UiaTextRangeBase::_getTextValue(std::optional maxLeng IFACEMETHODIMP UiaTextRangeBase::Move(_In_ TextUnit unit, _In_ int count, _Out_ int* pRetVal) noexcept +try { RETURN_HR_IF(E_INVALIDARG, pRetVal == nullptr); *pRetVal = 0; @@ -1011,26 +1024,22 @@ IFACEMETHODIMP UiaTextRangeBase::Move(_In_ TextUnit unit, constexpr auto endpoint = TextPatternRangeEndpoint::TextPatternRangeEndpoint_Start; constexpr auto preventBufferEnd = true; const auto wasDegenerate = IsDegenerate(); - try + if (unit == TextUnit::TextUnit_Character) { - if (unit == TextUnit::TextUnit_Character) - { - _moveEndpointByUnitCharacter(count, endpoint, pRetVal, preventBufferEnd); - } - else if (unit <= TextUnit::TextUnit_Word) - { - _moveEndpointByUnitWord(count, endpoint, pRetVal, preventBufferEnd); - } - else if (unit <= TextUnit::TextUnit_Line) - { - _moveEndpointByUnitLine(count, endpoint, pRetVal, preventBufferEnd); - } - else if (unit <= TextUnit::TextUnit_Document) - { - _moveEndpointByUnitDocument(count, endpoint, pRetVal, preventBufferEnd); - } + _moveEndpointByUnitCharacter(count, endpoint, pRetVal, preventBufferEnd); + } + else if (unit <= TextUnit::TextUnit_Word) + { + _moveEndpointByUnitWord(count, endpoint, pRetVal, preventBufferEnd); + } + else if (unit <= TextUnit::TextUnit_Line) + { + _moveEndpointByUnitLine(count, endpoint, pRetVal, preventBufferEnd); + } + else if (unit <= TextUnit::TextUnit_Document) + { + _moveEndpointByUnitDocument(count, endpoint, pRetVal, preventBufferEnd); } - CATCH_RETURN(); // If we actually moved... if (*pRetVal != 0) @@ -1044,13 +1053,14 @@ IFACEMETHODIMP UiaTextRangeBase::Move(_In_ TextUnit unit, else { // then just expand to get our _end - ExpandToEnclosingUnit(unit); + _expandToEnclosingUnit(unit); } } UiaTracing::TextRange::Move(unit, count, *pRetVal, *this); return S_OK; } +CATCH_RETURN(); IFACEMETHODIMP UiaTextRangeBase::MoveEndpointByUnit(_In_ TextPatternRangeEndpoint endpoint, _In_ TextUnit unit, diff --git a/src/types/UiaTextRangeBase.hpp b/src/types/UiaTextRangeBase.hpp index 47b9778a0b0..02bf0461bb0 100644 --- a/src/types/UiaTextRangeBase.hpp +++ b/src/types/UiaTextRangeBase.hpp @@ -153,6 +153,8 @@ namespace Microsoft::Console::Types void _getBoundingRect(const til::rectangle textRect, _Inout_ std::vector& coords) const; + void _expandToEnclosingUnit(TextUnit unit); + void _moveEndpointByUnitCharacter(_In_ const int moveCount, _In_ const TextPatternRangeEndpoint endpoint,