Bug 86738 – SVGTextRunRenderingContext can return null font, calling code asserts not null

• Component: SVG
• Status: RESOLVED FIXED

[Stephen Chenney]

The method SVGTextRunRenderingContext::glyphDataForCharacter has this code:


    const SimpleFontData* primaryFont = font.primaryFont();
    ASSERT(primaryFont);

    pair<GlyphData, GlyphPage*> pair = font.glyphDataAndPageForCharacter(character, mirror);
    GlyphData glyphData = pair.first;
    if (!glyphData.fontData)
        return glyphData;

    GlyphData missingGlyphData = primaryFont->missingGlyphData();
    if (glyphData.glyph == missingGlyphData.glyph && glyphData.fontData == missingGlyphData.fontData
)
        return glyphData;

Note that if glyphData.fontData is null the glyphData is returned as is.

The calling code is in WidthIterator::advance

    while (textIterator.consume(character, clusterLength)) {
        unsigned advanceLength = clusterLength;
        const GlyphData& glyphData = glyphDataForCharacter(character, rtl, textIterator.currentCharacter(), advanceLength);
        Glyph glyph = glyphData.glyph;
        const SimpleFontData* fontData = glyphData.fontData;

        ASSERT(fontData);

        // Now that we have a glyph and font data, get its width.
        float width;
        if (character == '\t' && m_run.allowTabs())
            width = m_font->tabWidth(*fontData, m_run.tabSize(), m_run.xPos() + m_runWidthSoFar + widthSinceLastRounding);
        else {
            width = fontData->widthForGlyph(glyph);

We have a problem here because fontData will be null in some cases, and this gets by the assert in release builds, and then crashes when the null font is used.

I have no idea at all how to test this, but it may be the cause, or a related symptom, of a top Chromium crasher: http://code.google.com/p/chromium/issues/detail?id=128418

I'll get a patch up shortly that I would like to put in.despite not having a test. It seems this is obviously wrong just by inspection, and we should fix it. The question is, do we assert in the SVG code, or return something non-null?

[Nikolas Zimmermann]

We should at least understand in what cases 'glyphDataAndPageForCharacter' from FontFastPath.cpp returns a 0 fontData.

The null-checks in SVGTextRunRenderingContext::glyphDataForCharacter are misleading, as the FontFastPath glyphDataAndPageForCharacter method should never return a 0 FontData -- it should use a system fallback font instead. Maybe this can fail as well? I'm not sure, and we should check this first.

Note that HTML would also be affected by this -- it uses the same glyphDataAndPageForCharacter functionality from FontFastPath.cpp, and for non-SVGFonts the ASSERT(fontData) is in since years.

[Stephen Chenney]

Created attachment 142740 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 142740 [details]
Patch

Attachment 142740 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12721868

[Stephen Chenney]

Comment on attachment 142740 [details]
Patch

gtk failure doesn't relate to anything in this patch. No idea what's wrong there.

[Nikolas Zimmermann]

Comment on attachment 142740 [details]
Patch

Good catch. I don't see a link between the Gtk build problems and your patch, so r=me.

[Nikolas Zimmermann]

Btw, please do try to construct a testcase - even if this now goes in as quick-fix for Chromium problems, we need to make sure to never regress this again.
I'd prefer to land the patch together with the testcase - if that's impractical I'm fine with this as-is, but we shouldn't forget about the test :-)

[Stephen Chenney]

I'm going to set it up to run with the acid3 test that was failing, without the real fix for that test. If it no longer crashes with this patch, I'll consider that as verifying that this change works (although I expect the actual test output may be wrong because the wrong font may get used).

I honestly have no idea at all how to test it otherwise. Well, a unit test could do the job, but that would really require some work, and hence needs its own bug.

[Stephen Chenney]

I tested that this fixes the acid3 test (Bug 86715), which I take as evidence that this patch solves a problem and does not introduce a new one. As I said, unit testing is the real way to get at this, but we'll have to wait for such support for SVG.

I'll commit now.

[Stephen Chenney]

Committed r117788: <http://trac.webkit.org/changeset/117788>