Bug 38530 - SVG fonts trigger GlyphPage::fill with null font
Summary: SVG fonts trigger GlyphPage::fill with null font
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2010-05-04 10:43 PDT by Joseph Pecoraro
Modified: 2010-05-24 11:42 PDT (History)
3 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 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.
Comment 1 Joseph Pecoraro 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?
Comment 2 mitz 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.
Comment 3 Joseph Pecoraro 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
Comment 4 Joseph Pecoraro 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
Comment 5 Joseph Pecoraro 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.
Comment 6 Joseph Pecoraro 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.
Comment 7 WebKit Review Bot 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.
Comment 8 Joseph Pecoraro 2010-05-04 15:35:31 PDT
Created attachment 55059 [details]
[PATCH] Suggested Fix Based on Comments + Style Fix

Style fixes.
Comment 9 Joseph Pecoraro 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.
Comment 10 mitz 2010-05-24 11:42:36 PDT
<rdar://problem/7828848>