Fix DBCS attribute corruption during reflow #12853

lhecker · 2022-04-07T17:47:49Z

855e136 contains a regression which breaks buffer reflow if wide surrogate
characters are present. This happens because we made use of the
TextBufferCellIterator whose increment operator skips 2 cells for wide
characters. This created a "misalignment" in the reflow logic which was written
for cell-wise iteration. This commit fixes the issue, by reverting back to the
previous algorithm without iterators.

Closes #12837
Closes MSFT-38904421

Validation Steps Performed

Run pwsh -noprofile -command echo "`u{D83D}`u{DE43}"
Resizing conhost preserves all contents ✅
Resizing Windows Terminal doesn't crash it ✅
Added a test covering this issue ✅

src/buffer/out/textBuffer.cpp

DHowett

The newAttrColumn thing seems like it would regress the original bug we fixed.

DHowett · 2022-04-07T18:01:49Z

We will not erroneously copy a DBCS Trailing Attribute to the edge of the screen, right? Or a leading one?

lhecker · 2022-04-07T18:03:51Z

We will not erroneously copy a DBCS Trailing Attribute to the edge of the screen, right? Or a leading one?

I'll have to check! This PR could've been a draft tbh. 😅
I'll need a bit to write a unit test and test this more thoroughly.

zadjii-msft · 2022-04-07T19:02:58Z

