Fix double width cursor for CJK characters

[skyline75489]

Summary of the Pull Request

Fix double width cursor for CJK characters

References

#2713

PR Checklist

• Closes Feature Request - filledBox cursor should cover the whole unicode character #2713
• CLA signed. If not, go over here and sign the CLA
• Tests added/passed
• Requires documentation to be updated
• I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan.

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

[DHowett-MSFT]

This looks right to me. Thanks for tracking this down!

[skyline75489]

This seems to be causing a use-after-free in TextBufferCellIterator constructor. I'm trying to find out why but I'm really quite new to the codebase. Sorry for any inconvience. Feel free to revert this in case it stops you guys from working.

[zadjii-msft]

Oh no, that's probably #2375. The use-after-free crash only comes when you close a tab first, right?

[skyline75489]

I think it's a worse one, the _attrIter was freed in

terminal/src/buffer/out/textBufferCellIterator.cpp

Line 48 in 429af0e

_attrIter += pos.X;

. The dereferncing will cause a crash.

[skyline75489]

I think I figured out the reason. Coincidently it relates to my other PR #2937 .

The ATTR_ROW::InsertAttrRuns method replaces _list with another vector, causing the original _list and all its iterator unavailable. This PR introduced IsCursorDoubleWidth is using an old iterator and trying to dereference it in race condition. A quick fix would be changing the last few lines in ATTR_ROW::InsertAttrRuns here:

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

[zadjii-msft]

@miniksa thoughts here? I know we've done a lot of optimization on InsertAttrRuns in the past, esp. w.r.t. std::swap, but I don't remember those specifics anymore. You probably remember better than I.

[skyline75489]

For what I've learned so far, #2937 is a must-have. It dramastically reduce the overhead of creating a vector and replace _list with it. With the help of #2937 and further optimization, the choice of using std::swap or plain old copy doesn't really matter any more.

[skyline75489]

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:

terminal/src/buffer/out/textBufferCellIterator.cpp

Line 46 in 429af0e

THROW_HR_IF(E_INVALIDARG, !limits.IsInBounds(pos));

I'm seeing pos.X is 209 and limit is 208, thus the failure.

I found a possible fix. It seems the cursor updating code here is the root cause:

terminal/src/cascadia/TerminalCore/Terminal.cpp

Line 398 in 429af0e

cursor.SetPosition(proposedCursorPosition);

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