Bug 118532 - [Qt] memory leak in WebCore::FontCache::getLastResortFallbackFont
Summary: [Qt] memory leak in WebCore::FontCache::getLastResortFallbackFont
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Allan Sandfeld Jensen
URL:
Keywords:
Depends on: 119740
Blocks: 110211
  Show dependency tree / graph
 
Reported: 2013-07-10 09:07 PDT by Fabienne Semeria
Modified: 2013-08-15 09:22 PDT (History)
7 users (show)

See Also:

Attachments
fix memory leak in FontCache::getLastResortFallbackFont (1.46 KB, patch)
2013-07-10 09:12 PDT, Fabienne Semeria 		no flags Details | Formatted Diff | Diff
Patch (1.71 KB, patch)
2013-08-13 05:22 PDT, Allan Sandfeld Jensen 		jturcotte: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Fabienne Semeria 2013-07-10 09:07:48 PDT 
memory leak in WebCore::FontCache::getLastResortFallbackFont
Comment 1 Fabienne Semeria 2013-07-10 09:12:27 PDT 
Created attachment 206393 [details]
fix memory leak in FontCache::getLastResortFallbackFont
Comment 2 WebKit Commit Bot 2013-07-11 03:29:39 PDT 
Comment on attachment 206393 [details]
fix memory leak in FontCache::getLastResortFallbackFont

Clearing flags on attachment: 206393

Committed r152563: <http://trac.webkit.org/changeset/152563>
Comment 3 WebKit Commit Bot 2013-07-11 03:29:42 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 4 Alex 2013-07-23 05:53:33 PDT 
It looks like after applying this patch any QtWebKit-based browser crashing on loading this site: http://www.kevs3d.co.uk/dev/canvasmark
Tested on Ubuntu 12.04 64bit
Comment 5 Fabienne Semeria 2013-07-23 09:06:30 PDT 
Indeed, it looks like the Qt version of FontCache::getLastResortFallbackFont may now return a null pointer in some cases. It may be safer to revert the patch until a fix for this is available.
Comment 6 WebKit Commit Bot 2013-08-13 04:44:28 PDT 
Re-opened since this is blocked by bug 119740
Comment 7 Allan Sandfeld Jensen 2013-08-13 05:22:09 PDT 
Created attachment 208619 [details]
Patch
Comment 8 Fabienne Semeria 2013-08-13 05:37:06 PDT 
(In reply to comment #7)
> Created an attachment (id=208619) [details]
> Patch

We tested a patch similar to this one, and it works for us.
Comment 9 Allan Sandfeld Jensen 2013-08-13 05:41:18 PDT 
(In reply to comment #8)
> (In reply to comment #7)
> > Created an attachment (id=208619) [details] [details]
> > Patch
> 
> We tested a patch similar to this one, and it works for us.

Great. We also had a case where the old fix crashed Qt browser-demo on 32bit, and the new version doesn't. I never got Alex' example to crash though, can anyone confirm that also works?
Comment 10 Milian Wolff 2013-08-14 04:53:17 PDT 
This patch works fine for me (see bug 119740 and bug 119088).
Comment 11 Jocelyn Turcotte 2013-08-15 06:37:09 PDT 
Comment on attachment 208619 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=208619&action=review

> Source/WebCore/ChangeLog:8
> +        Allocate FontPlatformData on the stack instead on the heap.

A note that getCachedFontData does a deep copy of FontPlatformData could be useful to explain why this change is necessary.
Comment 12 Allan Sandfeld Jensen 2013-08-15 09:22:55 PDT 
Committed r154103: <http://trac.webkit.org/changeset/154103>