Bug 154875 – Small-caps non-BMP characters are garbled in the complex text codepath

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Michael Saboff]

In platform/graphics/mac/ComplexTextController.cpp::capitalize() the ASSERT is firing for a baseCharacter value of 66639 (\u{1044f}) which has a u_toupper() value of 66599 (\u{10427}).

static inline Optional<UChar32> capitalized(UChar32 baseCharacter)
{
    if (U_GET_GC_MASK(baseCharacter) & U_GC_M_MASK)
        return Nullopt;

    UChar32 uppercaseCharacter = u_toupper(baseCharacter);
    ASSERT(uppercaseCharacter == baseCharacter || uppercaseCharacter <= 0xFFFF);
    if (uppercaseCharacter != baseCharacter)
        return uppercaseCharacter;
    return Nullopt;
}

The ASSERT checks that the upper case value is in the BMP.  If we care about the whether or not the upper case character stays in the same plane, a better ASSERT would be:
    ASSERT(uppercaseCharacter == baseCharacter || (U_IS_BMP(baseCharacter) == U_IS_BMP(uppercaseCharacter)));
If we don't care about the same plane, we could eliminate the ASSERT altogether.

Here is the crash trace:
ASSERTION FAILED: uppercaseCharacter == baseCharacter || uppercaseCharacter <= 0xFFFF
/Volumes/Data/src/webkit/Source/WebCore/platform/graphics/mac/ComplexTextController.cpp(301) : Optional<UChar32> WebCore::capitalized(UChar32)
1   0x104b78ad0 WTFCrash
2   0x1098a5aa2 WebCore::capitalized(int)
3   0x1098a3219 WebCore::ComplexTextController::collectComplexTextRuns()
4   0x1098a2aaf WebCore::ComplexTextController::ComplexTextController(WebCore::FontCascade const&, WebCore::TextRun const&, bool, WTF::HashSet<WebCore::Font const*, WTF::PtrHash<WebCore::Font const*>, WTF::HashTraits<WebCore::Font const*> >*, bool)
5   0x1098a4c04 WebCore::ComplexTextController::ComplexTextController(WebCore::FontCascade const&, WebCore::TextRun const&, bool, WTF::HashSet<WebCore::Font const*, WTF::PtrHash<WebCore::Font const*>, WTF::HashTraits<WebCore::Font const*> >*, bool)
6   0x109efb58f WebCore::FontCascade::floatWidthForComplexText(WebCore::TextRun const&, WTF::HashSet<WebCore::Font const*, WTF::PtrHash<WebCore::Font const*>, WTF::HashTraits<WebCore::Font const*> >*, WebCore::GlyphOverflow*) const
7   0x109ee8488 WebCore::FontCascade::width(WebCore::TextRun const&, WTF::HashSet<WebCore::Font const*, WTF::PtrHash<WebCore::Font const*>, WTF::HashTraits<WebCore::Font const*> >*, WebCore::GlyphOverflow*) const
8   0x10b34f90c WebCore::RenderText::widthFromCache(WebCore::FontCascade const&, int, int, float, WTF::HashSet<WebCore::Font const*, WTF::PtrHash<WebCore::Font const*>, WTF::HashTraits<WebCore::Font const*> >*, WebCore::GlyphOverflow*, WebCore::RenderStyle const&) const
9   0x10b34cc3a WebCore::RenderText::width(unsigned int, unsigned int, WebCore::FontCascade const&, float, WTF::HashSet<WebCore::Font const*, WTF::PtrHash<WebCore::Font const*>, WTF::HashTraits<WebCore::Font const*> >*, WebCore::GlyphOverflow*) const
10  0x10b34c9a5 WebCore::RenderText::width(unsigned int, unsigned int, float, bool, WTF::HashSet<WebCore::Font const*, WTF::PtrHash<WebCore::Font const*>, WTF::HashTraits<WebCore::Font const*> >*, WebCore::GlyphOverflow*) const
11  0x10b08cfde WebCore::setLogicalWidthForTextRun(WebCore::RootInlineBox*, WebCore::BidiRun*, WebCore::RenderText&, float, WebCore::LineInfo const&, WTF::HashMap<WebCore::InlineTextBox const*, std::__1::pair<WTF::Vector<WebCore::Font const*, 0ul, WTF::CrashOnOverflow, 16ul>, WebCore::GlyphOverflow>, WTF::PtrHash<WebCore::InlineTextBox const*>, WTF::HashTraits<WebCore::InlineTextBox const*>, WTF::HashTraits<std::__1::pair<WTF::Vector<WebCore::Font const*, 0ul, WTF::CrashOnOverflow, 16ul>, WebCore::GlyphOverflow> > >&, WebCore::VerticalPositionCache&, WTF::Vector<WebCore::WordMeasurement, 64ul, WTF::CrashOnOverflow, 16ul>&)
12  0x10b08ba6e WebCore::RenderBlockFlow::computeInlineDirectionPositionsForSegment(WebCore::RootInlineBox*, WebCore::LineInfo const&, WebCore::ETextAlign, float&, float&, WebCore::BidiRun*, WebCore::BidiRun*, WTF::HashMap<WebCore::InlineTextBox const*, std::__1::pair<WTF::Vector<WebCore::Font const*, 0ul, WTF::CrashOnOverflow, 16ul>, WebCore::GlyphOverflow>, WTF::PtrHash<WebCore::InlineTextBox const*>, WTF::HashTraits<WebCore::InlineTextBox const*>, WTF::HashTraits<std::__1::pair<WTF::Vector<WebCore::Font const*, 0ul, WTF::CrashOnOverflow, 16ul>, WebCore::GlyphOverflow> > >&, WebCore::VerticalPositionCache&, WTF::Vector<WebCore::WordMeasurement, 64ul, WTF::CrashOnOverflow, 16ul>&)
13  0x10b08b171 WebCore::RenderBlockFlow::computeInlineDirectionPositionsForLine(WebCore::RootInlineBox*, WebCore::LineInfo const&, WebCore::BidiRun*, WebCore::BidiRun*, bool, WTF::HashMap<WebCore::InlineTextBox const*, std::__1::pair<WTF::Vector<WebCore::Font const*, 0ul, WTF::CrashOnOverflow, 16ul>, WebCore::GlyphOverflow>, WTF::PtrHash<WebCore::InlineTextBox const*>, WTF::HashTraits<WebCore::InlineTextBox const*>, WTF::HashTraits<std::__1::pair<WTF::Vector<WebCore::Font const*, 0ul, WTF::CrashOnOverflow, 16ul>, WebCore::GlyphOverflow> > >&, WebCore::VerticalPositionCache&, WTF::Vector<WebCore::WordMeasurement, 64ul, WTF::CrashOnOverflow, 16ul>&)
14  0x10b08da37 WebCore::RenderBlockFlow::createLineBoxesFromBidiRuns(unsigned int, WebCore::BidiRunList<WebCore::BidiRun>&, WebCore::InlineIterator const&, WebCore::LineInfo&, WebCore::VerticalPositionCache&, WebCore::BidiRun*, WTF::Vector<WebCore::WordMeasurement, 64ul, WTF::CrashOnOverflow, 16ul>&)
15  0x10b08fb11 WebCore::RenderBlockFlow::layoutRunsAndFloatsInRange(WebCore::LineLayoutState&, WebCore::BidiResolverWithIsolate<WebCore::InlineIterator, WebCore::BidiRun, WebCore::BidiIsolatedRun>&, WebCore::InlineIterator const&, WebCore::BidiStatus const&, unsigned int)
16  0x10b08dffb WebCore::RenderBlockFlow::layoutRunsAndFloats(WebCore::LineLayoutState&, bool)
17  0x10b092caa WebCore::RenderBlockFlow::layoutLineBoxes(bool, WebCore::LayoutUnit&, WebCore::LayoutUnit&)
18  0x10b06ae82 WebCore::RenderBlockFlow::layoutInlineChildren(bool, WebCore::LayoutUnit&, WebCore::LayoutUnit&)
19  0x10b069eff WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit)
20  0x10b034f89 WebCore::RenderBlock::layout()
21  0x10b06d6ec WebCore::RenderBlockFlow::layoutBlockChild(WebCore::RenderBox&, WebCore::RenderBlockFlow::MarginInfo&, WebCore::LayoutUnit&, WebCore::LayoutUnit&)
22  0x10b06b0e6 WebCore::RenderBlockFlow::layoutBlockChildren(bool, WebCore::LayoutUnit&)
23  0x10b069f22 WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit)
24  0x10b034f89 WebCore::RenderBlock::layout()
25  0x10b06d6ec WebCore::RenderBlockFlow::layoutBlockChild(WebCore::RenderBox&, WebCore::RenderBlockFlow::MarginInfo&, WebCore::LayoutUnit&, WebCore::LayoutUnit&)
26  0x10b06b0e6 WebCore::RenderBlockFlow::layoutBlockChildren(bool, WebCore::LayoutUnit&)
27  0x10b069f22 WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit)
28  0x10b034f89 WebCore::RenderBlock::layout()
29  0x10b06d6ec WebCore::RenderBlockFlow::layoutBlockChild(WebCore::RenderBox&, WebCore::RenderBlockFlow::MarginInfo&, WebCore::LayoutUnit&, WebCore::LayoutUnit&)
30  0x10b06b0e6 WebCore::RenderBlockFlow::layoutBlockChildren(bool, WebCore::LayoutUnit&)
31  0x10b069f22 WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit)

