This bug report originated from issue QTBUG-4433 <http://bugreports.qt.nokia.com/browse/QTBUG-4433> --- Description --- WebKit crashes when loading certain SVG images
(In reply to comment #0) > This bug report originated from issue QTBUG-4433 > <http://bugreports.qt.nokia.com/browse/QTBUG-4433> > > --- Description --- > > WebKit crashes when loading certain SVG images Please make sure the URL is accessible.
Created attachment 39985 [details] Testcase
Thanks for triaging Charles. This bug made its way though 3 different bug trackers before ending up here, upstream, so the original content is not always retained. The test-case was attached to the original bug report. As for the bugreports.qt.nokia.com domain, it will go live in about a week.
Reproduced on trunk.
This did not crash for me on Safari Mac tip of tree just now. r49195
Created attachment 40954 [details] Make Qt uses the common FontFallbackList() and make sure a font is always returned This bug can only be reproduced if the SVG is used as an image in a HTML page. The font is asked without a family, the family name is NULL, we call the selector directly on it -> Webkit crashes. The other ports do not crash because of the test: if (currFamily->family().length()) { in FontCache. Instead of limiting the fix to the FontFallbackList, I have modified the way we handle font in Qt to be similar to the other ports (so we benefit from the patches made to FontFallbackList). Without the refactoring, the patch would be as simple as that: diff --git a/WebCore/platform/graphics/qt/FontFallbackListQt.cpp b/WebCore/platform/graphics/qt/FontFallbackListQt.cpp index 8e1e4f6..0306abf 100644 --- a/WebCore/platform/graphics/qt/FontFallbackListQt.cpp +++ b/WebCore/platform/graphics/qt/FontFallbackListQt.cpp @@ -102,7 +102,7 @@ const FontData* FontFallbackList::fontDataAt(const WebCore::Font* _font, unsigne const FontDescription& description = _font->fontDescription(); const FontFamily* family = &description.family(); while (family) { - if (m_fontSelector) { + if (family->family().length() && m_fontSelector) { FontData* data = m_fontSelector->getFontData(description, family->family()); if (data) { if (data->isLoading()) I have not made the test case yet.
We can use the WebCore FontCache without going through the hassle of having GlyphNode* implemented for Qt? What is the memory impact of this? We didn't use the FontCache as Font Caching is already implemented by Qt and having two caches didn't seem like a good idea.
(In reply to comment #7) > We can use the WebCore FontCache without going through the hassle of having > GlyphNode* implemented for Qt? If I understand correctly, FontCache does not store/cache the glyphs. It caches the relation between the WebCore::Font and the FontPlatformData. Most of the previous code was copied from FontFallbackList. The only advantage I could find on the previous implementation was to avoid the call to FontCache::getFontDataForCharacters(). I am new to this code so I might have missed something. > What is the memory impact of this? We didn't use the FontCache as Font Caching > is already implemented by Qt and having two caches didn't seem like a good > idea. The only difference for the memory usage is the new "bool m_isDeletedValue;" in FontPlatformData. If this is a problem I can use a special value of the size like in the previous implementation (FontPlatformDataCacheKey). I have added the bool because I think the size trick is less clear.
Created attachment 41417 [details] The patch with a test This is the same patch as 40954 with a test.
Created attachment 41418 [details] Patch with test case Same patch, just a minor change to follow the coding convention.
This seems a bit more major than just a small SVG crash fix. It's nice that we're fixing the crasher,but it seems we're also finally moving Qt onto a font system closer to the other ports... SEems the ChangeLog needs a lot more explanation as to what you're doing here!
Created attachment 41497 [details] Patch with test case I have added more comments in the ChangeLog to describe how the new implementation differs from the old one. I do not know if I should describe how this patch fixes 29856? The fix is mostly a by-product of using the common FontCache and FontFallbackList.
can you please git/svn blame the original code, and cc the original authors to this bug?
(In reply to comment #13) > can you please git/svn blame the original code, and cc the original authors to > this bug? The original author is Zecke, he is already in the CC list of this bug.
Comment on attachment 41497 [details] Patch with test case > -FontPlatformData* FontCache::getLastResortFallbackFont(const FontDescription&) > +const SimpleFontData* FontCache::getFontDataForCharacters(const Font& font, > + const UChar* characters, > + int length) > { > + UNUSED_PARAM(font); > + UNUSED_PARAM(characters); > + UNUSED_PARAM(length); > return 0; Omit the parameters and remove UNUSED_PARAM. Classically UNUSED_PARAM is only supposed to be used in C code were omitting the parameter is not legal. > > + FontPlatformData(WTF::HashTableDeletedValueType) : m_isDeletedValue(true) { } The coding style says m_isDeletedValue should be on the next line. > > #ifndef NDEBUG > String description() const; > @@ -77,6 +94,7 @@ public: > float m_size; > bool m_bold; > bool m_oblique; > + bool m_isDeletedValue; > QFont m_font; > }; Please look at the packing of this structure. Maybe we can reorder to save a byte per instance? :)
Created attachment 41569 [details] Patch with test case Thanks Holger for the comments.
Comment on attachment 41569 [details] Patch with test case It seems svg/text/resources/text-font-invalid.svg could be dumpAsText. It's just a crash test, no? It also should have some sort of <text>PASSED -- this test did not crash</text> Unintentional change? WebCore/platform/graphics/qt/FontPlatformData.h 1 /* 1 /* Is this what you want? 80 unsigned hash() const 81 { 82 return qHash(m_font.toString()); 83 } Does that correctly handle smallCaps and italic, etc? I'm not convinced that hash() and == are equivalent. If they are not, that could cause badness! In general this looks fine though.
Comment on attachment 41569 [details] Patch with test case Looks good to me.
Comment on attachment 41569 [details] Patch with test case Benjamin needs to verify changes in memory usage before this can be landed.
Ah, sorry. Didn't see the r-.
Created attachment 42096 [details] Patch with test case Here are the changes: -Change the tests as a dumpAsText (thanks Eric) -Remove the accidental space (thanks Eric) -fix the hash function, Eric is right, there are cases where the font.toString() do not follow the attributes of FontPlatformData Memory usage as reported by memusagestat (thanks Holger): without patch: MAX: total: 391554162 stack: 78384 heap: 391537982 with patch: MAX: total: 241243358 stack: 78912 heap: 241232030 Other questions: "Does m_font.toString() that correctly handle smallCaps and italic, etc?" (Eric) Yep, toString() serialize all the parameters of the font: http://doc.trolltech.com/4.6-snapshot/qfont.html#toString
Created attachment 42097 [details] Memory footprint of the cycler test (without the patch)
Created attachment 42098 [details] Memory footprint of the cycler test (with patch)
Created attachment 42101 [details] Memory footprint of the cycler test without the last test (without patch)
Created attachment 42102 [details] Memory footprint of the cycler test without the last test (with patch) With patch: MAX: total: 75229876 stack: 76544 heap: 75198548 Without patch: MAX: total: 81395638 stack: 76544 heap: 81361830
Feel free to land. Nice cleanup and progress on memory usage!!!! The last test is a real tough corner case and it is best to execute it in something like Xephyr (to make sure the window has focus and is redrawn) as the images are only decoded when they are drawn. From looking at the picture I was not sure if the tst_cycler was ran in the same way.
Comment on attachment 42096 [details] Patch with test case Excellent! Then changelog for text-font-invalid-expected.txt refers to the wrong directory, but I'll fix that when landing manually.
Committed r50496: <http://trac.webkit.org/changeset/50496>
This was crashing on the bot in QFont and was reverted in r50511.
Comment on attachment 42096 [details] Patch with test case Marking this r- since it was reverted.
Any update on this? Was the patch at least cherry-picked into Qt 4.6? so that we can stop blocking on it?
Created attachment 43356 [details] Simple patch to fix the SVG crash This is the simple version of the patch: copy the missing code from the common FontCache. The complete refactoring will come later when I am sure it does not have any regression.
Comment on attachment 43356 [details] Simple patch to fix the SVG crash Great Benjamin
(In reply to comment #31) > Any update on this? > > Was the patch at least cherry-picked into Qt 4.6? so that we can stop blocking > on it? Here is what went wrong with the refactoring and Qt 4.5: The WTF::HashTable is creating the empty FontPlatformData from a memory area filled with zeros (because of the emptyValueIsZero = true; in the trait defined for this hashmap). This fails with Qt 4.5 because the d-pointer of QFont must never be null. On Qt 4.6, the comparisons works because of the changes for exception-safety. I am going to improve PlatfromDataQt to work with zero-initializations and re-submit the refactoring.
Comment on attachment 43356 [details] Simple patch to fix the SVG crash Clearning review for manual landing
Comment on attachment 43356 [details] Simple patch to fix the SVG crash r-, the ChangeLog is broken as it talks about custom printing shrink factors and links to the wrong bugzilla url
Created attachment 43420 [details] Simple patch to fix the SVG crash I messed up the Changelog again :(
Comment on attachment 43420 [details] Simple patch to fix the SVG crash Landed attachment 43420 [details] in rr51103
Removing from the 4.6 blocker list. The simple fix has been integrated into the trunk and qtwebkit-4.6 and if all goes well it'll make it into 4.6.0, too.
We will need the FontCache unification as well. From Benjamin's finding we consume less memory, from my finding it is likely due us creating less FontPlatformData.. which means less usage of FontConfig in Qt and we don't pass stuff like "-webkit-*" as font name which all leads to a nice speedup on font config. So the FontCache change turns up to be a priority to me. Benjamin are you working on it or should I have a go?
Created attachment 44277 [details] Refactoring of the font cache to use the common FontCache This is the patch updated to work on Qt 4.5 and superior. I use a d-pointer in order to keep the PlatformFont as small as possible. With this patch, the cycler test uses 7% less memory. The tests are running, I will finish the patch tomorrow with the results and the changelog.
Have you considered just using a QFont* and then in the copy c'tor create a new QFont? Besides that great work!
(In reply to comment #42) > Have you considered just using a QFont* and then in the copy c'tor create a new > QFont? Besides that great work! That was my first idea, but I have avoided that on purpose. The copy operations on QFont are atomic, which is not necessary in this case (from what I have seen, FontCache is never accessed from multiple threads). Since the atomic operation is quite slow, and the copy is common in this hashmap, I wanted to avoid the AtomicInt and use a regular integer for the ref counting.
Created attachment 44299 [details] Refactoring of the font cache to use the common FontCache Three tests are failing with this patch. The layout is a bit different (2 to 4 pixels smaller), the problem seems related to the family problem. Should I send another patch to change the result of those tests or we wait for the proper fix for the font family before updating the tests?
Attachment 44299 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/platform/graphics/qt/FontPlatformData.h:35: This { should be at the end of the previous line [whitespace/braces] [4] Skipping input 'WebCore/platform/graphics/qt/FontFallbackListQt.cpp': Can't open for reading WebCore/platform/graphics/qt/FontPlatformDataQt.cpp:31: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/platform/graphics/qt/FontPlatformDataQt.cpp:36: One line control clauses should not use braces. [whitespace/braces] [4] WebCore/platform/graphics/qt/FontPlatformDataQt.cpp:56: Declaration has space between type name and & in QFont &font [whitespace/declaration] [3] WebCore/platform/graphics/qt/FontPlatformDataQt.cpp:96: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] Total errors found: 5
Created attachment 44302 [details] Refactoring of the font cache to use the common FontCache Same as the previous patch, but use webkit coding style instead of the Qt coding style.
style-queue ran check-webkit-style on attachment 44302 [details] without any errors.
Created attachment 44303 [details] Refactoring of the font cache to use the common FontCache Update the Changelog. Re-introduce "size == 0.f" because "!size" was confusing.
Created attachment 44304 [details] Refactoring of the font cache to use the common FontCache I had uploaded the wrong file.
Attachment 44304 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 Skipping input 'WebCore/platform/graphics/qt/FontFallbackListQt.cpp': Can't open for reading WebCore/platform/graphics/qt/FontPlatformDataQt.cpp:31: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 1
(In reply to comment #49) > Created an attachment (id=44304) [details] > Refactoring of the font cache to use the common FontCache > > I had uploaded the wrong file. +FontPlatformData::FontPlatformData(float size, bool bold, bool oblique) +{ + if (!bold && !oblique && size == 0.f) benjamin, you might want 'qFuzzyCompare' here ?
(In reply to comment #51) > +FontPlatformData::FontPlatformData(float size, bool bold, bool oblique) > +{ > + if (!bold && !oblique && size == 0.f) > > > benjamin, you might want 'qFuzzyCompare' here ? In this case I don't want the fuzzy compare. This test is only for the empty value as defined by the trait : "DEFINE_STATIC_LOCAL(FontPlatformData, key, (0.f, false, false));" (in FontCache.cpp). Anything different from this should not be an empty value.
(In reply to comment #52) > (In reply to comment #51) > > +FontPlatformData::FontPlatformData(float size, bool bold, bool oblique) > > +{ > > + if (!bold && !oblique && size == 0.f) > > > > > > benjamin, you might want 'qFuzzyCompare' here ? > > In this case I don't want the fuzzy compare. > > This test is only for the empty value as defined by the trait : > "DEFINE_STATIC_LOCAL(FontPlatformData, key, (0.f, false, false));" (in > FontCache.cpp). > > Anything different from this should not be an empty value. Actually, then I think it would be a good idea to add that comment about the comparison.
(In reply to comment #53) > Actually, then I think it would be a good idea to add that comment about the > comparison. There is a comment on the next line : if (!bold && !oblique && size == 0.f) // this is the empty value by definition of the trait FontDataCacheKeyTraits
Created attachment 44305 [details] Refactoring of the font cache to use the common FontCache
Attachment 44305 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 Skipping input 'WebCore/platform/graphics/qt/FontFallbackListQt.cpp': Can't open for reading WebCore/platform/graphics/qt/FontPlatformDataQt.cpp:32: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 1
Created attachment 44306 [details] Update the layout tests Update the test's results changed by the patch. With the patch, "bold", and "size" are always fully coherent with the font's data. We still don't take into account the family, Tor Arne is working on a solution for another bug.
style-queue ran check-webkit-style on attachment 44306 [details] without any errors.
Comment on attachment 44306 [details] Update the layout tests LAck of ChangeLog
Created attachment 44308 [details] Update the layout tests I forgot the changelog in the previous version. Thanks Kenneth for noticing.
Comment on attachment 44305 [details] Refactoring of the font cache to use the common FontCache Clearing flags on attachment: 44305 Committed r51699: <http://trac.webkit.org/changeset/51699>
Comment on attachment 44308 [details] Update the layout tests Clearing flags on attachment: 44308 Committed r51700: <http://trac.webkit.org/changeset/51700>
All reviewed patches have been landed. Closing bug.
cool stuff. landed patches did much more than what the bug title says. I would change in accordingly for search/reference in the future :)