Repository: microsoft/terminal (language: cpp)

Issue:
Windows Terminal (Preview) crashes with wrapped text
<!-- 
🚨🚨🚨🚨🚨🚨🚨🚨🚨🚨

I ACKNOWLEDGE THE FOLLOWING BEFORE PROCEEDING:
1. If I delete this entire template and go my own path, the core team may close my issue without further explanation or engagement.
2. If I list multiple bugs/concerns in this one issue, the core team may close my issue without further explanation or engagement.
3. If I write an issue that has many duplicates, the core team may close my issue without further explanation or engagement (and without necessarily spending time to find the exact duplicate ID number).
4. If I leave the title incomplete when filing the issue, the core team may close my issue without further explanation or engagement.
5. If I file something completely blank in the body, the core team may close my issue without further explanation or engagement.

All good? Then proceed!
-->

<!--
This bug tracker is monitored by Windows Terminal development team and other technical folks.

**Important: When reporting BSODs or security issues, DO NOT attach memory dumps, logs, or traces to Github issues**.
Instead, send dumps/traces to secure@microsoft.com, referencing this GitHub issue.

If this is an application crash, please also provide a Feedback Hub submission link so we can find your diagnostic data on the backend. Use the category "Apps > Windows Terminal (Preview)" and choose "Share My Feedback" after submission to get the link.

Please use this form and describe your issue, concisely but precisely, with as much detail as possible.

-->

❗ N.B. This issue has also been reported via **Feedback Hub link** https://aka.ms/AAa8g4n (video/screenshots have been uploaded; posted 2 months ago, but still no resolution)

# Environment

```none
Windows build number: 10.0.19042.0
Windows Terminal version: 1.5.3242.0

Any other software?
Windows Terminal version: 1.4.3243.0
```

# Steps to reproduce