[Radar WebKit Bug Importer]

<rdar://problem/24915740>

[Michael Saboff]

Created attachment 272601 [details]
Patch

[Michael Saboff]

Committed r197423: <http://trac.webkit.org/changeset/197423>

[Darin Adler]

Comment on attachment 272601 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=272601&action=review

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:301
> -    ASSERT(uppercaseCharacter == baseCharacter || uppercaseCharacter <= 0xFFFF);
> +    ASSERT(uppercaseCharacter == baseCharacter || (U_IS_BMP(baseCharacter) == U_IS_BMP(uppercaseCharacter)));

This change is incorrect. The assertion correctly reflects what the code that calls this function can currently handle. The code that calls capitalized does not handle a replaced character that is non-BMP correctly. This function should have been returning Optional<UChar> given what the callers expect.

Look for code below that puts the capitalized character into m_smallCapsBuffer. It does not turn the non-BMP character into a surrogate pair, and so depends on what was asserted here before. We need test cases that cover this.

[Myles C. Maxfield]

Comment on attachment 272601 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=272601&action=review

>> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:301
>> +    ASSERT(uppercaseCharacter == baseCharacter || (U_IS_BMP(baseCharacter) == U_IS_BMP(uppercaseCharacter)));
> 
> This change is incorrect. The assertion correctly reflects what the code that calls this function can currently handle. The code that calls capitalized does not handle a replaced character that is non-BMP correctly. This function should have been returning Optional<UChar> given what the callers expect.
> 
> Look for code below that puts the capitalized character into m_smallCapsBuffer. It does not turn the non-BMP character into a surrogate pair, and so depends on what was asserted here before. We need test cases that cover this.

Yes, you are correct. Michael and I are writing a patch write now to fix this up.

[Myles C. Maxfield]

Reopening to attach new patch.

[Myles C. Maxfield]

Created attachment 272620 [details]
Patch

[Michael Saboff]

Comment on attachment 272620 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=272620&action=review

r=me.  Remove the duplicate change in capitalized().

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:301
> -    ASSERT(uppercaseCharacter == baseCharacter || uppercaseCharacter <= 0xFFFF);
> +    ASSERT(uppercaseCharacter == baseCharacter || (U_IS_BMP(baseCharacter) == U_IS_BMP(uppercaseCharacter)));

This is not needed since it is the prior patch.

[Myles C. Maxfield]

Created attachment 272622 [details]
Patch

[Myles C. Maxfield]

Committed r197435: <http://trac.webkit.org/changeset/197435>

[Alexey Proskuryakov]

Comment on attachment 272622 [details]
Patch

Clearing review flag, as the bug is in RESOLVED state.