src/buffer/out/textBuffer.cpp

        {
            try
            {
                // TODO: MSFT: 19446208 - this should just use an iterator and the inserter...
-                const auto textAttr = *attrs;
+                const auto textAttr = row.GetAttrRow().GetAttrByColumn(copyAttrCol);


sad that this beautiful optimization had to die

zadjii-msft · 2022-04-07T19:07:15Z

src/buffer/out/textBuffer.cpp

-                const auto dbcsAttr = chars->DbcsAttr();
-                const auto textAttr = *attrs;
+                const auto glyph = row.GetCharRow().GlyphAt(iOldCol);
+                const auto dbcsAttr = row.GetCharRow().DbcsAttrAt(iOldCol);


ah cause chars++ might skip two columns for something like an emoji, right?

Yeah I think so.

DHowett · 2022-04-07T20:15:18Z

So, here is my hope.

WinDev is in "ask" mode locally, so I'm just gonna take this fix up during Ask for both Terminal and Console.
We need this to be rock solid. I need tests for resizing single- and double- width surrogate pairs, at the edge of the buffer, wider and narrower.
I need to understand the impact on Terminal, and I think we should wrap UpdatePatternLocationsUnderLock in a try/catch just to be safe so that any future regressions here only break URL updating and don't cause a crash.
We're in a tight corner here. :)

src/buffer/out/OutputCellIterator.cpp

ghost · 2022-04-08T17:07:33Z

Hello @DHowett!

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.

855e136 contains a regression which breaks buffer reflow if wide surrogate characters are present. This happens because we made use of the `TextBufferCellIterator` whose increment operator skips 2 cells for wide characters. This created a "misalignment" in the reflow logic which was written for cell-wise iteration. This commit fixes the issue, by reverting back to the previous algorithm without iterators. Closes #12837 Closes MSFT-38904421 ## Validation Steps Performed * Run ``pwsh -noprofile -command echo "`u{D83D}`u{DE43}"`` * Resizing conhost preserves all contents ✅ * Resizing Windows Terminal doesn't crash it ✅ * Added a test covering this issue ✅ (cherry picked from commit 10b9044) Service-Card-Id: 80340089 Service-Version: 1.12

855e136 contains a regression which breaks buffer reflow if wide surrogate characters are present. This happens because we made use of the `TextBufferCellIterator` whose increment operator skips 2 cells for wide characters. This created a "misalignment" in the reflow logic which was written for cell-wise iteration. This commit fixes the issue, by reverting back to the previous algorithm without iterators. Closes #12837 Closes MSFT-38904421 ## Validation Steps Performed * Run ``pwsh -noprofile -command echo "`u{D83D}`u{DE43}"`` * Resizing conhost preserves all contents ✅ * Resizing Windows Terminal doesn't crash it ✅ * Added a test covering this issue ✅ (cherry picked from commit 10b9044) Service-Card-Id: 80340090 Service-Version: 1.13

ghost · 2022-04-19T19:56:38Z

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

Handy links:

ghost · 2022-04-19T19:58:53Z

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

Handy links:

855e136 contains a regression which breaks buffer reflow if wide surrogate characters are present. This happens because we made use of the `TextBufferCellIterator` whose increment operator skips 2 cells for wide characters. This created a "misalignment" in the reflow logic which was written for cell-wise iteration. This commit fixes the issue, by reverting back to the previous algorithm without iterators. Closes #12837 Closes MSFT-38904421 ## Validation Steps Performed * Run ``pwsh -noprofile -command echo "`u{D83D}`u{DE43}"`` * Resizing conhost preserves all contents ✅ * Resizing Windows Terminal doesn't crash it ✅ * Added a test covering this issue ✅ (cherry picked from commit 10b9044) Service-Card-Id: 80340091 Service-Version: Inbox

Fix DBCS attribute corruption during reflow

1468dd1

lhecker requested a review from zadjii-msft April 7, 2022 17:47

miniksa approved these changes Apr 7, 2022

View reviewed changes

DHowett reviewed Apr 7, 2022

View reviewed changes

src/buffer/out/textBuffer.cpp Show resolved Hide resolved

DHowett requested changes Apr 7, 2022

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 7, 2022

Address Dustin's feedback

8e9d723

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 7, 2022

zadjii-msft reviewed Apr 7, 2022

View reviewed changes

lhecker added this to To Cherry Pick in 1.12 Servicing Pipeline via automation Apr 7, 2022

lhecker added this to To Cherry Pick in Inbox Servicing Pipeline via automation Apr 7, 2022

lhecker added this to To Cherry Pick in 1.13 Servicing Pipeline via automation Apr 7, 2022

Added unit test coverage

cdefd8a

lhecker requested a review from DHowett April 8, 2022 16:55

DHowett reviewed Apr 8, 2022

View reviewed changes

src/buffer/out/OutputCellIterator.cpp Show resolved Hide resolved

DHowett approved these changes Apr 8, 2022

View reviewed changes

DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Apr 8, 2022

ghost merged commit 10b9044 into main Apr 8, 2022

ghost deleted the dev/lhecker/12837-reflow-fix branch April 8, 2022 17:26

DHowett moved this from To Cherry Pick to Cherry Picked in Inbox Servicing Pipeline Apr 8, 2022

DHowett moved this from To Cherry Pick to Cherry Picked in 1.12 Servicing Pipeline Apr 8, 2022

DHowett moved this from To Cherry Pick to Cherry Picked in 1.13 Servicing Pipeline Apr 8, 2022

ghost mentioned this pull request Apr 19, 2022

Terminal crash on high unicode chars: out of range exception reading UnicodeStorage during UpdatePatternLocations #12837

Closed

DHowett moved this from Cherry Picked to Validated in 1.13 Servicing Pipeline May 10, 2022

DHowett moved this from Validated to Shipped in 1.13 Servicing Pipeline May 10, 2022

DHowett moved this from Cherry Picked to Shipped in Inbox Servicing Pipeline Jun 7, 2022

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix DBCS attribute corruption during reflow #12853

Fix DBCS attribute corruption during reflow #12853

lhecker commented Apr 7, 2022 •

edited by DHowett

DHowett left a comment

DHowett commented Apr 7, 2022

lhecker commented Apr 7, 2022

zadjii-msft Apr 7, 2022

zadjii-msft Apr 7, 2022

lhecker Apr 7, 2022

DHowett commented Apr 7, 2022

ghost commented Apr 8, 2022

ghost commented Apr 19, 2022

ghost commented Apr 19, 2022

Fix DBCS attribute corruption during reflow #12853

Fix DBCS attribute corruption during reflow #12853

Conversation

lhecker commented Apr 7, 2022 • edited by DHowett

Validation Steps Performed

DHowett left a comment

Choose a reason for hiding this comment

DHowett commented Apr 7, 2022

lhecker commented Apr 7, 2022

zadjii-msft Apr 7, 2022

Choose a reason for hiding this comment

zadjii-msft Apr 7, 2022

Choose a reason for hiding this comment

lhecker Apr 7, 2022

Choose a reason for hiding this comment

DHowett commented Apr 7, 2022

ghost commented Apr 8, 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.

ghost commented Apr 19, 2022

ghost commented Apr 19, 2022

lhecker commented Apr 7, 2022 •

edited by DHowett

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.