1. Open new Windows Terminal tab or window (using Windows Powershell in this case)
2. Paste a long text string WITHOUT pressing ENTER. 
    
    For example, this  string (H/T http://powershell-guru.com/): `Get-ChildItem -Path C:\Demo -Filter *.cs | Rename-Item -NewName {[System.IO.Path]::ChangeExtension($_.Name, ".kt")}`

    Any long string of ASCII characters should also reproduce issue: `XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX- `

    The text will soft wrap in a standard Windows Terminal configuration (12pt text)

3. Press backspace and begin deleting the text. 

# Expected behavior

The cursor should move up a line. Windows Terminal should not crash.

# Actual behavior

Windows Terminal crashes just before the cursor moves up a line, or in other words, just before the character where the text is wrapped.

_NOTE: This only appears to happen when there is one tab open._



Discussion / hints:
Updated title to make clear that this issue occurs with both the regular and preview releases of Windows Terminal.
Weird, I can't get this to repro, but maybe the feedback hub link will have more info in it. 
_Hmm_. Are you using any assistive technology software?

/cc @calos-zamora this is a crash at UiaTextRangeBase.cpp:422 - comparing the start anchor of a range with the viewport.

I wonder if a caller can hang onto an old text range from a buffer before it was resized and have it 'splode on them.
@DHowett Yes, I am. I'm using built-in Magnifier, configured via the Windows 10 _Ease of Access Settings_ (`%windir%\system32\magnify.exe`)

![image](https://user-images.githubusercontent.com/755/106136500-4f97c300-6161-11eb-8b60-aa494354dd65.png)
![image](https://user-images.githubusercontent.com/755/106137017-07c56b80-6162-11eb-8b9b-9465cbd31d08.png)
![image](https://user-images.githubusercontent.com/755/106137087-21ff4980-6162-11eb-85be-2ffabf1dc635.png)

I'm not sure if this information is pertinent, but I have scaling turned on: I typically use two displays - my primary, laptop screen and an external monitor. As a consequence of vision problems, I have found it necessary to set scaling to 125% on the built-in display.

![image](https://user-images.githubusercontent.com/755/106138237-c6ce5680-6163-11eb-866a-e02e5b14cde3.png)
![image](https://user-images.githubusercontent.com/755/106138562-33495580-6164-11eb-92b1-1b00ca0cf3fe.png)

I will try to see if resetting the _Ease if Access_ Settings to their defaults makes/turning off Magnifier affects the reproducibility of this crash.
Triaging this into v2 since we have a line of code identified. Thanks!
fwiw the original LOC:
https://github.com/microsoft/terminal/blob/d29d72e1e023491800b6a03bc226c35e5f9ec240/src/types/UiaTextRangeBase.cpp#L422 

is now:

https://github.com/microsoft/terminal/blob/68294f863d2adfafd6937324486b499fea73b8ab/src/types/UiaTextRangeBase.cpp#L849

which is in `UiaTextRangeBase::GetBoundingRectangles`
FYI, I could reproduce the crash in the debugger just by typing until I reached the right margin (obviously this is with the screen magnifier on). Seems to be the same thing as backspacing from a wrapped line. Either way, what's happening is the `startAnchor` X coordinate exactly matches the `RightExclusive` boundary of the `bufferSize` viewport, and thus is technically out of range. As a result, the `CompareInBounds` method triggers a `FAIL_FAST` exception.

https://github.com/microsoft/terminal/blob/fb597ed304ec6eef245405c9652e9b8a029b821f/src/types/viewport.cpp#L357-L359

As for why it's out of range, I'm guessing that might be a side effect of how the terminal handles delayed wrap in the last column.
Super interesting that using the magnifier tiickles this bug. Kind of unfortunate for low vision folks who need a little help :)

At least now I know why 'npm install -g gatsby-cli' was consistently blowing up Windows Terminal :)
For what it's worth anyone trying to use nodejs / npm on Windows is going to get bitten REALLY hard by this bug. It makes installing packages almost impossible.
Will also impact screen reader users. CC @carlos-zamora.
@carlos-zamora I'm deferring this one to you. Seems like we could easily just add a check to see if `startAnchor.x() >= bufferSize.RightExclusive()`, and clamp the coordinate in that case. But maybe `Viewport::IsInBounds(..., allowEndExclusive=true)` needs to be more forgiving for this scenario? IDK. 

It should definitely get a test though, and you're the UIA test man for sure.
Sorry I didn't look at this sooner! Somehow this never made it to the top of my inbox.

So here's what's going on:
1. Magnifier enables UIA
2. `ScreenInfoUiaProviderBase::GetSelection()` is called
   - This is called frequently to get the cursor position.
3. `UiaTextRangeBase::GetBoundingRectangles()` is called
   - This is called frequently to figure out where the cursor is on the screen and move magnifier's view appropriately.
4. `Viewport::CompareInBounds(<cursorPosition>, <topOfViewport>)` is called and crashes because `<cursorPosition>` is out of bounds from the buffer size (as mentioned above).

I'm hesitant to clamp the coordinate because I feel like the coordinate is wrong in the first place. Why is the cursor position `{120,8}` when the buffer is 120 cells wide? The cursor should be going from `{0,9}` to `{119,8}` when it wraps.

@miniksa @lhecker Any idea why the cursor's position would be off the right boundary? I imagine we're using the `OutputCellIterator` (or one of the iterators we have) to wrap the cursor properly, so that's the part that's confusing to me.
The invalid coordinates originate from here (see last line, `_AdjustCursorPosition`):
https://github.com/microsoft/terminal/blob/0a545b7b9446562561d0978b6c1664324a85ae48/src/cascadia/TerminalCore/Terminal.cpp#L954-L1002

If the current cursor position (`cursorPosBefore`) is at the end of the line - `{119,8}` for instance - then `proposedCursorPosition.X += gsl::narrow<SHORT>(cellDistance);` will increment that to be either `{120,8}` or `{121,8}`.

Now normally I'd just throw a quick
```cpp
if (const auto width = bufferSize.Width(); proposedCursorPosition.X >= width)
{
    proposedCursorPosition.X = 0;
    proposedCursorPosition.Y++;
}
```

into `_AdjustCursorPosition`, but this doesn't work here, because `TerminalDispatch` also modifies the cursor position even before the above code is executed. ConPTY emits carriage returns which set the cursor X position to 0, as well as explicit CSI CUP cursor position sequences.
...and `Terminal::_WriteBuffer` is oblivious about control codes and their meaning. The resulting interaction is a bit whacky, because if you press backspace on a newline it looks like that:
![image](https://user-images.githubusercontent.com/2256941/152272069-d25a9b82-fb63-4d1a-b005-e94ae5ae51f2.png)

So when you press backspace on a newline, what happens is that we receive a CUP sequence setting the cursor to the end of the previous line. And thus before the above code runs the cursor position will already have been modified from `{0,9}` to `{119,8}` (for instance). `_WriteBuffer` then doesn't see a backspace, but rather the actual buffer contents, which in this case is a whitespace (0x20). Since whitespace is regular text, the `cellDistance` is 1, which finally modifies the cursor position from `{119,8}` to `{120,8}`.

This behavior stands in direct conflict with the behavior when you enter regular text at the end of a line, because in that case we _don't _ receive a CUP sequence, nor carriage return in `TerminalDispatch`. Instead only `_WriteBuffer` is executed.
This means my short code snippet above will only work when entering text, but not when deleting text.

Optimally only a single place should modify cursor positions, because it's a bit confusing otherwise. But `TerminalDispatch` has no idea about unicode and `Terminal` has no idea about control codes.
@miniksa Suggestions?
The problem is just that the `Terminal::_WriteBuffer` method doesn't implement delayed EOL wrap. As a very simple test case, run `printf "\e[999C*"` with a breakpoint in the `_WriteBuffer` method. You'll see it write out the `*` character and then increment the X coordinate beyond the width of the page, passing that invalid value to `_AdjustCursorPosition`. That should never happen.

What it should be doing is recording the delayed wrap, and leaving the cursor where it is in the final column. You can see how it's handled in conhost here:

https://github.com/microsoft/terminal/blob/2b202ce6d993404010337fe8ff205d7b240d60f5/src/host/_stream.cpp#L573-L588

If more output is received after that, then the cursor will need to be moved to the start of the next line, _before_ processing the output. Again you can see how that's handled in conhost here:

https://github.com/microsoft/terminal/blob/2b202ce6d993404010337fe8ff205d7b240d60f5/src/host/_stream.cpp#L365-L376

The bottom line is that WT doesn't have a fully functional `WriteBuffer` implementation - I think that's what #780 was meant to resolve.
In the meantime, though, we should really get rid of those damn `FAIL_FAST` tests. Or anyone that is in favour of retaining them, should volunteer to have the telemetry from the crash logs wired up to their desk with a high-voltage shock.
FYI This impacts any tool that tracks the cursor using UIA. Such tools include the [text cursor indicator](https://www.tenforums.com/tutorials/137349-turn-off-text-cursor-indicator-windows-10-a.html), [magnifier](https://www.minitool.com/news/windows-magnifier.html), and screen readers.
> In the meantime, though, we should really get rid of those damn `FAIL_FAST` tests. Or anyone that is in favour of retaining them, should volunteer to have the telemetry from the crash logs wired up to their desk with a high-voltage shock.

It's my fault. If you want to shock my chair, you can. I realize, like many things, this was a mistake in hindsight.

Fix patch:
diff --git a/src/types/UiaTextRangeBase.cpp b/src/types/UiaTextRangeBase.cpp
index 6571d2ea13f..1658e6d2b16 100644
--- a/src/types/UiaTextRangeBase.cpp
+++ b/src/types/UiaTextRangeBase.cpp
@@ -42,9 +42,15 @@ HRESULT UiaTextRangeBase::RuntimeClassInitialize(_In_ IUiaData* pData,
                                                  _In_ std::wstring_view wordDelimiters) noexcept
 try
 {
+    RETURN_HR_IF_NULL(E_INVALIDARG, pData);
     RETURN_IF_FAILED(RuntimeClassInitialize(pData, pProvider, wordDelimiters));
 
+    // GH#8730: The cursor position may be in a delayed state, resulting in it being out of bounds.
+    // If that's the case, clamp it to be within bounds.
+    // TODO GH#12440: We should be able to just check some fields off of the Cursor object,
+    // but Windows Terminal isn't updating those flags properly.
     _start = cursor.GetPosition();
+    pData->GetTextBuffer().GetSize().Clamp(_start);
     _end = _start;
 
     UiaTracing::TextRange::Constructor(*this);


Test patch: