Bug 25770 – [chromium] Crash in FontFallbackList::determinePitch(const Font* font)

• Component: CSS
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

Chrome is seeing reports of a few crashes in:

void FontFallbackList::determinePitch(const Font* font) const
{
    const FontData* fontData = primaryFont(font);
    if (!fontData->isSegmented()) // CRASH RIGHT HERE
        m_pitch = static_cast<const SimpleFontData*>(fontData)->pitch();
    else {
        const SegmentedFontData* segmentedFontData = static_cast<const SegmentedFontData*>(fontData);
        unsigned numRanges = segmentedFontData->numRanges();
        if (numRanges == 1)
            m_pitch = segmentedFontData->rangeAt(0).fontData()->pitch();
        else
            m_pitch = VariablePitch;
    }
}

is it ever possible/legal for primaryFont(font) to return null?  It certainly looks like it can.  But I'm not sure if that's because Chrome is doing something wrong, or if because determinePitch() is making bad assumptions about primaryFont()?

If the primaryFont() assumption is wrong, then this crasher probably hits Safari too, but I don't know how I would reproduce it.


For context:

    const SimpleFontData* primaryFont() const {
        if (!m_cachedPrimaryFont)
            cachePrimaryFont();
        return m_cachedPrimaryFont;
    }

void Font::cachePrimaryFont() const
{
    ASSERT(m_fontList);
    ASSERT(!m_cachedPrimaryFont);
    m_cachedPrimaryFont = m_fontList->primaryFont(this)->fontDataForCharacter(' ');
}

    const SimpleFontData* fontDataForCharacter(UChar32 c) const
    {
        return m_glyphFontData[indexForCharacter(c)];
    }

[Eric Seidel (no email)]

Sorry, the crash reports are internal (probably for similar reasons to why Apple's crash reports are), but here is the link for posterity:
http://crash/reportview?product=Chrome&version=2.0.180.0&signature=WebCore%3A%3AFontFallbackList%3A%3AdeterminePitch(WebCore%3A%3AFont+const+*)-67F854

[Hironori Bono]

Greetings,

This issue somehow fall-backs to me. :)

