| Summary: | [SVG -> OTF Converter] Crashes when SVG font is invalid | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Myles C. Maxfield <mmaxfield> | ||||||||||||||||
| Component: | New Bugs | Assignee: | Myles C. Maxfield <mmaxfield> | ||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||
| Severity: | Normal | CC: | buildbot, commit-queue, dino, japhet, jonlee, kling, koivisto, rniwa, simon.fraser, thorton, webkit-bug-importer | ||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Myles C. Maxfield
2015-01-12 17:19:52 PST
Created attachment 244481 [details]
Patch
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 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!) It's actually not that busted. It can be saved. Created attachment 244748 [details]
Patch
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.
Created attachment 244750 [details]
Patch
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 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. 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 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. 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
Created attachment 244831 [details]
Patch
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 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 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. Created attachment 244841 [details]
Patch
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. (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 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. Committed r178628: <http://trac.webkit.org/changeset/178628> |