Bug 73849

Summary:

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

Product:

WebKit

Reporter:

Xianzhu Wang <wangxianzhu>

Component:

Platform

Assignee:

Xianzhu Wang <wangxianzhu>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, cc-bugs, jamesr, peter, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Android

OS:

Unspecified

Bug Depends on:

Bug Blocks:

73672

Attachments:

Description	Flags
patch	abarth: review+, abarth: commit-queue-
updated patch	none

Xianzhu Wang

Reported 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();

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
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Xianzhu Wang

Comment 1 2011-12-05 11:35:49 PST

Created attachment 117908 [details] patch

James Robinson

Comment 2 2011-12-05 12:18:09 PST

Comment on attachment 117908 [details] patch Is this code testable in any way?

Adam Barth

Comment 3 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"?

Peter Beverloo

Comment 4 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.

Adam Barth

Comment 5 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.

Peter Beverloo

Comment 6 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?

Adam Barth

Comment 7 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.

Xianzhu Wang

Comment 8 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.

Xianzhu Wang

Comment 9 2011-12-07 11:06:28 PST

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

Adam Barth

Comment 10 2011-12-07 11:09:08 PST

Thanks.

WebKit Review Bot

Comment 11 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>

WebKit Review Bot

Comment 12 2011-12-07 11:22:07 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.