Bug 25760 - Font leak in Chromium's Skia backend.
Summary: Font leak in Chromium's Skia backend.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Eric Seidel (no email)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-05-13 14:09 PDT by Evan Martin
Modified: 2009-05-15 00:02 PDT (History)
2 users (show)

See Also:


Attachments
fix some stuff (1.56 KB, patch)
2009-05-13 14:13 PDT, Evan Martin
fishd: review-
Details | Formatted Diff | Diff
fix (1.54 KB, patch)
2009-05-13 16:13 PDT, Evan Martin
fishd: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Evan Martin 2009-05-13 14:09:00 PDT
Valgrind found this:
http://code.google.com/p/chromium/issues/detail?id=9475
Comment 1 Evan Martin 2009-05-13 14:13:14 PDT
Created attachment 30285 [details]
fix some stuff

 WebCore/ChangeLog                                  |   11 +++++++++++
 .../platform/graphics/chromium/FontCacheLinux.cpp  |    5 ++---
 2 files changed, 13 insertions(+), 3 deletions(-)
Comment 2 Evan Martin 2009-05-13 14:14:51 PDT
Please note that I don't really know what I'm doing, so review carefully.
Comment 3 Evan Martin 2009-05-13 14:28:16 PDT
I apologize for not quite getting the review process right, but from some offline discussion it appears Brett is the right reviewer.

Brett, agl wrote the code but he'd never seen these getCachedFont* bits.  As our font man, what do you think?
Comment 4 Brett Wilson (Google) 2009-05-13 15:24:05 PDT
Comment on attachment 30285 [details]
fix some stuff

I believe this is the way it's supposed to work, it's what we do on Windows.

> -            FontPlatformData* fpd =
> -                createFontPlatformData(font.fontDescription(), AtomicString((char*) family));
> -            ret = new SimpleFontData(*fpd);
> +            FontPlatformData* fpd = getCachedFontPlatformData(font.fontDescription(), AtomicString((char*) family), false);
> +            ret = getCachedFontData(fpd);

Is it WebKit style to require static_cast?

Brett
Comment 5 Darin Fisher (:fishd, Google) 2009-05-13 15:54:20 PDT
Comment on attachment 30285 [details]
fix some stuff

> +2009-05-13  Evan Martin  <evan@chromium.org>
> +
> +        Reviewed by Darin Fisher <darin@chromium.org>.
> +
> +        Fix a font-related leak in Chromium's Skia backend found by Valgrind.
> +        http://code.google.com/p/chromium/issues/detail?id=9475

Please add a reference to the bugs.webkit.org bug instead.  In that bug you
should/may refer to the chromium bug.


yes, static_cast for the win!
Comment 6 Evan Martin 2009-05-13 16:04:06 PDT
static_cast where?
Comment 7 Evan Martin 2009-05-13 16:13:47 PDT
Created attachment 30294 [details]
fix

 WebCore/ChangeLog                                  |   11 +++++++++++
 .../platform/graphics/chromium/FontCacheLinux.cpp  |    5 ++---
 2 files changed, 13 insertions(+), 3 deletions(-)
Comment 8 Eric Seidel (no email) 2009-05-13 16:25:01 PDT
Comment on attachment 30294 [details]
fix

WebKit style guidelines:
"Use full words, except in the rare case where an abbreviation would be more canonical and easier to understand."

I would re-write this block:
+            FontPlatformData* fpd = getCachedFontPlatformData(font.fontDescription(), AtomicString(reinterpret_cast<char*>(family)), false);
+            ret = getCachedFontData(fpd);

as:

AtomicString fontFamily(reinterpret_cast<char*>(family));
ret = getCachedFontData(getCachedFontPlatformData(font.fontDescription(), fontFamily), false)

or as platformData =

Otherwise looks fine.  I can fix it an land for you.
Comment 9 Eric Seidel (no email) 2009-05-13 16:30:01 PDT
review collision it would appear.  Stupid bugzilla.
Comment 10 Evan Martin 2009-05-13 16:42:28 PDT
Please feel free to munge at will.  (In my defense, note that both the C-style cast and the "ret" were there before my change...)
Comment 11 Eric Seidel (no email) 2009-05-15 00:02:43 PDT
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/platform/graphics/chromium/FontCacheLinux.cpp
Committed r43756