Bug 118532 - [Qt] memory leak in WebCore::FontCache::getLastResortFallbackFont
Summary: [Qt] memory leak in WebCore::FontCache::getLastResortFallbackFont
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
Depends on: 119740
Blocks: 110211
  Show dependency treegraph
Reported: 2013-07-10 09:07 PDT by Fabienne Semeria
Modified: 2013-08-15 09:22 PDT (History)
7 users (show)

See Also:

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

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]
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]

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>