Bug 24551

Summary: [Qt] leaks in font-handling
Product: WebKit Reporter: Balazs Kelemen <kbalazs>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ariya.hidayat, hausmann, hyatt, laszlo.gombos, zecke
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
proposed patch
none
proposed patch
hyatt: review-
proposed patch
none
proposed patch
none
proposed patch
none
proposed patch
none
proposed patch - more clever solution
none
proposed patch - more clever solution
none
proposed patch - the simpler one
none
proposed patch - the caching one none

Description Balazs Kelemen 2009-03-12 11:04:22 PDT
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.
Comment 1 Balazs Kelemen 2009-03-12 11:07:32 PDT
Created attachment 28536 [details]
proposed patch
Comment 2 Balazs Kelemen 2009-03-17 01:29:59 PDT
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 3 Dave Hyatt 2009-03-17 11:07:40 PDT
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.
Comment 4 Balazs Kelemen 2009-03-25 09:39:58 PDT
Created attachment 28928 [details]
proposed patch

New solution what solves the problem internally.
Comment 5 Dave Hyatt 2009-03-25 09:55:29 PDT
Comment on attachment 28928 [details]
proposed patch

This looks ok to me.  Typo in ChangeLog... "objets" should "objects"
Comment 6 Balazs Kelemen 2009-03-25 10:06:58 PDT
Created attachment 28931 [details]
proposed patch

Typo in Changelog (and call zecke for review).
Comment 7 Holger Freyther 2009-03-26 02:43:47 PDT
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.
Comment 8 Balazs Kelemen 2009-03-26 04:20:04 PDT
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.
Comment 9 Balazs Kelemen 2009-03-26 05:57:28 PDT
Created attachment 28966 [details]
proposed patch

Local fix in WebCore::CSSFontFaceSource::getFontData
Comment 10 Balazs Kelemen 2009-03-26 06:07:18 PDT
(In reply to comment #9)
grep says we do not have to do more checks like this so I believe the patch is right
Comment 11 Balazs Kelemen 2009-03-26 10:35:39 PDT
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. 
Comment 12 Balazs Kelemen 2009-04-01 06:15:52 PDT
Created attachment 29158 [details]
proposed patch

At last ready, not leaking, not crashing.
Comment 13 Balazs Kelemen 2009-04-03 09:36:38 PDT
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.
Comment 14 Balazs Kelemen 2009-04-06 02:12:37 PDT
Created attachment 29277 [details]
proposed patch - more clever solution

Same as the previous with typos, copyright and Changelog.
Comment 15 Balazs Kelemen 2009-04-27 02:39:26 PDT
Created attachment 29815 [details]
proposed patch - the simpler one

Ifdefing the changes in CSSFontFaceSource
Comment 16 Balazs Kelemen 2009-04-27 02:50:57 PDT
Created attachment 29816 [details]
proposed patch - the caching one

Ifdefing changes in CSSFontFaceSource.cpp
Comment 17 Ariya Hidayat 2009-05-18 07:14:46 PDT
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