Created attachment 25802 [details] [review]
patch

The unscaled_font only needs to keep one zombie font_face.

I can't fully review the patch before studying the caching logic there from scratch again...  If you're confident that this is fully completely correct, please go ahead and push it in :).

Thanks

I extended test/ft-font-create-for-face and was able to reproduce the leak but not the potential crash. As the patch caused no other side effects, I've pushed it to master:

commit 0238fe2cafea2e1ed19bb222117bd73ee6898d4d
Author: Karl Tomlinson <karlt+@karlt.net>
Date:   Thu May 14 11:46:29 2009 +0100

    [ft] Resolve mutual referencing problems with zombie faces
    
I tried to find a test case to exercise:
_cairo_ft_font_face_create()
    if (font_face->unscaled == NULL) {
		/* Resurrect this "zombie" font_face (from
		 * _cairo_ft_font_face_destroy), switching its unscaled_font
		 * from owner to ownee. */
		font_face->unscaled = unscaled;
		_cairo_unscaled_font_reference (&unscaled->base);
		return &font_face->base;
    }
and
  if (unscaled->faces && unscaled->faces->unscaled == NULL) {
	/* This "zombie" font_face (from _cairo_ft_font_face_destroy)
	 * is no longer needed. */
	assert (unscaled->from_face && unscaled->faces->next == NULL);
	cairo_font_face_destroy (&unscaled->faces->base);
	unscaled->faces = NULL;
    }
to no avail. Also I tried the test.html in the firefox bug report but saw no evidence that it even tried to use cairo_ft_font_create_for_face() - which is probably just user error on my part.

If you can identify a simple test case, or even trace, to reproduce the crash that would be very useful. Thanks.

(In reply to comment #3)
> I extended test/ft-font-create-for-face and was able to reproduce the leak
> but not the potential crash. As the patch caused no other side effects, I've
> pushed it to master:

Thanks very much, Chris.

> If you can identify a simple test case, or even trace, to reproduce the crash
> that would be very useful. Thanks.

I don't have a testcase for the crash.  I haven't seen such a crash, but it
looked theoretically possible.  (The test in the Mozilla bug is run under the
"crashtest" infrastructure, which also tests leaks.  Also the test uses a
downloaded font from another part of the Mozilla source tree and so the
Mozilla tree needs to be accessible through a web server to make the test use
cairo_ft_font_create_for_face.)

Producing the crash would require:

1) Create two font_faces that would share a single unscaled_font.

2) Release (only) one font_face.  (It becomes a zombie.)

3) Create a font_face exactly the same as the one just released.
   (It pulls out the zombie face.)

4) Create a scaled_font from the font_face in 3. (crash)

> I tried to find a test case to exercise:
> _cairo_ft_font_face_create()
>     if (font_face->unscaled == NULL) {
>                 /* Resurrect this "zombie" font_face (from
>                  * _cairo_ft_font_face_destroy), switching its unscaled_font
>                  * from owner to ownee. */
>                 font_face->unscaled = unscaled;
>                 _cairo_unscaled_font_reference (&unscaled->base);
>                 return &font_face->base;
>     }
> and
>   if (unscaled->faces && unscaled->faces->unscaled == NULL) {
>         /* This "zombie" font_face (from _cairo_ft_font_face_destroy)
>          * is no longer needed. */
>         assert (unscaled->from_face && unscaled->faces->next == NULL);
>         cairo_font_face_destroy (&unscaled->faces->base);
>         unscaled->faces = NULL;
>     }
> to no avail.

I'm not sure that cairo could currently execute this.  It may be possible
through the unscaled_font reference that _cairo_type1_font_subset_init takes,
but I don't know that code well and whether that reference can remain alive
beyond the reference to the font_face (long enough to create another font_face
using the same unscaled_font).

The change in _cairo_ft_font_face_destroy means that it will now only make a
zombie font_face on the unscaled_font when there are no other font_faces on
the unscaled_font.  And scaled_fonts seem to release their unscaled_fonts
soon after the reference to the font_face.  So something else needs to keep a
reference to the unscaled_font to execute this code.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/cairo/cairo/issues/294.