Skip to content

Commit

Permalink
Prevent deadlock in UIA Move API (#10937)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@carlos-zamora @DHowett
carlos-zamora authored and DHowett committed Aug 25, 2021
1 parent 62814f8 commit 32e0759
Show file tree
Hide file tree
Showing 2 changed files with 75 additions and 63 deletions.
136 changes: 73 additions & 63 deletions src/types/UiaTextRangeBase.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
// - <none>
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:
Expand Down Expand Up @@ -994,6 +1006,7 @@ std::wstring UiaTextRangeBase::_getTextValue(std::optional<unsigned int> maxLeng
IFACEMETHODIMP UiaTextRangeBase::Move(_In_ TextUnit unit,
_In_ int count,
_Out_ int* pRetVal) noexcept
try
{
RETURN_HR_IF(E_INVALIDARG, pRetVal == nullptr);
*pRetVal = 0;
Expand All @@ -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)
Expand All @@ -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,
Expand Down
2 changes: 2 additions & 0 deletions src/types/UiaTextRangeBase.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,8 @@ namespace Microsoft::Console::Types

void _getBoundingRect(const til::rectangle textRect, _Inout_ std::vector<double>& coords) const;

void _expandToEnclosingUnit(TextUnit unit);

void
_moveEndpointByUnitCharacter(_In_ const int moveCount,
_In_ const TextPatternRangeEndpoint endpoint,
Expand Down

0 comments on commit 32e0759

Please sign in to comment.