Repository: microsoft/terminal (language: cpp)

Issue:
Crash in AtlasEngine::_emplaceGlyph
### Windows Terminal version

1.15.1862.0

### Windows build number

10.0.19044.1826

### Other Software

_No response_

### Steps to reproduce

1. Enable experimental text rendering engine in WT settings.
2. Rename the attached [Crash._emplaceGlyph.cmd.txt](https://github.com/microsoft/terminal/files/9126808/Crash._emplaceGlyph.cmd.txt) to `Crash._emplaceGlyph.cmd`.
3. Run WT.
4. Locate and execute `Crash._emplaceGlyph.cmd`.
5. Press any key when asked.

### Expected Behavior

No crash



### Actual Behavior

```
Unhandled exception at 0x00007FFE30AE286E (ucrtbase.dll) in WindowsTerminal.exe: Fatal program exit requested.
```

```
ucrtbase.dll!abort()
ucrtbase.dll!terminate()
[Inline Frame] Microsoft.Terminal.Control.dll!gsl::details::terminate() Line 116
Microsoft.Terminal.Control.dll!Microsoft::Console::Render::AtlasEngine::_emplaceGlyph(IDWriteFontFace * fontFace=0x00000221688e98c0, unsigned __int64 bufferPos1, unsigned __int64 bufferPos2) Line 1436
Microsoft.Terminal.Control.dll!Microsoft::Console::Render::AtlasEngine::_flushBufferLine() Line 1304
Microsoft.Terminal.Control.dll!Microsoft::Console::Render::AtlasEngine::PaintBufferLine(gsl::span<Microsoft::Console::Render::Cluster const ,-1> clusters={...}, til::point coord, bool fTrimLeft, bool lineWrapped=false) Line 461
Microsoft.Terminal.Control.dll!Microsoft::Console::Render::Renderer::_PaintBufferOutputHelper(Microsoft::Console::Render::IRenderEngine * const pEngine=0x000002215f689040, TextBufferCellIterator it={...}, const til::point target, const bool lineWrapped=false) Line 874
Microsoft.Terminal.Control.dll!Microsoft::Console::Render::Renderer::_PaintBufferOutput(Microsoft::Console::Render::IRenderEngine * const pEngine=0x000002215f689040) Line 718
Microsoft.Terminal.Control.dll!Microsoft::Console::Render::Renderer::_PaintFrameForEngine(Microsoft::Console::Render::IRenderEngine * const pEngine=0x000002215f689040) Line 160
Microsoft.Terminal.Control.dll!Microsoft::Console::Render::Renderer::PaintFrame() Line 78
Microsoft.Terminal.Control.dll!Microsoft::Console::Render::RenderThread::_ThreadProc() Line 214
kernel32.dll!BaseThreadInitThunk()
ntdll.dll!RtlUserThreadStart()
```


Discussion / hints:
This might have been fixed by the last couple PRs, but /cc @lhecker to confirm
Ah... the good old "overwriting wide chars with narrow ones f's up the text buffer" bug strikes yet again.

https://user-images.githubusercontent.com/2256941/179547827-cf8fe406-f577-4e42-a2a1-bcf47ffb403a.mp4

I've got a fix at hand.

Fix patch:
diff --git a/src/renderer/atlas/AtlasEngine.cpp b/src/renderer/atlas/AtlasEngine.cpp
index a5bd51b7eab..70e1e571041 100644
--- a/src/renderer/atlas/AtlasEngine.cpp
+++ b/src/renderer/atlas/AtlasEngine.cpp
@@ -1245,7 +1245,7 @@ void AtlasEngine::_flushBufferLine()
                 // Task: Replace all characters in this range with unicode replacement characters.
                 // Input (where "n" is a narrow and "ww" is a wide character):
                 //    _api.bufferLine       = "nwwnnw"
-                //    _api.bufferLineColumn = {0, 1, 1, 2, 3, 4, 4, 5}
+                //    _api.bufferLineColumn = {0, 1, 1, 3, 4, 5, 5, 6}
                 //                             n  w  w  n  n  w  w
                 // Solution:
                 //   Iterate through bufferLineColumn until the value changes, because this indicates we passed over a
@@ -1293,9 +1293,13 @@ void AtlasEngine::_flushBufferLine()
 
             if (isTextSimple)
             {
+                size_t beg = 0;
                 for (size_t i = 0; i < complexityLength; ++i)
                 {
-                    _emplaceGlyph(mappedFontFace.get(), idx + i, idx + i + 1u);
+                    if (_emplaceGlyph(mappedFontFace.get(), idx + beg, idx + i + 1))
+                    {
+                        beg = i + 1;
+                    }
                 }
             }
             else
@@ -1410,8 +1414,10 @@ void AtlasEngine::_flushBufferLine()
                     {
                         if (_api.textProps[i].canBreakShapingAfter)
                         {
-                            _emplaceGlyph(mappedFontFace.get(), a.textPosition + beg, a.textPosition + i + 1);
-                            beg = i + 1;
+                            if (_emplaceGlyph(mappedFontFace.get(), a.textPosition + beg, a.textPosition + i + 1))
+                            {
+                                beg = i + 1;
+                            }
                         }
                     }
                 }
