Repository: microsoft/terminal (language: cpp)

Issue:
Terminal crashes when starting nano in bash when screen reader is enabled
### Windows Terminal version

1.14.1433.0 preview

### Windows build number

22621.1

### Other Software

* Nano in either WSL or ssh
* Narrator or NVDA 2022.1

### Steps to reproduce

1. Start wsl from the terminal
2. Type `nano test`

### Expected Behavior

Nano opens as expected

### Actual Behavior

WT crashes


Discussion / hints:
Cc @codeofdusk, can you reproduce this?
I wonder if #12561 is related?

CC @carlos-zamora, @zadjii-msft.
FYI, I can reproduce this in vim too. It's another example of a `FAIL_FAST` crash in `Viewport::CompareInBounds` - same as #8730. I know we need to track down the source of the incorrect coordinates as well, but can we please, please, please get rid of those `FAIL_FAST` calls.
As far as I can see, the value that's out of range is coming from the `_getDocumentEnd` call, and the reason that's out of range is because `GetLastNonSpaceCharacter` is wrong. The reason that is wrong, is because the viewport passed to it from `_getOptimizedBufferSize` is wrong. And the reason that is wrong, is because `GetTextBufferEndPosition` is wrong. `GetTextBufferEndPosition` gets its Y coordinate from `ViewEndIndex`, and when we're in the alt buffer, that returns the buffer height, when it should be the height minus one (I believe it's meant to be the index of the last line).

In short, we need to change the value in `Terminal::ViewEndIndex` to `_altBufferSize.height-1`. See here:
https://github.com/microsoft/terminal/blob/bf41a90ad8cb596c4adae7565b0e6bcf917fc34e/src/cascadia/TerminalCore/Terminal.cpp#L986-L989

That said, the original alt buffer implementation (PR #12561) actually had this correct. It was in PR #12719 that it was essentially changed from `height-1` to `height`. If that was an intentional change, the fix may be more complicated that it first appears.
> If that was an intentional change, the fix may be more complicated that it first appears.

I really doubt it was. This one's on me I suppose. Thanks for the initial investigation! 
note before lunch: after replacing that with a `height-1`, there's still a crash after moving the cursor down some rows.


```
 	Microsoft.Terminal.Control.dll!wil::details::in1diag5::FailFast_If(void * callerReturnAddress, unsigned int lineNumber, const char * fileName, const char * functionName, const char * code, bool condition) Line 5435	C++
 	Microsoft.Terminal.Control.dll!Microsoft::Console::Types::Viewport::CompareInBounds(const _COORD & first, const _COORD & second, bool allowEndExclusive) Line 369	C++
>	Microsoft.Terminal.Control.dll!Microsoft::Console::Types::UiaTextRangeBase::_expandToEnclosingUnit(TextUnit unit) Line 296	C++
 	Microsoft.Terminal.Control.dll!Microsoft::Console::Types::UiaTextRangeBase::ExpandToEnclosingUnit(TextUnit unit) Line 273	C++
 	Microsoft.Terminal.Control.dll!winrt::Microsoft::Terminal::Control::implementation::XamlUiaTextRange::ExpandToEnclosingUnit(winrt::Windows::UI::Xaml::Automation::Text::TextUnit unit) Line 67	C++
```

  | Name | Value | Type
-- | -- | -- | --
▶ | _start | {56,167} | _COORD
▶ | bufferSize | {LT(0, 0) RB(79, 22) [80 x 23]} | Microsoft::Console::Types::Viewport
▶ | documentEnd | {x=0 X=0 y=23 ...} | til::point


Fix patch:
diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp
index bc4b01e608d..927abeef1bb 100644
--- a/src/cascadia/TerminalCore/Terminal.cpp
+++ b/src/cascadia/TerminalCore/Terminal.cpp
@@ -992,7 +992,7 @@ int Terminal::ViewStartIndex() const noexcept
 
 int Terminal::ViewEndIndex() const noexcept
 {
-    return _inAltBuffer() ? _altBufferSize.height : _mutableViewport.BottomInclusive();
+    return _inAltBuffer() ? _altBufferSize.height - 1 : _mutableViewport.BottomInclusive();
 }
 
 // _VisibleStartIndex is the first visible line of the buffer
diff --git a/src/types/UiaTextRangeBase.cpp b/src/types/UiaTextRangeBase.cpp
index 1202b7a34da..70654057a7d 100644
--- a/src/types/UiaTextRangeBase.cpp
+++ b/src/types/UiaTextRangeBase.cpp
@@ -1289,9 +1289,9 @@ try
         }
     }
 
-    FAIL_FAST_IF(!(newViewport.Top >= topRow));
-    FAIL_FAST_IF(!(newViewport.Bottom <= bottomRow));
-    FAIL_FAST_IF(!(_getViewportHeight(oldViewport) == _getViewportHeight(newViewport)));
+    assert(newViewport.Top >= topRow);
+    assert(newViewport.Bottom <= bottomRow);
+    assert(_getViewportHeight(oldViewport) == _getViewportHeight(newViewport));
 
     Unlock.reset();
 
@@ -1338,7 +1338,7 @@ til::size UiaTextRangeBase::_getScreenFontSize() const noexcept
 // - The viewport height
 til::CoordType UiaTextRangeBase::_getViewportHeight(const til::inclusive_rect& viewport) const noexcept
 {
-    FAIL_FAST_IF(!(viewport.Bottom >= viewport.Top));
+    assert(viewport.Bottom >= viewport.Top);
     // + 1 because til::inclusive_rect is inclusive on both sides so subtracting top
     // and bottom gets rid of 1 more then it should.
     return viewport.Bottom - viewport.Top + 1;
diff --git a/src/types/viewport.cpp b/src/types/viewport.cpp
index 45e1728887b..15dc97a0a31 100644
--- a/src/types/viewport.cpp
+++ b/src/types/viewport.cpp
@@ -365,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)
+#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
-    FAIL_FAST_IF(!IsInBounds(first, allowEndExclusive));
-    FAIL_FAST_IF(!IsInBounds(second, allowEndExclusive));
+    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.
@@ -432,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.
-    FAIL_FAST_IF(!IsInBounds(pos, allowEndExclusive));
+    assert(IsInBounds(pos, allowEndExclusive));
 
     if (dir.x == XWalk::LeftToRight)
     {


Test patch: