(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.
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.
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.
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.
(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? :)
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.
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 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.
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.
(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
r-, the ChangeLog is broken as it talks about custom printing shrink factors and links to the wrong bugzilla url
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.
(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.
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.
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
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.
2009-09-23 03:42 PDT, Tor Arne Vestbø
2009-10-09 10:55 PDT, Benjamin Poulain
2009-10-19 08:03 PDT, Benjamin Poulain
2009-10-19 08:33 PDT, Benjamin Poulain
2009-10-20 04:54 PDT, Benjamin Poulain
2009-10-21 09:24 PDT, Benjamin Poulain
2009-10-29 07:42 PDT, Benjamin Poulain
2009-10-29 07:51 PDT, Benjamin Poulain
2009-10-29 07:51 PDT, Benjamin Poulain
2009-10-29 08:26 PDT, Benjamin Poulain
2009-10-29 08:27 PDT, Benjamin Poulain
2009-11-17 05:47 PST, Benjamin Poulain
2009-11-18 02:22 PST, Benjamin Poulain
2009-12-03 16:52 PST, Benjamin Poulain
2009-12-04 03:13 PST, Benjamin Poulain
2009-12-04 04:14 PST, Benjamin Poulain
2009-12-04 04:30 PST, Benjamin Poulain
2009-12-04 04:33 PST, Benjamin Poulain
2009-12-04 05:12 PST, Benjamin Poulain
2009-12-04 05:22 PST, Benjamin Poulain
kenneth: commit-queue-
2009-12-04 05:33 PST, Benjamin Poulain