Terminal crashes when starting nano in bash when screen reader is enabled #13183

LeonarddeR · 2022-05-26T07:42:35Z

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

Start wsl from the terminal
Type nano test

Expected Behavior

Nano opens as expected

Actual Behavior

WT crashes

The text was updated successfully, but these errors were encountered:

LeonarddeR · 2022-05-26T07:43:01Z

Cc @codeofdusk, can you reproduce this?

codeofdusk · 2022-05-26T07:45:05Z

I wonder if #12561 is related?

CC @carlos-zamora, @zadjii-msft.

j4james · 2022-05-26T09:29:05Z

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.

j4james · 2022-05-26T13:45:56Z

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:

terminal/src/cascadia/TerminalCore/Terminal.cpp

Lines 986 to 989 in bf41a90

    
           int Terminal::ViewEndIndex() const noexcept 
        
           { 
        
               return _inAltBuffer() ? _altBufferSize.height : _mutableViewport.BottomInclusive(); 
        
           }

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.

zadjii-msft · 2022-05-26T17:20:27Z

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!

zadjii-msft · 2022-05-26T17:43:50Z

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

@j4james

## Summary of the Pull Request This fixes the crashes caused by using a screen reader when in an app that uses the alt buffer via two changes: 1. Fix `Terminal::ViewEndIndex()` - `UiaTextRangeBase` receives a coordinate that is outside of the bounds of the text buffer via the following chain of functions... `_getDocumentEnd()` --> `GetLastNonSpaceCharacter()` --> `_getOptimizedBufferSize()` --> `GetTextBufferEndPoisition()` --> `ViewEndIndex()` - Since support for the alt buffer was added recently, `ViewEndIndex()` was recently changed, so that explains why this issue came up recently. We were accidentally setting the view end index to `height` instead of `height-1`. Thanks @j4james for finding this! - The UIA code would get the "exclusive end" of the alt buffer. Since it was using `ViewEndIndex()` to calculate that, it was one more than it should be. The UIA code has explicit allowance for "one past the end of the viewport" in its `IsInBounds()` check. Since the `ViewEndIndex()` is way beyond that, it's not allowed, hitting the fail fast. 2. Replace `FAIL_FAST_IF` with `assert` - These fail fast calls have caused so many issues with our UIA code. Those checks still provide value, but they shouldn't take the whole app down. This change replaces the `Viewport` and `UiaTextRangeBase` fail fasts with asserts to still perform those checks, but not take down the entire app in release builds. Closes #13183 ## Validation Steps Performed While using Narrator... - opened nano in bash - generated text and scrolled in nano - generated text and scrolled in PowerShell

@j4james

This fixes the crashes caused by using a screen reader when in an app that uses the alt buffer via two changes: 1. Fix `Terminal::ViewEndIndex()` - `UiaTextRangeBase` receives a coordinate that is outside of the bounds of the text buffer via the following chain of functions... `_getDocumentEnd()` --> `GetLastNonSpaceCharacter()` --> `_getOptimizedBufferSize()` --> `GetTextBufferEndPoisition()` --> `ViewEndIndex()` - Since support for the alt buffer was added recently, `ViewEndIndex()` was recently changed, so that explains why this issue came up recently. We were accidentally setting the view end index to `height` instead of `height-1`. Thanks @j4james for finding this! - The UIA code would get the "exclusive end" of the alt buffer. Since it was using `ViewEndIndex()` to calculate that, it was one more than it should be. The UIA code has explicit allowance for "one past the end of the viewport" in its `IsInBounds()` check. Since the `ViewEndIndex()` is way beyond that, it's not allowed, hitting the fail fast. 2. Replace `FAIL_FAST_IF` with `assert` - These fail fast calls have caused so many issues with our UIA code. Those checks still provide value, but they shouldn't take the whole app down. This change replaces the `Viewport` and `UiaTextRangeBase` fail fasts with asserts to still perform those checks, but not take down the entire app in release builds. Closes #13183 While using Narrator... - opened nano in bash - generated text and scrolled in nano - generated text and scrolled in PowerShell (cherry picked from commit 94e1697) Service-Card-Id: 82972865 Service-Version: 1.14

## Summary of the Pull Request Replaces most uses of `Viewport::CompareInBounds()` with `til::point`'s `<` and `>` operators. `CompareInBounds` has been the cause of a bunch of UIA crashes over the years. Replacing them entirely ensures that the `FAILFAST_IF` isn't ever touched. Unfortunately, we still need `IncrementInBounds` and `DecrementInBounds` to have support for that exclusive end. ## References #13183

ghost · 2022-07-06T18:06:19Z

🎉This issue was addressed in #13250, which has now been successfully released as Windows Terminal v1.14.186.:tada:

Handy links:

ghost · 2022-07-06T18:07:18Z

🎉This issue was addressed in #13250, which has now been successfully released as Windows Terminal Preview v1.15.186.:tada:

Handy links:

LeonarddeR added the Issue-Bug It either shouldn't be doing this or needs an investigation. label May 26, 2022

ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels May 26, 2022

ghost removed the Needs-Tag-Fix Doesn't match tag requirements label May 26, 2022

zadjii-msft added this to the Terminal v1.14 milestone May 26, 2022

zadjii-msft added Needs-Tag-Fix Doesn't match tag requirements Needs-Discussion Something that requires a team discussion before we can proceed labels May 26, 2022

ghost removed the Needs-Tag-Fix Doesn't match tag requirements label May 26, 2022

zadjii-msft removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Discussion Something that requires a team discussion before we can proceed labels Jun 6, 2022

zadjii-msft modified the milestones: Terminal v1.14, Terminal v1.15 Jun 6, 2022

DHowett assigned carlos-zamora Jun 7, 2022

This was referenced Jun 7, 2022

Remove most uses of CompareInBounds #13244

Merged

Fix a11y crash in alt buffer apps #13250

Merged

ghost added the In-PR This issue has a related PR label Jun 8, 2022

ghost closed this as completed in #13250 Jun 10, 2022

ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Jun 10, 2022

zadjii-msft mentioned this issue Jun 14, 2022

"Less" pager on Linux closes Windows Terminal #13283

Closed

carlos-zamora mentioned this issue Jun 16, 2022

[1.14] Remove most uses of CompareInBounds #13313

Closed

zadjii-msft mentioned this issue Sep 2, 2022

Revert "Remove most uses of CompareInBounds (#13244)" #13907

Merged

This issue was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Terminal crashes when starting nano in bash when screen reader is enabled #13183

Terminal crashes when starting nano in bash when screen reader is enabled #13183

LeonarddeR commented May 26, 2022

LeonarddeR commented May 26, 2022

codeofdusk commented May 26, 2022

j4james commented May 26, 2022

j4james commented May 26, 2022

zadjii-msft commented May 26, 2022

zadjii-msft commented May 26, 2022

ghost commented Jul 6, 2022

ghost commented Jul 6, 2022

Terminal crashes when starting nano in bash when screen reader is enabled #13183

Terminal crashes when starting nano in bash when screen reader is enabled #13183

Comments

LeonarddeR commented May 26, 2022

Windows Terminal version

Windows build number

Other Software

Steps to reproduce

Expected Behavior

Actual Behavior

LeonarddeR commented May 26, 2022

codeofdusk commented May 26, 2022

j4james commented May 26, 2022

j4james commented May 26, 2022

zadjii-msft commented May 26, 2022

zadjii-msft commented May 26, 2022

ghost commented Jul 6, 2022

ghost commented Jul 6, 2022