Bug 140378 - [SVG -> OTF Converter] Crashes when SVG font is invalid
Summary: [SVG -> OTF Converter] Crashes when SVG font is invalid
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-01-12 17:19 PST by Myles C. Maxfield
Modified: 2015-01-17 14:51 PST (History)
11 users (show)

See Also:


Attachments
Patch (2.16 KB, patch)
2015-01-12 17:24 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (5.40 KB, patch)
2015-01-15 21:10 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (5.43 KB, patch)
2015-01-15 21:20 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-mavericks-wk2 (1.05 MB, application/zip)
2015-01-15 22:09 PST, Build Bot
no flags Details
Archive of layout-test-results from ews100 for mac-mavericks (707.85 KB, application/zip)
2015-01-16 04:06 PST, Build Bot
no flags Details
Patch (5.41 KB, patch)
2015-01-16 18:49 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (7.53 KB, patch)
2015-01-17 10:40 PST, Myles C. Maxfield
koivisto: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2015-01-12 17:19:52 PST
[SVG -> OTF Converter] Crashes when SVG font is invalid
Comment 1 Myles C. Maxfield 2015-01-12 17:24:27 PST
Created attachment 244481 [details]
Patch
Comment 2 Myles C. Maxfield 2015-01-12 18:29:19 PST
Additional crashes. This sequence:

svg/text/hidpi-text-selection-rect-position.html
svg/text/highcharts-assertion.html
svg/text/inline-text-destroy-attributes-crash.xhtml
svg/text/kerning.svg
svg/text/lengthAdjust-text-metrics.html
svg/text/modify-text-node-in-tspan.html
svg/text/modify-tspan-position-bug.html
svg/text/multichar-glyph.svg
svg/text/non-bmp-positioning-lists.svg
svg/text/non-bmp-tspans.svg
svg/text/preserve-break-word.html
svg/text/remove-text-node-from-tspan.html
svg/text/remove-tspan-from-text.html
svg/text/scaled-font.svg
svg/text/scaling-font-with-geometric-precision.html
svg/text/select-text-inside-non-static-position.html
svg/text/select-text-svgfont.html
svg/text/select-textLength-spacing-squeeze-1.svg
svg/text/select-textLength-spacing-squeeze-2.svg
svg/text/select-textLength-spacing-squeeze-3.svg
svg/text/text-overflow-ellipsis-svgfont-kerning-ligatures.html