@@ -1419,23 +1425,42 @@ void AtlasEngine::_flushBufferLine()
         }
     }
 }
+// ^^^ Look at that amazing 8-fold nesting level. Lovely. <3
 
-void AtlasEngine::_emplaceGlyph(IDWriteFontFace* fontFace, size_t bufferPos1, size_t bufferPos2)
+bool AtlasEngine::_emplaceGlyph(IDWriteFontFace* fontFace, size_t bufferPos1, size_t bufferPos2)
 {
     static constexpr auto replacement = L'\uFFFD';
 
     // This would seriously blow us up otherwise.
     Expects(bufferPos1 < bufferPos2 && bufferPos2 <= _api.bufferLine.size());
 
-    const auto chars = fontFace ? &_api.bufferLine[bufferPos1] : &replacement;
-    const auto charCount = fontFace ? bufferPos2 - bufferPos1 : 1;
-
     // _flushBufferLine() ensures that bufferLineColumn.size() > bufferLine.size().
     const auto x1 = _api.bufferLineColumn[bufferPos1];
     const auto x2 = _api.bufferLineColumn[bufferPos2];
 
-    Expects(x1 < x2 && x2 <= _api.cellCount.x);
+    // x1 == x2, if our TextBuffer and DirectWrite disagree where glyph boundaries are. Example:
+    // Our line of text contains a wide glyph consisting of 2 surrogate pairs "xx" and "yy".
+    // If DirectWrite considers the first "xx" to be separate from the second "yy", we'll get:
+    //   _api.bufferLine       = "...xxyy..."
+    //   _api.bufferLineColumn = {01233335678}
+    //                               ^ ^
+    //                              /   \
+    //                      bufferPos1  bufferPos2
+    //   x1: _api.bufferLineColumn[bufferPos1] == 3
+    //   x1: _api.bufferLineColumn[bufferPos2] == 3
+    // --> cellCount (which is x2 - x1) is now 0 (invalid).
+    //
+    // Assuming that the TextBuffer implementation doesn't have any bugs...
+    // I'm not entirely certain why this occurs, but to me, a layperson, it appears as if
+    // IDWriteFontFallback::MapCharacters() doesn't respect extended grapheme clusters.
+    // It could also possibly be due to a difference in the supported Unicode version.
+    if (x1 >= x2 || x2 > _api.cellCount.x)
+    {
+        return false;
+    }
 
+    const auto chars = fontFace ? &_api.bufferLine[bufferPos1] : &replacement;
+    const auto charCount = fontFace ? bufferPos2 - bufferPos1 : 1;
     const u16 cellCount = x2 - x1;
 
     auto attributes = _api.attributes;
@@ -1503,4 +1528,5 @@ void AtlasEngine::_emplaceGlyph(IDWriteFontFace* fontFace, size_t bufferPos1, si
     }
 
     std::fill_n(cellGlyphMappings, cellCount, it);
+    return true;
 }
diff --git a/src/renderer/atlas/AtlasEngine.h b/src/renderer/atlas/AtlasEngine.h
index d1da93c9bc7..ac0d2ece6a1 100644
--- a/src/renderer/atlas/AtlasEngine.h
+++ b/src/renderer/atlas/AtlasEngine.h
@@ -876,7 +876,7 @@ namespace Microsoft::Console::Render
         TileHashMap::iterator* _getCellGlyphMapping(u16 x, u16 y) noexcept;
         void _setCellFlags(u16r coords, CellFlags mask, CellFlags bits) noexcept;
         void _flushBufferLine();
-        void _emplaceGlyph(IDWriteFontFace* fontFace, size_t bufferPos1, size_t bufferPos2);
+        bool _emplaceGlyph(IDWriteFontFace* fontFace, size_t bufferPos1, size_t bufferPos2);
 
         // AtlasEngine.api.cpp
         void _resolveAntialiasingMode() noexcept;


Test patch: