Bug 26353 - Incorrect Handling of Cairo Fonts
: Incorrect Handling of Cairo Fonts
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebKit Misc.
: 528+ (Nightly build)
: PC Windows XP
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-06-12 13:24 PDT by Brent Fulgham
Modified: 2009-06-16 14:56 PDT (History)
1 user (show)

See Also:


Attachments
First Iteration Patch (6.96 KB, patch)
2009-06-12 13:55 PDT, Brent Fulgham
eric: review+
Details | Formatted Diff | Diff
Add assignment operator (3.14 KB, patch)
2009-06-16 13:51 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Revised patch. (3.22 KB, patch)
2009-06-16 14:09 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Final revision. (3.22 KB, patch)
2009-06-16 14:32 PDT, Brent Fulgham
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2009-06-12 13:24:51 PDT
Cairo's internal reference counting for its various font types (cairo_font_face_t* and cairo_scaled_font_t*) are getting out of sync because the various FontPlatformData implementations are not consistently marking the fonts in use and removing them when finished.

One significant cause of this problem is that the Cairo font implementations use the default copy constructor, which does a shallow pointer assignment for the font pointers, which can cause multiple destruction of the same font and possible crashes.

The attached patch corrects the font handling.
Comment 1 Brent Fulgham 2009-06-12 13:29:40 PDT
Note: One potentially confusing coding convention is that the cairo_font_face_t* pointers passed during construction are assumed to have a reference count of one (i.e., they were created prior to the constructor), and that the FontPlatformData object takes ownership.

Therefore, the main constructor does not increment the reference count, but the copy constructor must (or else the destructor count will be out of sync).

This is probably a bad idea and should be cleaned up in a future refactoring.

Better yet, we should probably create a RefCounted class to take care of all of this automatically.
Comment 2 Brent Fulgham 2009-06-12 13:55:53 PDT
Created attachment 31212 [details]
First Iteration Patch

Clean up font references to avoid double-destruction of cairo font types.
Comment 3 Eric Seidel 2009-06-12 14:00:26 PDT
Comment on attachment 31212 [details]
First Iteration Patch

Style:
 FontCustomPlatformData(const FontCustomPlatformData& source)
 40     : m_fontFace(cairo_font_face_reference(source.m_fontFace))
 41     {}
officially that should be:
 FontCustomPlatformData(const FontCustomPlatformData& source)
    : m_fontFace(cairo_font_face_reference(source.m_fontFace))
{
}
At least that's my understanding of the WK style.

Looks OK I guess.
Comment 4 Brent Fulgham 2009-06-12 14:21:17 PDT
Based on discussions with Gustavo Noronha Silva, leaving the GTK+ piece of this out.  The GTK+ fonts are noncopyable, so they don't see this problem.
Comment 5 Brent Fulgham 2009-06-12 14:32:06 PDT
Landed in http://trac.webkit.org/changeset/44628
Comment 6 Mihnea Ovidenie 2009-06-16 05:08:43 PDT
(In reply to comment #5)
> Landed in http://trac.webkit.org/changeset/44628
> 

Hi,
Wouldn't be better to add also an assignment operator to FontPlatformData class, just as a safe precaution? What did you mean by: "The GTK+ fonts are noncopyable, so they don't see this problem."

Regards,
Mihnea
Comment 7 Brent Fulgham 2009-06-16 10:24:57 PDT
(In reply to comment #6)
> Wouldn't be better to add also an assignment operator to FontPlatformData
> class, just as a safe precaution?

That's a good idea; I'll reopen the bug and add that as a patch.

> What did you mean by: "The GTK+ fonts are noncopyable, so they don't see this problem."

The GTK+ FontCustomPlatformData object inherits from the Noncopyable object, so the font objects can never be copied, and are therefore not impacted by this problem.
Comment 8 Brent Fulgham 2009-06-16 10:25:40 PDT
Reopening bug to address Mihnea's excellent point that the assignment operator should also be corrected for this case.
Comment 9 Brent Fulgham 2009-06-16 13:44:32 PDT
(In reply to comment #4)
> Based on discussions with Gustavo Noronha Silva, leaving the GTK+ piece of this
> out.  The GTK+ fonts are noncopyable, so they don't see this problem.

It turns out this is incorrect!  The FontCustomPlatformData object for GTK+ *is* noncopyable (as it is for the Windows Cairo build), but the area in question is the FontPlatformData object.

In GTK+/Pango, the FontPlatformData object implements both a copy constructor and an assignment operator, which is why this platform has never had the problem.
Comment 10 Brent Fulgham 2009-06-16 13:51:51 PDT
Created attachment 31368 [details]
Add assignment operator

Update to original change to provide an assignment operator.  This brings the behavior in line with the GTK+ Cairo port, and helps guarantee that we get proper behavior.
Comment 11 Brent Fulgham 2009-06-16 14:09:34 PDT
Created attachment 31369 [details]
Revised patch.
Comment 12 Brent Fulgham 2009-06-16 14:32:05 PDT
Created attachment 31373 [details]
Final revision.

Revised patch based on full build.
Comment 13 Maciej Stachowiak 2009-06-16 14:35:44 PDT
Comment on attachment 31373 [details]
Final revision.

r=me
Comment 14 Brent Fulgham 2009-06-16 14:56:14 PDT
Landed second patch in http://trac.webkit.org/changeset/44740.