Bug 73849 – In FontCacheAndroid.cpp should keep the pointer valid returned from CString::data()

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Xianzhu Wang]

In FontCacheAndroid.cpp, name will be invalid after the following piece of code if name is got from String::utf8().data(), because the temporary CString returned from String::utf8() has been destructed after that statement.

FontPlatformData* FontCache::createFontPlatformData(const FontDescription& fontDescription, const AtomicString& family)
{
    const char* name = 0;

    // If a fallback font is being created (e.g. "-webkit-monospace"), convert
    // it in to the fallback name (e.g. "monospace").
    if (!family.length() || family.startsWith("-webkit-"))
        name = getFallbackFontName(fontDescription);
    else
        name = family.string().utf8().data();

[Xianzhu Wang]

Created attachment 117908 [details]
patch

[James Robinson]

Comment on attachment 117908 [details]
patch

Is this code testable in any way?

[Adam Barth]

Comment on attachment 117908 [details]
patch

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

I agree with James that this should be testable.  Is this covered by an existing LayoutTests on Android?

> Source/WebCore/platform/graphics/chromium/FontCacheAndroid.cpp:150
> +    CString nameString; // Keeps name valid within scope of this function in case that name is got from family.

I'm not sure "got" is grammatically correct in this comment.  Maybe remplace "got from family" with "from a family"?

[Peter Beverloo]

Thank you!
This change lgtm and actually is how it's being used downstream. The nameString variable got lost in a clean-up, for which I'm to blame.

In order to properly run tests we need to be able to link and run, but as of this morning there still are ~30 unique linking errors present. Downstream the code is being tested using regular layout tests.

[Adam Barth]

Comment on attachment 117908 [details]
patch

I'm not as concerned about whether the tests are currently being run upstream but rather whether we will have test coverage for this change when we do start running tests.  If I understood you correctly, you're saying that we will.

[Peter Beverloo]

Indeed, the font cache will be exercised by most layout tests as it's used in the CSS Font Selector. Xianzhu Wang, do we have any layout tests specifically testing the fallback behavior?

[Adam Barth]

Ok.  Looks like we just need to fix the typo in the comment and we're ready to land.

[Xianzhu Wang]

Created attachment 118231 [details]
updated patch

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

>> Source/WebCore/platform/graphics/chromium/FontCacheAndroid.cpp:150
>> +    CString nameString; // Keeps name valid within scope of this function in case that name is got from family.
> 
> I'm not sure "got" is grammatically correct in this comment.  Maybe remplace "got from family" with "from a family"?

Done.

(In reply to comment #6)
> Indeed, the font cache will be exercised by most layout tests as it's used in the CSS Font Selector. Xianzhu Wang, do we have any layout tests specifically testing the fallback behavior?

I've that the changed code has already been covered by existing layout tests.

I'm running layout tests to see if the fallback behavior is covered. If not, will file another bug for adding a layout test for it.

[Xianzhu Wang]

Verified that the fallback behavior is also covered by at least fast/css/font-family-pictograph.html.

[Adam Barth]

Thanks.

[WebKit Review Bot]

Comment on attachment 118231 [details]
updated patch

Clearing flags on attachment: 118231

Committed r102255: <http://trac.webkit.org/changeset/102255>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.