[cairo] Evince crash in FT_Get_PS_Font_Info

[Chris Wilson]

On Mon, 2008-04-28 at 12:03 -0400, Behdad Esfahbod wrote:
> On Mon, 2008-04-28 at 09:08 +0100, Chris Wilson wrote:
> > On Sun, 2008-04-27 at 02:07 -0400, Behdad Esfahbod wrote:
> > > Hi,
> > > 
> > > I came across this bug:
> > > 
> > >   https://bugs.launchpad.net/ubuntu/+source/evince/+bug/157797
> > > 
> > > Looks to me like an evince bug freeing FT_Face before cairo is done with
> > > it, but thought I post here so people better at this can check it out.
> > 
> > Been there, done that, wrote the patch. ;-)
> > http://cgit.freedesktop.org/poppler/poppler/commit/?id=42db4890e8295aaec5a1be12d1414fc0a9048550
> 
> Ah right.  I saw that going by.  Somehow I thought it was a mozilla
> patch.  Doesn't make sense now that I think about it ;).
> 
> Two notes:
> 
>   - cairo doesn't use the face behind poppler's back.  It looks like
> there should be a poppler lifecycle issue involved as well, and your
> patch actually hiding the bug.  Just a thought.  And what about the
> actual font file data, is that kept alive anyway?

AIUI, the cairo_font_face_t is kept alive via references to the
scaled_font_t in the meta-surface and, potentially, the pdf-surface.
Skimming through the poppler code paths, it looked like they create
short-lived CairoFonts for immediate use, combined with a simple caching
scheme to mitigate the number of
FT_New_Face()/cairo_ft_font_face_create_for_ft_face() calls. (This will
actually generate sub-par PDFs, as cairo will embed the same font
multiple times - so a better patch would duplicate the caching scheme
used within cairo for FcPattern font_faces.)

As for the original font file - it appears that poppler already uses a
temporary file that only exists for the call to FT_New_Face() (it is
immediately unlinked afterwards), so I presume the FT_Face either keeps
the fd open, or sucks the entire font into memory for its lifetime.
However, now you've got me worried!

>   - Can update the documentation with the fact that the face should be
> kept alive as long as the cairo_font_face_t is alive, and add sample
> code using user_data for handling it.

The documentation for cairo_ft_font_face_create_for_ft_face() already
mentions that the FT_Face object must be kept around until the
cairo_font_face_t reference count drops to zero, and to use the
user_data callback to determine when it safe to call FT_Done_Face. An
example, will of course, be helpful.

Even worse though, it appears that not even
test/ft-font-create-for-ft-face.c gets the lifetime management of the
FT_Face correct (as the FT_Face leaks out of scope through a reference
held by the context)!

I've added a bit of blurb to the documentation to provide an example of
how to use cairo_font_face_set_user_data() in combination with an
FT_Face and released the errant reference in the test code.
-- 
Chris Wilson