Summary: | [Qt] leaks in font-handling | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Balazs Kelemen <kbalazs> | ||||||||||||||||||||||
Component: | Platform | Assignee: | 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
Balazs Kelemen
2009-03-12 11:04:22 PDT
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 |