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] proposed patch
Created attachment 28684 [details] proposed patch 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] proposed patch 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] proposed patch New solution what solves the problem internally.
Comment on attachment 28928 [details] proposed patch This looks ok to me. Typo in ChangeLog... "objets" should "objects"
Created attachment 28931 [details] proposed patch Typo in Changelog (and call zecke for review).
Comment on attachment 28931 [details] proposed patch > 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] proposed patch 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] proposed patch 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: http://trac.webkit.org/changeset/43828 The second part is a simplification of the caching method. It is landed in r43830: http://trac.webkit.org/changeset/43830