(In reply to comment #0)
> is it ever possible/legal for primaryFont(font) to return null?  It certainly looks like it can.  But I'm not sure if that's because Chrome is doing something wrong, or if because determinePitch() is making bad assumptions about primaryFont()?
> If the primaryFont() assumption is wrong, then this crasher probably hits Safari too, but I don't know how I would reproduce it.

Yes, it is possible on Windows Chrome as I wrote in <http://crbug.com/59707>. This crash happens on a PC that satisfies all the following conditions:
1. a last-resort font ("Arial", "Time New Roman", or "Courier New") is corrupted, and;
2. FontCache::getFontData() calls FontCache::getLastResortFallbackFont() to use the corrupted last-resort font.

One of its reproduction steps is listed below:
1. Delete "Arial", "Times New Roman", and "Courier New" fonts from the "C:\WINDOWS\Fonts" folder on Windows XP. (We cannot delete them on Vista or 7 because they are protected by Windows.)
2. Start Chrome.
(I recommend to try this on a virtual machine because it is very dangerous.)

I think this crash is a problem of our FontCache::getLastResortFallbackFont() implementation for Windows that may choose a corrupted font. (We also try choosing DEFAULT_GUI_FONT and non-client metrics fonts, which Windows ensures they are sane, as Windows Safari does?)

Regards,

Hironori Bono

[Hironori Bono]

Created attachment 74209 [details]
A copy-and-paste fix

Greetings,

This change just copied some font-fallback code from "WebCore/platform/graphics/win/FontCacheWin.cpp" to "WebCore/platform/graphics/chromium/FontCacheChromiumWin.cpp" to fix this Chromium crash. (As far as I tested with my reproduction steps, this change fixes this crash.) Unfortunately, I do not have any ideas how to write a layout test for this crash because it happens only on a PC that do not have some system fonts corrupted.

Regards,

Hironori Bono

[Darin Fisher (:fishd, Google)]

Comment on attachment 74209 [details]
A copy-and-paste fix

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

R=me, but it sure would be nice to find a way to share code with the PLATFORM(WIN) port.

> WebCore/platform/graphics/chromium/FontCacheChromiumWin.cpp:426
> +static SimpleFontData* fontDataFromDescriptionAndLogFont(FontCache* fontCache, const FontDescription& fontDescription, const LOGFONT& font, wchar_t* outFontFamilyName)

nit: it is normally nice to put helper functions at the top of the file, so as not
to break up the flow of class method implementations.  this doesn't cause a readability
problem in this case, but often once a static method is added like this, it becomes
an invitation for more people to add static functions like this, and pretty soon the
organization of a .cpp file is lost.

[Hironori Bono]

(In reply to comment #4)
> (From update of attachment 74209 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74209&action=review
> 
> R=me, but it sure would be nice to find a way to share code with the PLATFORM(WIN) port.
> 
> > WebCore/platform/graphics/chromium/FontCacheChromiumWin.cpp:426
> > +static SimpleFontData* fontDataFromDescriptionAndLogFont(FontCache* fontCache, const FontDescription& fontDescription, const LOGFONT& font, wchar_t* outFontFamilyName)
> 
> nit: it is normally nice to put helper functions at the top of the file, so
> as not to break up the flow of class method implementations.  this doesn't
> cause a readability problem in this case, but often once a static method is
> added like this, it becomes an invitation for more people to add static
> functions like this, and pretty soon the organization of a .cpp file is lost.

Thank you for your comment. Even though I realize this is a coding style of Chromium and I don't have any objections, I would like to write an excuse why I put this function there. In brief, I have put this function there just because I thought this is a style for this file. It seems this file arranges its functions (including static ones) in the same order as the ones in 'FontCacheWin.cpp'. To keep this order consistency, I thought I needed to put fontDataFromDescriptionAndLogFont() to the same place (i.e. between FontCache::getSimilarFontPlatformData() and FontCache::getLastResortFallbackFont()).
In fact, this file has some other static functions inserted between FontCache methods, such as charactersAreAllASCII(), lookupAltName(), etc. Do we need refactoring for this file so we can move all these static functions in this file at the beginning of this file?

Regards,

Hironori Bono

[David Levin]

(In reply to comment #5)
> Do we need refactoring for this file so we can move all these static functions in this file at the beginning of this file?

Short answer: Sounds good.

Long answer:

I appreciate your goal of following the file's current style. However, here's a simple outlook that is slightly different:
1. WebKit has no stated style on this matter.
2. This file is solely used by Chromium (and thus almost always only modified by chromium folks).

Since there is no conflict between the two styles and Chromium has a standard practice here that makes readability easier for Chromium folks, I'd try to follow its standard. 

Feel free to submit a *different* patch that fixes places in this file where it doesn't meet this practice. (cc me and) I'm happy to r+ such a change.

[Hironori Bono]

Created attachment 75345 [details]
A copy-and-paste fix (including refactoring)

(In reply to comment #6)
> (In reply to comment #5)
> > Do we need refactoring for this file so we can move all these static functions in this file at the beginning of this file?
> 
> Short answer: Sounds good.

Thank you for your comment. I have moved all the static functions at the beginning of this file because it is pretty easy. (That is, the functionality is the same as the previous one.) Is it possible to take another look?

Regards,

Hironori Bono

[WebKit Review Bot]

Attachment 75345 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebCore/ChangeLog', u'WebCore/platform/graphics/chromium/FontCacheChromiumWin.cpp']" exit_code: 1
WebCore/platform/graphics/chromium/FontCacheChromiumWin.cpp:335:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/graphics/chromium/FontCacheChromiumWin.cpp:336:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/graphics/chromium/FontCacheChromiumWin.cpp:337:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/graphics/chromium/FontCacheChromiumWin.cpp:338:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/graphics/chromium/FontCacheChromiumWin.cpp:339:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/graphics/chromium/FontCacheChromiumWin.cpp:340:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/graphics/chromium/FontCacheChromiumWin.cpp:341:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/graphics/chromium/FontCacheChromiumWin.cpp:342:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/graphics/chromium/FontCacheChromiumWin.cpp:343:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/graphics/chromium/FontCacheChromiumWin.cpp:399:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 10 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Levin]

(In reply to comment #7)
> Created an attachment (id=75345) [details]
> A copy-and-paste fix (including refactoring)
> 
> (In reply to comment #6)
> > (In reply to comment #5)
> > > Do we need refactoring for this file so we can move all these static functions in this file at the beginning of this file?
> > 
> > Short answer: Sounds good.
> 
> Thank you for your comment. I have moved all the static functions at the beginning of this file because it is pretty easy. (That is, the functionality is the same as the previous one.) Is it possible to take another look?

Please just commit the previous patch as it was (with the fixed) and then do the clean up in a separate patch. Otherwise by putting all of this in one patch, it hides what the fix was for the bug. 

When someone goes back in the history of the file to determine why a line of code changed, they will see a patch that says "Move static functions to top of file to match chromium style with no code changes" or something like that, and they can skip over that change.

If they happen to get to a line that was added/changed for "Crash in FontFallbackList::determinePitch(const Font* font)", then they will know it was fixing that issue (and not related to a code rearrangement).

I say all this due to experiences where I've been the person digging through the history of files.

[David Levin]

Comment on attachment 75345 [details]
A copy-and-paste fix (including refactoring)

See my previous comment. Thanks!

[Hironori Bono]

(In reply to comment #9)
> Please just commit the previous patch as it was (with the fixed) and then do the clean up in a separate patch. Otherwise by putting all of this in one patch, it hides what the fix was for the bug. 

Unfortunately, I cannot commit WebKit changes because I'm not a WebKit committer. Can you commit the previous patch on my behalf?

Regards,

Hironori Bono

[David Levin]

Comment on attachment 74209 [details]
A copy-and-paste fix

Ok, cq+'ing this. I subsequent patch should address the static function issue throughout the file.

[David Levin]

(In reply to comment #11)
> Can you commit the previous patch on my behalf?

Ok, I cq+'ed it. Please file another bug after it lands and attach a patch to clean up the file in general as you were doing.

[WebKit Commit Bot]

Comment on attachment 74209 [details]
A copy-and-paste fix

Clearing flags on attachment: 74209

Committed r73116: <http://trac.webkit.org/changeset/73116>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[David Levin]

(In reply to comment #13)
> (In reply to comment #11)
> > Can you commit the previous patch on my behalf?
> 
> Ok, I cq+'ed it. Please file another bug after it lands and attach a patch to clean up the file in general as you were doing.

Where is the patch to clean up the file?

[Hironori Bono]

(In reply to comment #16)
> Where is the patch to clean up the file?

Thank you for your notification. I have filed Bug 50611 <http://webkit.org/b/50611> and uploaded a refactoring change there.

Regards,

Hironori Bono

[Hironori Bono]

(In reply to comment #17)
> (In reply to comment #16)
> > Where is the patch to clean up the file?
> 
> Thank you for your notification. I have filed Bug 50611 <http://webkit.org/b/50611> and uploaded a refactoring change there.
> 
> Regards,
> 
> Hironori Bono

Unfortunately, this crash still happens after Chrome merged this change and I would like to post another change, which just adds a list of fallback fonts in FontCache::getLastResortFallbackFont(). Is it better to file another bug for this change, or re-open this bug?

Regards,

Hironori Bono

[David Levin]

(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #16)
> > > Where is the patch to clean up the file?
> > 
> > Thank you for your notification. I have filed Bug 50611 <http://webkit.org/b/50611> and uploaded a refactoring change there.
> > 
> > Regards,
> > 
> > Hironori Bono
> 
> Unfortunately, this crash still happens after Chrome merged this change and I would like to post another change, which just adds a list of fallback fonts in FontCache::getLastResortFallbackFont(). Is it better to file another bug for this change, or re-open this bug?
> 
> Regards,
> 
> Hironori Bono



Hmm good question. I would go with a new bug since bugs in webkit ~= code reviews in chromium. You wouldn't try to repro an old code review and start putting in new patches The old comments would make it more confusing. (Feel free to reference this bug in the new one.)

[Hironori Bono]

(In reply to comment #19)

> Hmm good question. I would go with a new bug since bugs in webkit ~= code reviews in chromium. You wouldn't try to repro an old code review and start putting in new patches The old comments would make it more confusing. (Feel free to reference this bug in the new one.)

Thank you for your suggestion. I have filed Bug 52422 <http://webkit.org/b/52422> and uploaded another speculative fix.

Regards,

Hironori Bono