Bug 91273 – Assertion failure/crash on Windows when using a font in an SVG element with an unreasonably large font size

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Roger Fong]

If you create an SVG Element and put some text in it but specify the font size to be something like 10000000000000000pt, Webkit will crash.
The problem is that it's expected that at least one of the fall back fonts will always work if a font is invalid but a check for an unreasonable size is never made,
thus the fall back checks keep trying valid font types with the invalid font size. Only happens on windows.

The fix would be to cap the font size when it gets processed.

[Roger Fong]

Created attachment 152329 [details]
Applies font size capping for windows

Caps font size between 0 and 1000000 when Windows processes font.

[Tim Horton]

Comment on attachment 152329 [details]
Applies font size capping for windows

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

> Source/WebCore/ChangeLog:5
> +        https://bugs.webkit.org/show_bug.cgi?id=91273

If there's a relevant radar you could add it here after the bugzilla link.

> Source/WebCore/ChangeLog:9
> +        This bug only happens on windows. The problem has to do with 

"This bug only happens on windows." should probably go after the explanation of the bug, and Windows is a proper noun.

> Source/WebCore/ChangeLog:12
> +        font sizes overflowing into negative values (which isn't being
> +        checked for) in the Windows specific code (Mac handles 
> +        these values just fine). The fix is to cap the font sizes to 

Most of the parenthesized commentary here could probably be removed.

> Source/WebCore/ChangeLog:15
> +        CSS font sizes work fine in Windows because they're already capped,
> +        we should do the same with SVG.

I know I've asked this before, but it would be nice to understand if there's a single bottleneck that all font sizes go through where we could cap them instead of two separate places. The other nice thing about the CSS bottleneck is that the capped size is exposed via the computed style to the web content, just for consistency's sake, though that likely doesn't matter with such ridiculous values in practice.

> Source/WebCore/ChangeLog:17
> +        No new tests (OOPS!).

OOPS. Better add a test! I'll help you.

> Source/WebCore/css/StyleResolver.cpp:4565
> +    // between Mac and Windows

Trailing period. Perhaps "to prevent changing behavior on other platforms".

> Source/WebCore/platform/graphics/win/FontCacheWin.cpp:563
> +    // All Windows fonts get's processed through here (SVG and CSS). Cap the font size to be between 0 and 1000000 here.

s/get's/get/

"processed" might not be the right word.

You might just remove this comment entirely, the code is pretty clear.

> Source/WebCore/platform/graphics/win/FontCacheWin.cpp:564
> +    float adjustedFontSize = fontDescription.computedPixelSize() > 1000000.0f || fontDescription.computedPixelSize() < 0 ? 1000000.0f : fontDescription.computedPixelSize();

s/adjusted/capped/? I'm not sure.

[Tim Horton]

<rdar://problem/8355401>

[Tim Horton]

I think you should look at moving all the capping into StyleBuilder.cpp's ApplyPropertyFontSize::applyValue.

[Roger Fong]

Created attachment 152826 [details]
Applies font size capping in StyleBuilder

[Tim Horton]

Comment on attachment 152826 [details]
Applies font size capping in StyleBuilder

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

> Source/WebCore/ChangeLog:8
> +        Timothy Horton <[e-mail]>

Fix this!

> Source/WebCore/ChangeLog:11
> +        font size in Windows, Webkit crashes. The problem has to do with 

WebKit has a capital K

> Source/WebCore/ChangeLog:16
> +        Test: svg/text/font-size-too-large-crash.svg

Yay! I think you need to svn add the test or something. And the results.

[Roger Fong]

Created attachment 152834 [details]
Repeat of last patch (fixed changelog)

[Tim Horton]

Failed to find the property value for the SVN property "svn:executable": "## -0,0 +1 ##

Wharr?

[Tim Horton]

Comment on attachment 152834 [details]
Repeat of last patch (fixed changelog)

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

> LayoutTests/ChangeLog:13
> +        * svg/text/font-size-too-large-expected.txt: Added.
> +        * svg/text/font-size-too-large.svg: Added.

You forgot to update these when you changed the filenames.

[Jessie Berlin]

(In reply to comment #8)
> Failed to find the property value for the SVN property "svn:executable": "## -0,0 +1 ##
> 
> Wharr?

svn propdel svn:executable svg/text/font-size-too-large.svg

[Roger Fong]

Created attachment 152845 [details]
Repeat of last patch (added a comment for clarification)

[Roger Fong]

Created attachment 152848 [details]
Repeat of last patch (I saved the file this time...)

[WebKit Review Bot]

Comment on attachment 152848 [details]
Repeat of last patch (I saved the file this time...)

Clearing flags on attachment: 152848

Committed r122896: <http://trac.webkit.org/changeset/122896>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.