I found there are leaking SimpleFontData objects in QtWebKit.
That's because the result of FontFallbackList::fontDataAt
is not freed. I made FontData, SimpleFontData and SegmentedFontData
ref-counted and used RefPtr-s when ownership is passed.
The patch are not complete yet because other ports are not compile with it.
Our measurements shows it has a very good affect on memory usage at least
in the case of Qt port.
Created attachment 28536 [details]
Created attachment 28684 [details]
Extend the first patch with doing a delete in
FontCache::getCachedFontData(const FontPlatformData* data) for the parameter
because without this it is leaking.
Comment on attachment 28684 [details]
I am not convinced this is necessary. If the bug is Qt-specific, it seems like Qt should come more in line with the other platforms rather than making all the other platforms use refcounting unnecessarily.
Created attachment 28928 [details]
New solution what solves the problem internally.
Comment on attachment 28928 [details]
This looks ok to me. Typo in ChangeLog... "objets" should "objects"
Created attachment 28931 [details]
Typo in Changelog (and call zecke for review).
Comment on attachment 28931 [details]
> FontPlatformData* FontCache::getCachedFontPlatformData(const FontDescription& description, const AtomicString& family, bool checkingAlternateName)
> - return new FontPlatformData(description);
> + return 0;
This will lead to a null pointer dereference in CSSFontFaceSource::getFontData.
I see. We can solve it locally like checking the pointer and if it is 0 then creating a new FontPlatformData object. If it is a unique situation then it would be enough. If it is just an example of how can bad happens then we must
find a more general solution. I guess it can be unique because the patch worked form me (I could browse with QtLauncher without segfault after applied it).
I see two possibilities: 1. introduce a macro like USE_FONTCACHE and add #ifdef-s when it is necessary
2. implement a FontCache that provides the needed interface without leaking
So, what do you prefer? Local solution, one of the two above, or something else?
Anyway, I think we must not return with a pointer of a newly created and instantly released object from methods of FontCache.
Created attachment 28966 [details]
Local fix in WebCore::CSSFontFaceSource::getFontData
(In reply to comment #9)
grep says we do not have to do more checks like this so I believe the patch is right
Ops, in a more fully browsing I get segfault, so I must clear the review flag.
Things get wrong in method(s) of SVGFont.
It is because returning with zero from FontCache::getCachedFontPlatformData.
The leak can also destroy through returning from this with a new object and delete it in FontCache::getCachedFontData (it gets it as a parameter) but I think it is silly so I try to find a better solution.
Created attachment 29158 [details]
At last ready, not leaking, not crashing.
Created attachment 29228 [details]
proposed patch - more clever solution
There is a logical mistake in the previous patch: FontFallbackList::fontDataAt gives back the SimpleFontData what has been created the first time without taking care about what FontDescription the Font what it gets as parameter has. I am not sure it is really a problem because the outputs of LayoutTests are equivalents of what they are with a clean TOT build. I tried the tests in css1/font_properties and in fonts by load and see them (because they fail even with the clean build). However I made another solution what does not suffer from this problem by transplanted some of the logic from FontCache.cpp to FontFallbackListQt.cpp and made a mapping from FontDescription to the SimpleFontData what we create and give back. I am not sure about what copyright and license should I use for the new file and where should I put it - let we discuss about it if you like the patch. Now there are 2 different solution for the same problem - you can choose what is better. I would be very happy if we could fix and close the bug somehow.
Created attachment 29277 [details]
proposed patch - more clever solution
Same as the previous with typos, copyright and Changelog.
Created attachment 29815 [details]
proposed patch - the simpler one
Ifdefing the changes in CSSFontFaceSource
Created attachment 29816 [details]
proposed patch - the caching one
Ifdefing changes in CSSFontFaceSource.cpp
As discussed with Kelemen on IRC, I splitted the patch into two parts.
The first part is landed in r43828:
The second part is a simplification of the caching method. It is landed in r43830: