Bug 38530

Summary:

SVG fonts trigger GlyphPage::fill with null font

Product:

WebKit

Reporter:

Joseph Pecoraro <joepeck>

Component:

Text

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

joepeck, mitz, webkit.review.bot

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

OS:

OS X 10.5

Attachments:

Description	Flags
[PATCH] Proposed Fix - Test for SVG Fonts in initializePage	mitz: review-
[PATCH] Suggested Fix Based on Comments	none
[PATCH] Suggested Fix Based on Comments + Style Fix	mitz: review+

Joseph Pecoraro

Reported 2010-05-04 10:43:40 PDT

If the primary font is non-SVG, but there is an SVG segment, than when initializing GlyphPages a page may attempt to be filled with an SVG font. This results in a null font being used in GlyphPage::fill() which does not crash but instead fills the page buffer with 0s. This point can be reached if the SVG font is loading while layout occurs.

Attachments
[PATCH] Proposed Fix - Test for SVG Fonts in initializePage (5.23 KB, patch) 2010-05-04 11:13 PDT, Joseph Pecoraro	mitz: review-	Details Formatted Diff Diff
[PATCH] Suggested Fix Based on Comments (3.05 KB, patch) 2010-05-04 15:27 PDT, Joseph Pecoraro	no flags	Details Formatted Diff Diff
[PATCH] Suggested Fix Based on Comments + Style Fix (3.05 KB, patch) 2010-05-04 15:35 PDT, Joseph Pecoraro	mitz: review+	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Joseph Pecoraro

Comment 1 2010-05-04 11:13:37 PDT

Created attachment 55029 [details] [PATCH] Proposed Fix - Test for SVG Fonts in initializePage This attempts to quickly handle an SVG font in GlyphPageTreeNode::initializePage. This prevents sending a null font, so it should be saving some memory. I can't think of a way to test this. It may just be a memory improvement. Any thoughts from font/text experts?

mitz

Comment 2 2010-05-04 11:25:37 PDT

Comment on attachment 55029 [details] [PATCH] Proposed Fix - Test for SVG Fonts in initializePage I don’t think this is the best fix. SegmentedFontData::isSVGFont()—and by extension FontData::isSVGFont()— doesn’t make sense. Even though currently SVG segments aren’t handled correctly due to bug 32227, introducing this concept is a step in the wrong direction. Instead, GlyphPageTreeNode::initializePage() should examine individual SimpleFontData for SVG-ness, and in the SVG case, instead of calling fill() it should zero-fill and behave as if fill() returned false. Since fill() has two call sites in initializePage(), you can add a small static helper method that does this.

Joseph Pecoraro

Comment 3 2010-05-04 14:27:59 PDT

While writing a new patch, which shouldn't have changed anything, I was seeing a few changes to this pixel test. This looks stale to me, since Safari 4.0.5 doesn't look like that for me: http://trac.webkit.org/browser/trunk/LayoutTests/platform/mac/svg/text/text-fonts-01-t-expected.png

Joseph Pecoraro

Comment 4 2010-05-04 14:42:49 PDT

Both of these are now failing for me after I backed my patch out. Work for me on another machine. svg/W3C-SVG-1.1/text-fonts-01-t.svg svg/text/text-fonts-01-t.svg

Joseph Pecoraro

Comment 5 2010-05-04 15:16:54 PDT

After a reboot, and updating to ToT without my patch these are still failing 100% for me but not on the bots. So I'm going to reapply my patch and suggest it without changes to these tests.

Joseph Pecoraro

Comment 6 2010-05-04 15:27:22 PDT

Created attachment 55052 [details] [PATCH] Suggested Fix Based on Comments After putting in my own expected results for before this change, this change did not affect the results.

WebKit Review Bot

Comment 7 2010-05-04 15:28:52 PDT

Attachment 55052 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebCore/platform/graphics/GlyphPageTreeNode.cpp:129: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] WebCore/platform/graphics/GlyphPageTreeNode.cpp:134: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] WebCore/platform/graphics/GlyphPageTreeNode.cpp:136: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 3 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.

Joseph Pecoraro

Comment 8 2010-05-04 15:35:31 PDT

Created attachment 55059 [details] [PATCH] Suggested Fix Based on Comments + Style Fix Style fixes.

Joseph Pecoraro

Comment 9 2010-05-04 16:18:36 PDT

Committed r58786 M WebCore/ChangeLog M WebCore/platform/graphics/GlyphPageTreeNode.cpp r58786 = 3aefd0619567aa7a18a92b20d442d1274a40e72f (refs/remotes/trunk) http://trac.webkit.org/changeset/58786 I'll be watching the bots to see if the tests change, but I don't expect them to.

mitz

Comment 10 2010-05-24 11:42:36 PDT

<rdar://problem/7828848>

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