Bug 24551 – [Qt] leaks in font-handling

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Balazs Kelemen]

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.

[Balazs Kelemen]

Created attachment 28536 [details]
proposed patch

[Balazs Kelemen]

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.

[Dave Hyatt]

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.

[Balazs Kelemen]

Created attachment 28928 [details]
proposed patch

New solution what solves the problem internally.

[Dave Hyatt]

Comment on attachment 28928 [details]
proposed patch

This looks ok to me.  Typo in ChangeLog... "objets" should "objects"

[Balazs Kelemen]

Created attachment 28931 [details]
proposed patch

Typo in Changelog (and call zecke for review).

[Holger Freyther]

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.

[Balazs Kelemen]

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.

[Balazs Kelemen]

Created attachment 28966 [details]
proposed patch

Local fix in WebCore::CSSFontFaceSource::getFontData

[Balazs Kelemen]

(In reply to comment #9)
grep says we do not have to do more checks like this so I believe the patch is right

[Balazs Kelemen]

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. 

[Balazs Kelemen]

Created attachment 29158 [details]
proposed patch

At last ready, not leaking, not crashing.

[Balazs Kelemen]

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.

[Balazs Kelemen]

Created attachment 29277 [details]
proposed patch - more clever solution

Same as the previous with typos, copyright and Changelog.

[Balazs Kelemen]

Created attachment 29815 [details]
proposed patch - the simpler one

Ifdefing the changes in CSSFontFaceSource

[Balazs Kelemen]

Created attachment 29816 [details]
proposed patch - the caching one

Ifdefing changes in CSSFontFaceSource.cpp

[Ariya Hidayat]

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