makes svg/text/text-overflow-ellipsis-svgfont-kerning-ligatures.html crash
Comment 3 Radar WebKit Bug Importer 2015-01-14 14:35:19 PST
<rdar://problem/19476310>
Comment 4 Myles C. Maxfield 2015-01-15 19:57:29 PST
This is because of the memory cache. Turns out that r176276 (which I wrote, see https://bugs.webkit.org/show_bug.cgi?id=138686) is completely busted.

CachedResources are, in fact, cached. This means that one page load might create a resource and cache it, and a secondary page load might pull the same thing from the cache. Therefore, a CachedResource subclass cannot assume that any state gets reset during a subsequent document load. For example, RefPtrs don't get set back to null. They may be referencing part of the previous document (even keeping it alive!)
Comment 5 Myles C. Maxfield 2015-01-15 20:35:33 PST
It's actually not that busted. It can be saved.
Comment 6 Myles C. Maxfield 2015-01-15 21:10:00 PST
Created attachment 244748 [details]
Patch
Comment 7 WebKit Commit Bot 2015-01-15 21:11:54 PST
Attachment 244748 [details] did not pass style-queue:


ERROR: Source/WebCore/loader/cache/CachedSVGFont.cpp:56:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/loader/cache/CachedSVGFont.cpp:56:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WebCore/loader/cache/CachedFont.h:60:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
ERROR: Source/WebCore/loader/cache/CachedFont.cpp:100:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 4 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Myles C. Maxfield 2015-01-15 21:20:25 PST
Created attachment 244750 [details]
Patch
Comment 9 WebKit Commit Bot 2015-01-15 21:22:03 PST
Attachment 244750 [details] did not pass style-queue:


ERROR: Source/WebCore/loader/cache/CachedSVGFont.cpp:56:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/loader/cache/CachedSVGFont.cpp:56:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WebCore/loader/cache/CachedSVGFont.cpp:105:  Line contains only semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
ERROR: Source/WebCore/loader/cache/CachedFont.h:60:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
ERROR: Source/WebCore/loader/cache/CachedFont.cpp:100:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 5 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Build Bot 2015-01-15 22:09:42 PST
Comment on attachment 244750 [details]
Patch

Attachment 244750 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5103001194725376

Number of test failures exceeded the failure limit.
Comment 11 Build Bot 2015-01-15 22:09:47 PST
Created attachment 244753 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 12 Build Bot 2015-01-16 04:06:12 PST
Comment on attachment 244750 [details]
Patch

Attachment 244750 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4764469322317824

Number of test failures exceeded the failure limit.
Comment 13 Build Bot 2015-01-16 04:06:22 PST
Created attachment 244761 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 14 Myles C. Maxfield 2015-01-16 18:49:37 PST
Created attachment 244831 [details]
Patch
Comment 15 WebKit Commit Bot 2015-01-16 18:50:52 PST
Attachment 244831 [details] did not pass style-queue:


ERROR: Source/WebCore/loader/cache/CachedSVGFont.cpp:56:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/loader/cache/CachedSVGFont.cpp:56:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WebCore/loader/cache/CachedSVGFont.cpp:105:  Line contains only semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
ERROR: Source/WebCore/loader/cache/CachedFont.h:60:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
ERROR: Source/WebCore/loader/cache/CachedFont.cpp:100:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 5 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 16 Antti Koivisto 2015-01-17 04:37:48 PST
Comment on attachment 244831 [details]
Patch

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

> Source/WebCore/loader/cache/CachedFont.cpp:100
> -bool CachedFont::ensureCustomFontData(RefPtr<SharedBuffer>&& data)
> +bool CachedFont::ensureCustomFontData(RefPtr<SharedBuffer> data)

RefPtr as argument is never correct. If this function takes ownership it should be RefPtr<SharedBuffer>&&. If it doesn't it should be SharedBuffer*.

> Source/WebCore/loader/cache/CachedSVGFont.cpp:65
> +    if (!externalSVG
> +#if ENABLE(SVG_OTF_CONVERTER)
> +        || firstFontFace(remoteURI)
>  #endif
> +        )
>          return CachedFont::getFontData(fontDescription, remoteURI, syntheticBold, syntheticItalic, externalSVG);
> +#if ENABLE(SVG_OTF_CONVERTER)
> +    else
> +        return nullptr;
> +#endif

This is difficult to follow. I'd suggest just having a full separate #if block for ENABLE(SVG_OTF_CONVERTER).

> Source/WebCore/loader/cache/CachedSVGFont.cpp:92
>          if (decoder->sawError())
>              m_externalSVGDocument = nullptr;
>  #if ENABLE(SVG_OTF_CONVERTER)
> -        firstFontFace(remoteURI); // Sets m_externalSVGFontElement
> +        else
> +            firstFontFace(remoteURI); // Sets m_externalSVGFontElement

Can you refactor this without dangling else on the other side of the #if?

Relying on a function side effect is confusing. There shouldn't be need to add a comment like this. Also firstFontFace() does work that gets thrown out.

> Source/WebCore/loader/cache/CachedSVGFont.cpp:97
>          if (m_externalSVGFontElement) {
>              Vector<char> convertedFont = convertSVGToOTFFont(*m_externalSVGFontElement);
> -            return CachedFont::ensureCustomFontData(SharedBuffer::adoptVector(convertedFont));
> -        }
> +            m_convertedFont = SharedBuffer::adoptVector(convertedFont);
> +        } else
> +            return false;

We prefer early return style:

if (!m_externalSVGFontElement)
   return false;
...

> Source/WebCore/loader/cache/CachedSVGFont.cpp:105
> +    return m_externalSVGDocument
> +#if ENABLE(SVG_OTF_CONVERTER)
> +        && CachedFont::ensureCustomFontData(m_convertedFont)
> +#endif
> +        ;

