Bug 26353 - Incorrect Handling of Cairo Fonts
: Incorrect Handling of Cairo Fonts
Status: RESOLVED FIXED
: WebKit
WebKit Misc.
: 528+ (Nightly build)
: PC Windows XP
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-06-12 13:24 PST by
Modified: 2009-06-16 14:56 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-06-12 13:24:51 PST
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 From 2009-06-12 13:29:40 PST -------
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 From 2009-06-12 13:55:53 PST -------
Created an attachment (id=31212) [details]
First Iteration Patch

Clean up font references to avoid double-destruction of cairo font types.
------- Comment #3 From 2009-06-12 14:00:26 PST -------
(From update of attachment 31212 [details])
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 From 2009-06-12 14:21:17 PST -------
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 From 2009-06-12 14:32:06 PST -------
Landed in http://trac.webkit.org/changeset/44628
------- Comment #6 From 2009-06-16 05:08:43 PST -------
(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 From 2009-06-16 10:24:57 PST -------
(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 From 2009-06-16 10:25:40 PST -------
Reopening bug to address Mihnea's excellent point that the assignment operator should also be corrected for this case.
------- Comment #9 From 2009-06-16 13:44:32 PST -------
(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 From 2009-06-16 13:51:51 PST -------
Created an attachment (id=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 From 2009-06-16 14:09:34 PST -------
Created an attachment (id=31369) [details]
Revised patch.
------- Comment #12 From 2009-06-16 14:32:05 PST -------
Created an attachment (id=31373) [details]
Final revision.

Revised patch based on full build.
------- Comment #13 From 2009-06-16 14:35:44 PST -------
(From update of attachment 31373 [details])
r=me
------- Comment #14 From 2009-06-16 14:56:14 PST -------
Landed second patch in http://trac.webkit.org/changeset/44740.