Bug 99430 - hb_face_t instances should not depend on FontPlatformData
Summary: hb_face_t instances should not depend on FontPlatformData
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kenichi Ishibashi
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-10-16 00:50 PDT by Kenichi Ishibashi
Modified: 2012-10-16 02:28 PDT (History)
2 users (show)

See Also:

Attachments
Patch (5.65 KB, patch)
2012-10-16 01:19 PDT, Kenichi Ishibashi 		no flags Details | Formatted Diff | Diff
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 Kenichi Ishibashi 2012-10-16 00:50:14 PDT 
The lifetime of hb_face_t instances should correspond with the lifetime of underlying font data(e.g. SkTypeface and CTFont). HarfBuzzNGFace has its cache mechanism to allow hb_face_t instances live as long as underlying font data live. Since the lifetime of underlying font data and FontPlatformData are different, hb_face_t instances should not depend on FontPlatformData.

harfbuzzSkiaGetTable(), harfbuzzCoreTextGetTable() and harfbuzzCairoGetTable() violate this restriction. These functions uses FontPlatformData to get font tables. We should pass underlying font data (or handle of underlying font data) to these functions instead. Otherwise, these functions can access freed FontPlatformData objects (http://crbug.com/156015 is an instance).

Note: we can use FontPlatformData in HarfBuzzNGFace::createFont(). This should be safe.
Note: we need not add a reference to underlying font data because the cache mechanism takes care of it.
Comment 1 Kenichi Ishibashi 2012-10-16 01:19:09 PDT 
Created attachment 168886 [details]
Patch
Comment 2 Kenichi Ishibashi 2012-10-16 01:25:53 PDT 
Kent-san, could you take a look? The change itself is trivial. I'm ccing you on crbug.com.

I confirmed the fix on chromium linux, and compiled the patch for chromium mac.

I'll wait and see whether efl port can compile the patch. (for changes of harfbuzzCairoGetTable())
Comment 3 Kent Tamura 2012-10-16 01:30:47 PDT 
Comment on attachment 168886 [details]
Patch

rubber-stamped
Comment 4 Kenichi Ishibashi 2012-10-16 02:06:41 PDT 
Comment on attachment 168886 [details]
Patch

Thanks!
Comment 5 WebKit Review Bot 2012-10-16 02:28:09 PDT 
Comment on attachment 168886 [details]
Patch

Clearing flags on attachment: 168886

Committed r131432: <http://trac.webkit.org/changeset/131432>
Comment 6 WebKit Review Bot 2012-10-16 02:28:12 PDT 
All reviewed patches have been landed.  Closing bug.