-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix double width cursor for CJK characters #2932
Fix double width cursor for CJK characters #2932
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks right to me. Thanks for tracking this down!
This seems to be causing a use-after-free in |
Oh no, that's probably #2375. The use-after-free crash only comes when you close a tab first, right? |
I think it's a worse one, the
|
I think I figured out the reason. Coincidently it relates to my other PR #2937 . The - _list.swap(newRun);
+ _list.resize(newRun.size());
+ for (auto i = 0; i < _list.size(); i++)
+ {
+ _list[i] = newRun[i];
+ } I think there could be a better way to do this. It's easy to reproduce the crash. Just open Vim and quickly navigating up and down. |
@miniksa thoughts here? I know we've done a lot of optimization on |
This PR also exposed another problem. When the windows is maximized and I'm navigationg up and down in vim, an assert failure was raised here:
I'm seeing I found a possible fix. It seems the cursor updating code here is the root cause:
I added the border validation code like this: + if (proposedCursorPosition.X > bufferSize.Width() - 1)
+
+ proposedCursorPosition.X = bufferSize.Width() - 1;
+ }
+
// This section is essentially equivalent to `AdjustCursorPosition`
// Update Cursor Position
cursor.SetPosition(proposedCursorPosition); |
This reverts commit eafa884.
# Summary of the Pull Request This PR will allow the cursor to be double width when on top of a double width character. This required changing `IsCursorDoubleWidth` to check whether the glyph the cursor's on top of is double width. This code is exactly the same as the original PR that addressed this issue in #2932. That one got reverted at some point due to the crashes related to it, but due to a combination of Terminal having come further since that PR and other changes to address use-after-frees, some of the crashes may/may not be relevant now. The ones that seemed to be relevant/repro-able, I attempt to address in this PR. The `IsCursorDoubleWidth` check would fail during the `TextBuffer::Reflow` call inside of `Terminal::UserResize` occasionally, particularly when `newCursor.EndDeferDrawing()` is called. This is because when we tell the newCursor to `EndDefer`, the renderer will attempt to redraw the cursor. As part of this redraw, it'll ask if `IsCursorDoubleWidth`, and if the renderer managed to ask this before `UserResize` swapped out the old buffer with the new one from `Reflow`, the renderer will be asking the old buffer if its out-of-bounds cursor is double width. This was pretty easily repro'd using `cmatrix -u0` and resizing the window like a madman. As a solution, I've moved the Start/End DeferDrawing calls out of `Reflow` and into `UserResize`. This way, I can "clamp" the portion of the code where the newBuffer is getting created and reflowed and swapped into the Terminal buffer, and only allow the renderer to draw once the swap is done. This also means that ConHost's `ResizeWithReflow` needed to change slightly. In addition, I've added a WriteLock to `SetCursorOn`. It was mentioned as a fix for a crash in #2965 (although I can't repro), and I also figured it would be good to try to emulate where ConHost locks with regards to Cursor operations, and this seemed to be one that we were missing. ## PR Checklist * [x] Closes #2713 * [x] CLA signed * [x] Tests added/passed ## Validation Steps Performed Manual validation that the cursor is indeed chonky, added a test case to check that we are correctly saying that the cursor is double width (not too sure if I put it in the right place). Also open to other test case ideas and thoughts on what else I should be careful for since I am quite nervous about what other crashes might occur.
Summary of the Pull Request
Fix double width cursor for CJK characters
References
#2713
PR Checklist
Detailed Description of the Pull Request / Additional comments
The code was copied from https://github.com/microsoft/terminal/blob/master/src/host/renderData.cpp#L266
Validation Steps Performed