Repository: microsoft/terminal (language: cpp)

Issue:
Accessibility: NVDA emits errors in WT1.15 preview
### Windows Terminal version

1.15.2283.0

### Windows build number

25188.1000

### Other Software

NVDA 2022.2.2

### Steps to reproduce

1. Open bash in windows terminal
2. type nano /etc/hosts

### Expected Behavior

No error in NVDA

### Actual Behavior

NVDA emits the following error:

```
ERROR - NVDAObjects.behaviors.LiveText._monitor (18:21:06.798) - Dynamic_WinTerminalUIAEditableTextWithAutoSelectDetectionUIA._monitorThread (22344):
Error getting or calculating new text
Traceback (most recent call last):
  File "NVDAObjects\behaviors.pyc", line 386, in _monitor
  File "NVDAObjects\behaviors.pyc", line 350, in _getText
  File "diffHandler.pyc", line 62, in _getText
  File "baseObject.pyc", line 26, in __get__
  File "NVDAObjects\UIA\__init__.pyc", line 848, in _get_text
  File "NVDAObjects\UIA\__init__.pyc", line 555, in _getTextFromUIARange
  File "monkeyPatches\comtypesMonkeyPatches.pyc", line 32, in __call__
_ctypes.COMError: (-2147467259, 'Unspecified error', (None, None, None, 0, None))
```

I wasn't able to reproduce this with WT1.14, so therefore I'm filing this here since there might be a regression in UIA related code. @codeofdusk could you reproduce this?


Discussion / hints:
I've also seen this, but not with `wtNotifications`.

This is probably related to broken assumptions when switching to/from the alt buffer.
This error indeed has nothing to do with the notifications implementation.

CC @carlos-zamora just want to make sure that this bug doesn't end up in a release, since that would be annoying for many sr users.
@leonardder (or @codeofdusk since you're familiar with the space) I think I need a little more context here. It looks like NVDA is calling the UIA API `GetText()` off of a `TextRange`. That corresponds to the following code in Windows Terminal: https://github.com/microsoft/terminal/blob/f2a691863bb4add22d225ca4f912345eb815a8d9/src/types/UiaTextRangeBase.cpp#L954-L1023

It looks like we emit an error if...
1.  the second parameter (the out param) wasn't provided properly
2.  the first parameter (dictating the max length) is less than -1
3.  the newly created terminal content isn't fully initialized yet
4.  we're out of memory and we can't create the string result to return
5. (as @codeofdusk said) the text range points to content that doesn't exist (this is a quick-n-easy way to invalidate a text range that was on the alt buffer when we're now on the main buffer)

I assume the main culprit here is number 5, but to be certain, could you provide the following:
- What UIA APIs was NVDA calling? Obviously `GetText()` is the one that returns an error, but what were the calls leading up to that. 
- If you can explain how I can get a trace from NVDA, that'd be even better.
Yeah, that error code is `E_FAIL` so it's 100% that alt buffer thing.

Fix patch:
diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp
index 2c7e0359387..21f153d1794 100644
--- a/src/buffer/out/textBuffer.cpp
+++ b/src/buffer/out/textBuffer.cpp
@@ -1138,7 +1138,7 @@ til::point TextBuffer::GetWordStart(const til::point target, const std::wstring_
         // that it actually points to a space in the buffer
         copy = bufferSize.BottomRightInclusive();
     }
-    else if (target >= limit)
+    else if (bufferSize.CompareInBounds(target, limit, true) >= 0)
     {
         // if at/past the limit --> clamp to limit
         copy = limitOptional.value_or(bufferSize.BottomRightInclusive());
@@ -1254,7 +1254,7 @@ til::point TextBuffer::GetWordEnd(const til::point target, const std::wstring_vi
     // Already at/past the limit. Can't move forward.
     const auto bufferSize{ GetSize() };
     const auto limit{ limitOptional.value_or(bufferSize.EndExclusive()) };
-    if (target >= limit)
+    if (bufferSize.CompareInBounds(target, limit, true) >= 0)
     {
         return target;
     }
@@ -1282,7 +1282,7 @@ til::point TextBuffer::_GetWordEndForAccessibility(const til::point target, cons
     const auto bufferSize{ GetSize() };
     auto result{ target };
 
-    if (target >= limit)
+    if (bufferSize.CompareInBounds(target, limit, true) >= 0)
     {
         // if we're already on/past the last RegularChar,
         // clamp result to that position
@@ -1419,7 +1419,7 @@ bool TextBuffer::MoveToNextWord(til::point& pos, const std::wstring_view wordDel
     const auto limit{ limitOptional.value_or(bufferSize.EndExclusive()) };
     const auto copy{ _GetWordEndForAccessibility(pos, wordDelimiters, limit) };
 
-    if (copy >= limit)
+    if (bufferSize.CompareInBounds(copy, limit, true) >= 0)
     {
         return false;
     }
@@ -1466,7 +1466,7 @@ til::point TextBuffer::GetGlyphStart(const til::point pos, std::optional<til::po
     const auto limit{ limitOptional.value_or(bufferSize.EndExclusive()) };
 
     // Clamp pos to limit
-    if (resultPos > limit)
+    if (bufferSize.CompareInBounds(resultPos, limit, true) > 0)
     {
         resultPos = limit;
     }
@@ -1494,7 +1494,7 @@ til::point TextBuffer::GetGlyphEnd(const til::point pos, bool accessibilityMode,
     const auto limit{ limitOptional.value_or(bufferSize.EndExclusive()) };
 
     // Clamp pos to limit
-    if (resultPos > limit)
+    if (bufferSize.CompareInBounds(resultPos, limit, true) > 0)
     {
         resultPos = limit;
     }
@@ -1524,19 +1524,9 @@ til::point TextBuffer::GetGlyphEnd(const til::point pos, bool accessibilityMode,
 bool TextBuffer::MoveToNextGlyph(til::point& pos, bool allowExclusiveEnd, std::optional<til::point> limitOptional) const
 {
     const auto bufferSize = GetSize();
-    bool pastEndInclusive;
-    til::point limit;
-    {
-        // if the limit is past the end of the buffer,
-        // 1) clamp limit to end of buffer
-        // 2) set pastEndInclusive
-        const auto endInclusive{ bufferSize.BottomRightInclusive() };
-        const auto val = limitOptional.value_or(endInclusive);
-        pastEndInclusive = val > endInclusive;
-        limit = pastEndInclusive ? endInclusive : val;
-    }
+    const auto limit{ limitOptional.value_or(bufferSize.EndExclusive()) };
 
-    const auto distanceToLimit{ bufferSize.CompareInBounds(pos, limit) + (pastEndInclusive ? 1 : 0) };
+    const auto distanceToLimit{ bufferSize.CompareInBounds(pos, limit, true) };
     if (distanceToLimit >= 0)
     {
         // Corner Case: we're on/past the limit
@@ -1579,7 +1569,7 @@ bool TextBuffer::MoveToPreviousGlyph(til::point& pos, std::optional<til::point>
     const auto bufferSize = GetSize();
     const auto limit{ limitOptional.value_or(bufferSize.EndExclusive()) };
 
-    if (pos > limit)
+    if (bufferSize.CompareInBounds(pos, limit, true) > 0)
     {
         // we're past the end
         // clamp us to the limit
@@ -1621,7 +1611,7 @@ const std::vector<til::inclusive_rect> TextBuffer::GetTextRects(til::point start
     // (0,0) is the top-left of the screen
     // the physically "higher" coordinate is closer to the top-left
     // the physically "lower" coordinate is closer to the bottom-right
-    const auto [higherCoord, lowerCoord] = start <= end ?
+    const auto [higherCoord, lowerCoord] = bufferSize.CompareInBounds(start, end) <= 0 ?
                                                std::make_tuple(start, end) :
                                                std::make_tuple(end, start);
 
diff --git a/src/cascadia/TerminalCore/TerminalSelection.cpp b/src/cascadia/TerminalCore/TerminalSelection.cpp
index 2dc0e2eadcd..f9df66e2659 100644
--- a/src/cascadia/TerminalCore/TerminalSelection.cpp
+++ b/src/cascadia/TerminalCore/TerminalSelection.cpp
@@ -251,7 +251,7 @@ void Terminal::SetSelectionEnd(const til::point viewportPos, std::optional<Selec
 // - the new start/end for a selection
 std::pair<til::point, til::point> Terminal::_PivotSelection(const til::point targetPos, bool& targetStart) const
 {
-    if (targetStart = targetPos <= _selection->pivot)
+    if (targetStart = _activeBuffer().GetSize().CompareInBounds(targetPos, _selection->pivot) <= 0)
     {
         // target is before pivot
         // treat target as start
@@ -698,7 +698,7 @@ void Terminal::_MoveByWord(SelectionDirection direction, til::point& pos)
     case SelectionDirection::Left:
     {
         const auto wordStartPos{ _activeBuffer().GetWordStart(pos, _wordDelimiters) };
-        if (_selection->pivot < pos)
+        if (_activeBuffer().GetSize().CompareInBounds(_selection->pivot, pos) < 0)
         {
             // If we're moving towards the pivot, move one more cell
             pos = wordStartPos;
@@ -721,7 +721,7 @@ void Terminal::_MoveByWord(SelectionDirection direction, til::point& pos)
     case SelectionDirection::Right:
     {
         const auto wordEndPos{ _activeBuffer().GetWordEnd(pos, _wordDelimiters) };
-        if (pos < _selection->pivot)
+        if (_activeBuffer().GetSize().CompareInBounds(pos, _selection->pivot) < 0)
         {
             // If we're moving towards the pivot, move one more cell
             pos = _activeBuffer().GetWordEnd(pos, _wordDelimiters);
diff --git a/src/interactivity/win32/ut_interactivity_win32/UiaTextRangeTests.cpp b/src/interactivity/win32/ut_interactivity_win32/UiaTextRangeTests.cpp
index 5185811315f..34b3ff374cb 100644
--- a/src/interactivity/win32/ut_interactivity_win32/UiaTextRangeTests.cpp
+++ b/src/interactivity/win32/ut_interactivity_win32/UiaTextRangeTests.cpp
@@ -484,9 +484,9 @@ class UiaTextRangeTests
 
         Log::Comment(L"_start and end should be 2 units apart. Sign depends on order of comparison.");
         THROW_IF_FAILED(utr1->CompareEndpoints(TextPatternRangeEndpoint_End, utr2.Get(), TextPatternRangeEndpoint_End, &comparison));
-        VERIFY_IS_TRUE(comparison == -1);
+        VERIFY_IS_TRUE(comparison == -2);
         THROW_IF_FAILED(utr2->CompareEndpoints(TextPatternRangeEndpoint_End, utr1.Get(), TextPatternRangeEndpoint_End, &comparison));
-        VERIFY_IS_TRUE(comparison == 1);
+        VERIFY_IS_TRUE(comparison == 2);
     }
 
     TEST_METHOD(ExpandToEnclosingUnit)
diff --git a/src/types/UiaTextRangeBase.cpp b/src/types/UiaTextRangeBase.cpp
index 877bed745a1..4efd48977aa 100644
--- a/src/types/UiaTextRangeBase.cpp
+++ b/src/types/UiaTextRangeBase.cpp
@@ -70,7 +70,7 @@ try
     RETURN_IF_FAILED(RuntimeClassInitialize(pData, pProvider, wordDelimiters));
 
     // start is before/at end, so this is valid
-    if (start <= end)
+    if (_pData->GetTextBuffer().GetSize().CompareInBounds(start, end, true) <= 0)
     {
         _start = start;
         _end = end;
@@ -166,7 +166,7 @@ bool UiaTextRangeBase::SetEndpoint(TextPatternRangeEndpoint endpoint, const til:
     case TextPatternRangeEndpoint_End:
         _end = val;
         // if end is before start...
-        if (_end < _start)
+        if (bufferSize.CompareInBounds(_end, _start, true) < 0)
         {
             // make this range degenerate at end
             _start = _end;
@@ -175,7 +175,7 @@ bool UiaTextRangeBase::SetEndpoint(TextPatternRangeEndpoint endpoint, const til:
     case TextPatternRangeEndpoint_Start:
         _start = val;
         // if start is after end...
-        if (_start > _end)
+        if (bufferSize.CompareInBounds(_start, _end, true) > 0)
         {
             // make this range degenerate at start
             _end = _start;
@@ -246,13 +246,13 @@ try
     const auto mine = GetEndpoint(endpoint);
 
     // TODO GH#5406: create a different UIA parent object for each TextBuffer
-    //   We should return E_FAIL if we detect that the endpoints are from two different TextBuffer objects.
-    //   For now, we're fine to not do that because we're not using any code that can FAIL_FAST anymore.
+    //   This is a temporary solution to comparing two UTRs from different TextBuffers
+    //   Ensure both endpoints fit in the current buffer.
+    const auto bufferSize = _pData->GetTextBuffer().GetSize();
+    RETURN_HR_IF(E_FAIL, !bufferSize.IsInBounds(mine, true) || !bufferSize.IsInBounds(other, true));
 
     // compare them
-    *pRetVal = mine < other ? -1 :
-               mine > other ? 1 :
-                              0;
+    *pRetVal = bufferSize.CompareInBounds(mine, other, true);
 
     UiaTracing::TextRange::CompareEndpoints(*this, endpoint, *range, targetEndpoint, *pRetVal);
     return S_OK;
@@ -293,7 +293,7 @@ void UiaTextRangeBase::_expandToEnclosingUnit(TextUnit unit)
     // If we're past document end,
     // set us to ONE BEFORE the document end.
     // This allows us to expand properly.
-    if (_start >= documentEnd)
+    if (bufferSize.CompareInBounds(_start, documentEnd, true) >= 0)
     {
         _start = documentEnd;
         bufferSize.DecrementInBounds(_start, true);
@@ -653,8 +653,8 @@ try
         bufferSize.IncrementInBounds(end, true);
 
         // make sure what was found is within the bounds of the current range
-        if ((searchDirection == Search::Direction::Forward && end < _end) ||
-            (searchDirection == Search::Direction::Backward && start > _start))
+        if ((searchDirection == Search::Direction::Forward && bufferSize.CompareInBounds(end, _end, true) < 0) ||
+            (searchDirection == Search::Direction::Backward && bufferSize.CompareInBounds(start, _start) > 0))
         {
             RETURN_IF_FAILED(Clone(ppRetVal));
             auto& range = static_cast<UiaTextRangeBase&>(**ppRetVal);
@@ -872,7 +872,7 @@ IFACEMETHODIMP UiaTextRangeBase::GetBoundingRectangles(_Outptr_result_maybenull_
 
         // startAnchor: the earliest til::point we will get a bounding rect for
         auto startAnchor = GetEndpoint(TextPatternRangeEndpoint_Start);
-        if (startAnchor < viewportOrigin)
+        if (bufferSize.CompareInBounds(startAnchor, viewportOrigin, true) < 0)
         {
             // earliest we can be is the origin
             startAnchor = viewportOrigin;
@@ -880,7 +880,7 @@ IFACEMETHODIMP UiaTextRangeBase::GetBoundingRectangles(_Outptr_result_maybenull_
 
         // endAnchor: the latest til::point we will get a bounding rect for
         auto endAnchor = GetEndpoint(TextPatternRangeEndpoint_End);
-        if (endAnchor > viewportEnd)
+        if (bufferSize.CompareInBounds(endAnchor, viewportEnd, true) > 0)
         {
             // latest we can be is the viewport end
             endAnchor = viewportEnd;
@@ -889,7 +889,7 @@ IFACEMETHODIMP UiaTextRangeBase::GetBoundingRectangles(_Outptr_result_maybenull_
         // _end is exclusive, let's be inclusive so we don't have to think about it anymore for bounding rects
         bufferSize.DecrementInBounds(endAnchor, true);
 
-        if (IsDegenerate() || _start > viewportEnd || _end < viewportOrigin)
+        if (IsDegenerate() || bufferSize.CompareInBounds(_start, viewportEnd, true) > 0 || bufferSize.CompareInBounds(_end, viewportOrigin, true) < 0)
         {
             // An empty array is returned for a degenerate (empty) text range.
             // reference: https://docs.microsoft.com/en-us/windows/win32/api/uiautomationclient/nf-uiautomationclient-iuiautomationtextrange-getboundingrectangles
@@ -997,7 +997,7 @@ std::wstring UiaTextRangeBase::_getTextValue(til::CoordType maxLength) const
 
         // TODO GH#5406: create a different UIA parent object for each TextBuffer
         // nvaccess/nvda#11428: Ensure our endpoints are in bounds
-        THROW_HR_IF(E_FAIL, !bufferSize.IsInBounds(_start) || !bufferSize.IsInBounds(_end));
+        THROW_HR_IF(E_FAIL, !bufferSize.IsInBounds(_start, true) || !bufferSize.IsInBounds(_end, true));
 
         // convert _end to be inclusive
         auto inclusiveEnd = _end;
@@ -1052,11 +1052,11 @@ try
     constexpr auto endpoint = TextPatternRangeEndpoint::TextPatternRangeEndpoint_Start;
     const auto bufferSize{ _pData->GetTextBuffer().GetSize() };
     const auto documentEnd = _getDocumentEnd();
-    if (_start > documentEnd)
+    if (bufferSize.CompareInBounds(_start, documentEnd, true) > 0)
     {
         _start = documentEnd;
     }
-    if (_end > documentEnd)
+    if (bufferSize.CompareInBounds(_end, documentEnd, true) > 0)
     {
         _end = documentEnd;
     }
@@ -1126,11 +1126,11 @@ IFACEMETHODIMP UiaTextRangeBase::MoveEndpointByUnit(_In_ TextPatternRangeEndpoin
     }
     CATCH_LOG();
 
-    if (_start > documentEnd)
+    if (bufferSize.CompareInBounds(_start, documentEnd, true) > 0)
     {
         _start = documentEnd;
     }
-    if (_end > documentEnd)
+    if (bufferSize.CompareInBounds(_end, documentEnd, true) > 0)
     {
         _end = documentEnd;
     }
@@ -1180,7 +1180,7 @@ try
     const auto bufferSize = _pData->GetTextBuffer().GetSize();
     const auto mine = GetEndpoint(endpoint);
     const auto other = range->GetEndpoint(targetEndpoint);
-    RETURN_HR_IF(E_FAIL, !bufferSize.IsInBounds(mine) || !bufferSize.IsInBounds(other));
+    RETURN_HR_IF(E_FAIL, !bufferSize.IsInBounds(mine, true) || !bufferSize.IsInBounds(other, true));
 
     SetEndpoint(endpoint, range->GetEndpoint(targetEndpoint));
 
@@ -1206,7 +1206,10 @@ try
     else
     {
         const auto bufferSize = _pData->GetTextBuffer().GetSize();
-        RETURN_HR_IF(E_FAIL, !bufferSize.IsInBounds(_start) || !bufferSize.IsInBounds(_end));
+        if (!bufferSize.IsInBounds(_start, true) || !bufferSize.IsInBounds(_end, true))
+        {
+            return E_FAIL;
+        }
         auto inclusiveEnd = _end;
         bufferSize.DecrementInBounds(inclusiveEnd);
         _pData->SelectNewRegion(_start, inclusiveEnd);
@@ -1520,7 +1523,7 @@ void UiaTextRangeBase::_moveEndpointByUnitWord(_In_ const int moveCount,
         {
         case MovementDirection::Forward:
         {
-            if (nextPos >= documentEnd)
+            if (bufferSize.CompareInBounds(nextPos, documentEnd, true) >= 0)
             {
                 success = false;
             }
@@ -1741,7 +1744,7 @@ void UiaTextRangeBase::_moveEndpointByUnitDocument(_In_ const int moveCount,
         }
         CATCH_LOG();
 
-        if (preventBoundary || target >= documentEnd)
+        if (preventBoundary || bufferSize.CompareInBounds(target, documentEnd, true) >= 0)
         {
             return;
         }
diff --git a/src/types/inc/viewport.hpp b/src/types/inc/viewport.hpp
index 0772d8fe8cb..d762f64348f 100644
--- a/src/types/inc/viewport.hpp
+++ b/src/types/inc/viewport.hpp
@@ -55,7 +55,7 @@ namespace Microsoft::Console::Types
         til::size Dimensions() const noexcept;
 
         bool IsInBounds(const Viewport& other) const noexcept;
-        bool IsInBounds(const til::point pos) const noexcept;
+        bool IsInBounds(const til::point pos, bool allowEndExclusive = false) const noexcept;
 
         void Clamp(til::point& pos) const;
         Viewport Clamp(const Viewport& other) const noexcept;
@@ -65,7 +65,7 @@ namespace Microsoft::Console::Types
         bool IncrementInBoundsCircular(til::point& pos) const noexcept;
         bool DecrementInBounds(til::point& pos, bool allowEndExclusive = false) const noexcept;
         bool DecrementInBoundsCircular(til::point& pos) const noexcept;
-        int CompareInBounds(const til::point first, const til::point second) const noexcept;
+        int CompareInBounds(const til::point first, const til::point second, bool allowEndExclusive = false) const noexcept;
 
         enum class XWalk
         {
diff --git a/src/types/viewport.cpp b/src/types/viewport.cpp
index f0eb50cf33e..15dc97a0a31 100644
--- a/src/types/viewport.cpp
+++ b/src/types/viewport.cpp
@@ -203,10 +203,18 @@ bool Viewport::IsInBounds(const Viewport& other) const noexcept
 // - Determines if the given coordinate position lies within this viewport.
 // Arguments:
 // - pos - Coordinate position
+// - allowEndExclusive - if true, allow the EndExclusive til::point as a valid position.
+//                        Used in accessibility to signify that the exclusive end
+//                        includes the last til::point in a given viewport.
 // Return Value:
 // - True if it lies inside the viewport. False otherwise.
-bool Viewport::IsInBounds(const til::point pos) const noexcept
+bool Viewport::IsInBounds(const til::point pos, bool allowEndExclusive) const noexcept
 {
+    if (allowEndExclusive && pos == EndExclusive())
+    {
+        return true;
+    }
+
     return pos.X >= Left() && pos.X < RightExclusive() &&
            pos.Y >= Top() && pos.Y < BottomExclusive();
 }
@@ -347,6 +355,9 @@ bool Viewport::DecrementInBoundsCircular(til::point& pos) const noexcept
 // Arguments:
 // - first- The first coordinate position
 // - second - The second coordinate position
+// - allowEndExclusive - if true, allow the EndExclusive til::point as a valid position.
+//                        Used in accessibility to signify that the exclusive end
+//                        includes the last til::point in a given viewport.
 // Return Value:
 // -  Negative if First is to the left of the Second.
 // -  0 if First and Second are the same coordinate.
@@ -354,11 +365,12 @@ bool Viewport::DecrementInBoundsCircular(til::point& pos) const noexcept
 // -  This is so you can do s_CompareCoords(first, second) <= 0 for "first is left or the same as second".
 //    (the < looks like a left arrow :D)
 // -  The magnitude of the result is the distance between the two coordinates when typing characters into the buffer (left to right, top to bottom)
-int Viewport::CompareInBounds(const til::point first, const til::point second) const noexcept
+#pragma warning(suppress : 4100)
+int Viewport::CompareInBounds(const til::point first, const til::point second, bool allowEndExclusive) const noexcept
 {
     // Assert that our coordinates are within the expected boundaries
-    assert(IsInBounds(first));
-    assert(IsInBounds(second));
+    assert(IsInBounds(first, allowEndExclusive));
+    assert(IsInBounds(second, allowEndExclusive));
 
     // First set the distance vertically
     //   If first is on row 4 and second is on row 6, first will be -2 rows behind second * an 80 character row would be -160.
@@ -421,7 +433,7 @@ bool Viewport::WalkInBounds(til::point& pos, const WalkDir dir, bool allowEndExc
 bool Viewport::WalkInBoundsCircular(til::point& pos, const WalkDir dir, bool allowEndExclusive) const noexcept
 {
     // Assert that the position given fits inside this viewport.
-    assert((allowEndExclusive && pos == EndExclusive()) || IsInBounds(pos));
+    assert(IsInBounds(pos, allowEndExclusive));
 
     if (dir.x == XWalk::LeftToRight)
     {


Test patch: