Fix a11y crash in alt buffer apps #13250

carlos-zamora · 2022-06-08T17:21:38Z

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:

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.
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

zadjii-msft

Sure. When I was originally testing this, I found another crash after "moving the cursor down some rows" (presumably, in vim).

Did we ever figure out why that caused the out of bounds? or are we just leaning on the FAIL_FAST -> assert to silently ignore?

carlos-zamora · 2022-06-08T18:36:24Z

Sure. When I was originally testing this, I found another crash after "moving the cursor down some rows" (presumably, in vim).
Just tested it in vim as well, and I can't repro the crash. Not sure how you got it to crash in the past :/.

Did we ever figure out why that caused the out of bounds? or are we just leaning on the FAIL_FAST -> assert to silently ignore?

oh geez, I meant to include that in the PR body. Adding it now. But here's what I'm adding for quick reference:

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.

Also relevant, this is really just a backport-able solution for 1.14. The better solution is #13244, but since Leonard's sweeping til::point PR isn't in 1.14, we need to make it not crash. #13244 straight up removes uses of CompareInBounds which is responsible for most of our UIA-related crashes. Leveraging til::point's < and > gives us the same benefit but none of the crashing!

DHowett · 2022-06-08T21:27:30Z

src/types/UiaTextRangeBase.cpp

-    FAIL_FAST_IF(!(newViewport.Top >= topRow));
-    FAIL_FAST_IF(!(newViewport.Bottom <= bottomRow));
-    FAIL_FAST_IF(!(_getViewportHeight(oldViewport) == _getViewportHeight(newViewport)));
+    assert(newViewport.Top >= topRow);


@lhecker, can you suggest something better than just assert? If there is anything better. Otherwise, let's just do this.

The thread I dug up from like 3 years ago had Michael suggesting telemetry asserts. Unfortunately, that requires a decent amount of work to set up.

The only alternative to telemetry asserts that come to my mind is WI_ASSERT.

DHowett · 2022-06-08T21:27:55Z

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();


@zadjii-msft are there any alt buffer conformance scenarios you'd like us to check before committing to this change?

This I checked myself before I left last week, and seemed like it was just a typo tbh. Like, there's no reason to not have the -1

DHowett · 2022-06-08T21:28:32Z

src/types/viewport.cpp

@@ -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));


What happens down the line when UIA tries to use this new coordinate that is provably out of bounds? What happens when we Walk it in bounds circular?

I'm honestly stumped here. We shouldn't get to that point, but history shows that's never the case haha.

What if I clamp it when we get something out of bounds? Specifically, something like this...

til::point MyClamp(til::point pos, Viewport size) { if (pos < size.Origin()) return size.Origin(); // clamp to origin else if (pos > size.ExclusiveEnd()) return size.ExclusiveEnd(); // clamp to exclusive end else if (pos.x > size.RightInclusive()) return {size.Left(), pos.y+1}; // too far right --> assume we wrapped to next line else if (pos.x < size.Left()) return {size.RightInclusive(), pos.y-1};// too far left --> assume we wrapped to prev line }

This would ensure we're always in a valid space. Even if we kept the FailFast calls, we would never hit them because now we'd be clamped into a valid position.

The main sources of these issues I can think of are switching to/from the alt buffer, because we can literally switch to a buffer with a different size entirely. Clamping would still return an incorrect answer but (1) we 100% wouldn't crash and (2) it would be a stepping stone until #5406 get done.

Thoughts?

Hmmmmmmm.

I guess the problem with MyClamp is that if we knew when to use it, we wouldn't have had this problem in the first place. Right?

I personally would say that we should use fail-fasts or exceptions for security or safety relevant checks (e.g. boundary checks for array access). For anything else I'd use debug assertions + clamping/truncation/etc. in release on a "lazy" best-effort basis.

The suggested code above would be overkill already for that purpose IMO. It's not just stopping incorrect behavior, but also tries to correct the position in a smart manner. But how do we know that this improves the behavior? It shouldn't pass incorrect positions in the first place after all!

As such I'd just use std::clamp 4 times for numeric parameters like that.
(I don't think this is necessary for this PR however. Or is it?)

DHowett · 2022-06-08T21:28:37Z

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)


It's dumb. allowEndExclusive is now a param that's not referenced anywhere... but like, it is, it's just hidden behind an assert.

ghost · 2022-06-10T18:07:41Z

Hello @zadjii-msft!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

DHowett

FWIW we automerged this without addressing exactly what happens when an out of bounds coordinate escapes through the assert since it's not built in Release

@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

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

🎉Windows Terminal v1.14.186 has been released which incorporates this pull request.:tada:

Handy links:

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

🎉Windows Terminal Preview v1.15.186 has been released which incorporates this pull request.:tada:

Handy links:

Fix a11y crash in alt buffer apps

1141ec3

carlos-zamora requested a review from zadjii-msft June 8, 2022 17:21

zadjii-msft approved these changes Jun 8, 2022

View reviewed changes

bye bye audit mode

8f87158

carlos-zamora added the Needs-Second It's a PR that needs another sign-off label Jun 8, 2022

ghost requested review from PankajBhojwani, DHowett and lhecker June 8, 2022 21:08

DHowett reviewed Jun 8, 2022

View reviewed changes

lhecker approved these changes Jun 9, 2022

View reviewed changes

zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jun 10, 2022

ghost merged commit 94e1697 into main Jun 10, 2022

ghost deleted the dev/cazamor/a11y/bugfix-altbuffercrash branch June 10, 2022 18:09

DHowett reviewed Jun 10, 2022

View reviewed changes

ghost removed the Needs-Second It's a PR that needs another sign-off label Jun 10, 2022

zadjii-msft mentioned this pull request Jun 14, 2022

"Less" pager on Linux closes Windows Terminal #13283

Closed

ghost mentioned this pull request Jul 6, 2022

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

Closed

zadjii-msft mentioned this pull request Sep 2, 2022

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

Merged

carlos-zamora mentioned this pull request Oct 27, 2022

Audit FAIL_FASTs across the code #14298

Open

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix a11y crash in alt buffer apps #13250

Fix a11y crash in alt buffer apps #13250

carlos-zamora commented Jun 8, 2022 •

edited

Loading

zadjii-msft left a comment

carlos-zamora commented Jun 8, 2022

DHowett Jun 8, 2022

carlos-zamora Jun 8, 2022

lhecker Jun 9, 2022

DHowett Jun 8, 2022

zadjii-msft Jun 9, 2022

DHowett Jun 8, 2022

carlos-zamora Jun 9, 2022

DHowett Jun 9, 2022 •

edited

Loading

lhecker Jun 9, 2022 •

edited

Loading

DHowett Jun 8, 2022

carlos-zamora Jun 8, 2022

ghost commented Jun 10, 2022

DHowett left a comment •

edited

Loading

ghost commented Jul 6, 2022

ghost commented Jul 6, 2022

Fix a11y crash in alt buffer apps #13250

Fix a11y crash in alt buffer apps #13250

Conversation

carlos-zamora commented Jun 8, 2022 • edited Loading

Summary of the Pull Request

Validation Steps Performed

zadjii-msft left a comment

Choose a reason for hiding this comment

carlos-zamora commented Jun 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett Jun 9, 2022 • edited Loading

Choose a reason for hiding this comment

lhecker Jun 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jun 10, 2022

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

DHowett left a comment • edited Loading

Choose a reason for hiding this comment

ghost commented Jul 6, 2022

ghost commented Jul 6, 2022

carlos-zamora commented Jun 8, 2022 •

edited

Loading

DHowett Jun 9, 2022 •

edited

Loading

lhecker Jun 9, 2022 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

DHowett left a comment •

edited

Loading