You can avoid #if in the middle of a statement either by early return or by using a boolean helper.
Comment 17 Myles C. Maxfield 2015-01-17 09:50:28 PST
Comment on attachment 244831 [details]
Patch

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

>> Source/WebCore/loader/cache/CachedFont.cpp:100
>> +bool CachedFont::ensureCustomFontData(RefPtr<SharedBuffer> data)
> 
> RefPtr as argument is never correct. If this function takes ownership it should be RefPtr<SharedBuffer>&&. If it doesn't it should be SharedBuffer*.

Hrm, that makes sense. Perhaps we can document this better? Or, better yet, make the compiler enforce this somehow?

Done.

>> Source/WebCore/loader/cache/CachedSVGFont.cpp:65
>> +#endif
> 
> This is difficult to follow. I'd suggest just having a full separate #if block for ENABLE(SVG_OTF_CONVERTER).

Done.

>> Source/WebCore/loader/cache/CachedSVGFont.cpp:92
>> +            firstFontFace(remoteURI); // Sets m_externalSVGFontElement
> 
> Can you refactor this without dangling else on the other side of the #if?
> 
> Relying on a function side effect is confusing. There shouldn't be need to add a comment like this. Also firstFontFace() does work that gets thrown out.

Yeah, you're right. Let me clean this up.

Done.

What do you mean by "Also firstFontFace() does work that gets thrown out."?

>> Source/WebCore/loader/cache/CachedSVGFont.cpp:97
>> +            return false;
> 
> We prefer early return style:
> 
> if (!m_externalSVGFontElement)
>    return false;
> ...

This is much cleaner!

Done.

>> Source/WebCore/loader/cache/CachedSVGFont.cpp:105
>> +        ;
> 
> You can avoid #if in the middle of a statement either by early return or by using a boolean helper.

In this case, early return would mean I'd have duplicate code because I want both code paths to return this thing. I think it would be better to just surround the entire return statement with the preprocessor macro (and having two return statements) rather than surrounding just part of the return statement.
Comment 18 Myles C. Maxfield 2015-01-17 10:40:00 PST
Created attachment 244841 [details]
Patch
Comment 19 Antti Koivisto 2015-01-17 12:10:38 PST
Comment on attachment 244841 [details]
Patch

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

> Source/WebCore/loader/cache/CachedSVGFont.cpp:65
>  #if !ENABLE(SVG_OTF_CONVERTER)
>      if (!externalSVG)
> -#endif
>          return CachedFont::getFontData(fontDescription, remoteURI, syntheticBold, syntheticItalic, externalSVG);
> +#else
> +    if (!externalSVG || firstFontFace(remoteURI))
> +        return CachedFont::getFontData(fontDescription, remoteURI, syntheticBold, syntheticItalic, externalSVG);
> +    return nullptr;
> +#endif
>  
>      if (SVGFontFaceElement* firstFontFace = this->firstFontFace(remoteURI))

ENABLE(SVG_OTF_CONVERTER) case always returns and so never uses the rest of the function body. You should put it all inside the first #if. Also I wonder why the ! case is the first.
Comment 20 Antti Koivisto 2015-01-17 12:12:07 PST
(In reply to comment #17)
> What do you mean by "Also firstFontFace() does work that gets thrown out."?

You never used the return value which was computed by a tree traversal.
Comment 21 Myles C. Maxfield 2015-01-17 14:29:44 PST
Comment on attachment 244841 [details]
Patch

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

>> Source/WebCore/loader/cache/CachedSVGFont.cpp:65
>>      if (SVGFontFaceElement* firstFontFace = this->firstFontFace(remoteURI))
> 
> ENABLE(SVG_OTF_CONVERTER) case always returns and so never uses the rest of the function body. You should put it all inside the first #if. Also I wonder why the ! case is the first.

Done.
Comment 22 Myles C. Maxfield 2015-01-17 14:51:44 PST
Committed r178628: <http://trac.webkit.org/changeset/178628>