Bug 140378 – [SVG -> OTF Converter] Crashes when SVG font is invalid

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

[SVG -> OTF Converter] Crashes when SVG font is invalid

[Myles C. Maxfield]

Created attachment 244481 [details]
Patch

[Myles C. Maxfield]

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

[Radar WebKit Bug Importer]

<rdar://problem/19476310>

[Myles C. Maxfield]

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!)

[Myles C. Maxfield]

It's actually not that busted. It can be saved.

[Myles C. Maxfield]

Created attachment 244748 [details]
Patch

[WebKit Commit Bot]

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.

[Myles C. Maxfield]

Created attachment 244750 [details]
Patch

[WebKit Commit Bot]

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.

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Myles C. Maxfield]

Created attachment 244831 [details]
Patch

[WebKit Commit Bot]

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.

[Antti Koivisto]

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.

[Myles C. Maxfield]

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.

[Myles C. Maxfield]

Created attachment 244841 [details]
Patch

[Antti Koivisto]

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.

[Antti Koivisto]

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

[Myles C. Maxfield]

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.

[Myles C. Maxfield]

Committed r178628: <http://trac.webkit.org/changeset/178628>