don't crash an app if we got empty run during text shaping (#821)

* don't crash an app if we got empty run during text shaping * on windows and linux it may produce some glyph
JetBrains · Nov 8, 2023 · 5825d5c · 5825d5c
1 parent 748b004
commit 5825d5c
Show file tree

Hide file tree

Showing 3 changed files with 42 additions and 35 deletions.
diff --git a/skiko/src/commonMain/cpp/common/include/TextLineRunHandler.hh b/skiko/src/commonMain/cpp/common/include/TextLineRunHandler.hh
@@ -56,32 +56,34 @@ public:
 
     void commitRunBuffer(const RunInfo& info) override {
         TextLine::Run& run = fLine->fRuns.back();
-        int32_t glyph = 0;
-        int32_t graphemesInGlyph = 1;
-        SkScalar glyphLeft = run.fPos[glyph].fX;
-        run.fBreakOffsets.reserve(info.utf8Range.size() + 1);
-        run.fBreakPositions.reserve(info.utf8Range.size() + 1);
+        if (0 < info.glyphCount) {
+            int32_t glyph = 0;
+            int32_t graphemesInGlyph = 1;
+            SkScalar glyphLeft = run.fPos[glyph].fX;
+            run.fBreakOffsets.reserve(info.utf8Range.size() + 1);
+            run.fBreakPositions.reserve(info.utf8Range.size() + 1);
 
-        // Only record grapheme clusters boundaries
-        for (int32_t offset = fGlyphOffsets[0]; offset <= info.utf8Range.end(); offset = ubrk_following(fGraphemeIter.get(), offset)) {
-            run.fBreakOffsets.push_back(conv.from8To16(offset));
+            // Only record grapheme clusters boundaries
+            for (int32_t offset = fGlyphOffsets[0]; offset <= info.utf8Range.end(); offset = ubrk_following(fGraphemeIter.get(), offset)) {
+                run.fBreakOffsets.push_back(conv.from8To16(offset));
 
-            // if grapheme clusters includes multiple glyphs, skip over them
-            while (glyph < info.glyphCount && fGlyphOffsets[glyph] < offset)
-                ++glyph;
+                // if grapheme clusters includes multiple glyphs, skip over them
+                while (glyph < info.glyphCount && fGlyphOffsets[glyph] < offset)
+                    ++glyph;
 
-            // if one glyph includes multiple grapheme clusters (ligature, e.g. <->), accumulate
-            if ((glyph < info.glyphCount ? fGlyphOffsets[glyph] : info.utf8Range.end()) > offset)
-                ++graphemesInGlyph;
+                // if one glyph includes multiple grapheme clusters (ligature, e.g. <->), accumulate
+                if ((glyph < info.glyphCount ? fGlyphOffsets[glyph] : info.utf8Range.end()) > offset)
+                    ++graphemesInGlyph;
 
-            // when boundaries meet, distribute break positions evenly inside glyph
-            else {
-                SkScalar glyphRight = glyph < info.glyphCount ? run.fPos[glyph].fX : fPosition + info.fAdvance.fX;
-                SkScalar step = (glyphRight - glyphLeft) / graphemesInGlyph;
-                for (int i = 0; i < graphemesInGlyph; ++i)
-                    run.fBreakPositions.push_back(glyphLeft + step * (i + 1));
-                graphemesInGlyph = 1;
-                glyphLeft = glyphRight;
+                // when boundaries meet, distribute break positions evenly inside glyph
+                else {
+                    SkScalar glyphRight = glyph < info.glyphCount ? run.fPos[glyph].fX : fPosition + info.fAdvance.fX;
+                    SkScalar step = (glyphRight - glyphLeft) / graphemesInGlyph;
+                    for (int i = 0; i < graphemesInGlyph; ++i)
+                        run.fBreakPositions.push_back(glyphLeft + step * (i + 1));
+                    graphemesInGlyph = 1;
+                    glyphLeft = glyphRight;
+                }
             }
         }
         fPosition += info.fAdvance.fX;

diff --git a/skiko/src/commonMain/kotlin/org/jetbrains/skia/shaper/Shaper.kt b/skiko/src/commonMain/kotlin/org/jetbrains/skia/shaper/Shaper.kt
@@ -195,16 +195,16 @@ class Shaper internal constructor(ptr: NativePointer) : Managed(ptr, _FinalizerH
             Stats.onNativeCall()
             ManagedString(text).use { managedString ->
                 interopScope {
-                    TextLine(
-                        _nShapeLine(
-                            _ptr,
-                            managedString._ptr,
-                            getPtr(font),
-                            optsFeaturesLen = opts.features?.size ?: 0,
-                            optsFeatures = arrayOfFontFeaturesToInterop(opts.features),
-                            optsBooleanProps = opts._booleanPropsToInt()
-                        )
+                    val linePtr = _nShapeLine(
+                        _ptr,
+                        managedString._ptr,
+                        getPtr(font),
+                        optsFeaturesLen = opts.features?.size ?: 0,
+                        optsFeatures = arrayOfFontFeaturesToInterop(opts.features),
+                        optsBooleanProps = opts._booleanPropsToInt()
                     )
+                    require(linePtr != NullPointer) { "Shape line: $text returned nullptr" }
+                    TextLine(linePtr)
                 }
 
             }

diff --git a/skiko/src/commonTest/kotlin/org/jetbrains/skia/TextLineTest.kt b/skiko/src/commonTest/kotlin/org/jetbrains/skia/TextLineTest.kt
@@ -1,10 +1,8 @@
 package org.jetbrains.skiko
 
-import org.jetbrains.skia.Font
-import org.jetbrains.skia.ManagedString
-import org.jetbrains.skia.TextLine
-import org.jetbrains.skia.Typeface
+import org.jetbrains.skia.*
 import org.jetbrains.skia.impl.use
+import org.jetbrains.skia.shaper.ShapingOptions
 import kotlin.test.Test
 import kotlin.test.assertContentEquals
 import kotlin.test.assertEquals
@@ -227,4 +225,11 @@ class TextLineTest {
         assertCloseEnough(expected = 48.0f, actual = textLine.getCoordAtOffset(2), epsilon = eps)
         assertCloseEnough(expected = 69.0f, actual = textLine.getCoordAtOffset(3), epsilon = eps)
     }
+
+    @Test
+    fun dontCrashOnStringsWithoutGlyphs() = runTest {
+        val failedOn = "\uFE0F" // Variation Selector-16
+        val textLine = TextLine.make(failedOn, jbMono36(), ShapingOptions.DEFAULT)
+        assertCloseEnough(expected = 0f, actual = textLine.width)
+    }
 }