Bug 25760 – Font leak in Chromium's Skia backend.

• Component: Platform
• Status: RESOLVED FIXED

[Evan Martin]

Valgrind found this:
http://code.google.com/p/chromium/issues/detail?id=9475

[Evan Martin]

Created attachment 30285 [details]
fix some stuff

 WebCore/ChangeLog                                  |   11 +++++++++++
 .../platform/graphics/chromium/FontCacheLinux.cpp  |    5 ++---
 2 files changed, 13 insertions(+), 3 deletions(-)

[Evan Martin]

Please note that I don't really know what I'm doing, so review carefully.

[Evan Martin]

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?

[Brett Wilson (Google)]

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

[Darin Fisher (:fishd, Google)]

Comment on attachment 30285 [details]
fix some stuff

> [phone]  Evan Martin  <[e-mail]>
> +
> +        Reviewed by Darin Fisher <[e-mail]>.
> +
> +        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!

[Evan Martin]

static_cast where?

[Evan Martin]

Created attachment 30294 [details]
fix

 WebCore/ChangeLog                                  |   11 +++++++++++
 .../platform/graphics/chromium/FontCacheLinux.cpp  |    5 ++---
 2 files changed, 13 insertions(+), 3 deletions(-)

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

review collision it would appear.  Stupid bugzilla.

[Evan Martin]

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...)

[Eric Seidel (no email)]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/platform/graphics/chromium/FontCacheLinux.cpp
Committed r43756