Bug 26353 – Incorrect Handling of Cairo Fonts

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Brent Fulgham]

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.

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 31212 [details]
First Iteration Patch

Clean up font references to avoid double-destruction of cairo font types.

[Eric Seidel (no email)]

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.

[Brent Fulgham]

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.

[Brent Fulgham]

Landed in http://trac.webkit.org/changeset/44628

[Mihnea Ovidenie]

(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

[Brent Fulgham]

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

[Brent Fulgham]

Reopening bug to address Mihnea's excellent point that the assignment operator should also be corrected for this case.

[Brent Fulgham]

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

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 31369 [details]
Revised patch.

[Brent Fulgham]

Created attachment 31373 [details]
Final revision.

Revised patch based on full build.

[Maciej Stachowiak]

Comment on attachment 31373 [details]
Final revision.

r=me

[Brent Fulgham]

Landed second patch in http://trac.webkit.org/changeset/44740.