Bug 73849 - In FontCacheAndroid.cpp should keep the pointer valid returned from CString::data()
Summary: In FontCacheAndroid.cpp should keep the pointer valid returned from CString::...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Android Unspecified
: P2 Normal
Assignee: Xianzhu Wang
URL:
Keywords:
Depends on:
Blocks: 73672
  Show dependency treegraph
 
Reported: 2011-12-05 11:29 PST by Xianzhu Wang
Modified: 2011-12-07 11:22 PST (History)
5 users (show)

See Also:


Attachments
patch (1.80 KB, patch)
2011-12-05 11:35 PST, Xianzhu Wang
abarth: review+
abarth: commit-queue-
Details | Formatted Diff | Diff
updated patch (1.87 KB, patch)
2011-12-07 10:51 PST, Xianzhu Wang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xianzhu Wang 2011-12-05 11:29:47 PST
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();
Comment 1 Xianzhu Wang 2011-12-05 11:35:49 PST
Created attachment 117908 [details]
patch
Comment 2 James Robinson 2011-12-05 12:18:09 PST
Comment on attachment 117908 [details]
patch

Is this code testable in any way?
Comment 3 Adam Barth 2011-12-05 13:17:51 PST
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"?
Comment 4 Peter Beverloo 2011-12-06 05:39:50 PST
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.
Comment 5 Adam Barth 2011-12-06 09:55:50 PST
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.
Comment 6 Peter Beverloo 2011-12-07 03:45:42 PST
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?
Comment 7 Adam Barth 2011-12-07 10:37:37 PST
Ok.  Looks like we just need to fix the typo in the comment and we're ready to land.
Comment 8 Xianzhu Wang 2011-12-07 10:51:40 PST
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.
Comment 9 Xianzhu Wang 2011-12-07 11:06:28 PST
Verified that the fallback behavior is also covered by at least fast/css/font-family-pictograph.html.
Comment 10 Adam Barth 2011-12-07 11:09:08 PST
Thanks.
Comment 11 WebKit Review Bot 2011-12-07 11:22:02 PST
Comment on attachment 118231 [details]
updated patch

Clearing flags on attachment: 118231

Committed r102255: <http://trac.webkit.org/changeset/102255>
Comment 12 WebKit Review Bot 2011-12-07 11:22:07 PST
All reviewed patches have been landed.  Closing bug.