Bug 4192

I'll take this bug over.

Created attachment 2988 [details] [review]
Detect num_open_faces bug

(In reply to comment #0)
> I also think an assert statement in the cairo_debug_reset_static_data
> path should catch it.

Indeed. This patch does exactly that. I won't commit it now, as it makes
every text using test crash (when using the cairo-ft text backend).

But this patch should go in as well once we have a fix.

Created attachment 2992 [details] [review]
Adjust num_open_faces before each call to _cairo_ft_unscaled_font_fini

Here's a patch to fix the bug, (including the assert statement from the
previous patch).

It's not lovely in that the decrement of num_open_faces is distant from the
call
to FT_Done_Face. But it's not obvious to me what a cleaner solution would be,
(since passing the font_map parameter down into unscaled_font_fini would cause
the locking to deviate from local-only, which seems worse).

Created attachment 2997 [details] [review]
Pull FT_Done_Face up out of _cairo_ft_unscaled_font_fini

Here's an alternate approach, pulling FT_Done_Face up out of
_cairo_ft_unscaled_font_fini, and replacing it with:

    assert (unscaled->face == NULL);

This should work just as well as the previous patch. It's not too obvious
to me which one is uglier. If anyone prefers one, I'll commit it. Otherwise
I'll pick one myself soon.

The second looks better to me, but I'd add some sort of 

 _cairo_font_map_release_face (font_map, face);

To avoid the duplication ... even though it's just a few lines of
code. You probably could consolidate the existing place where
Done_Font and num_open_facse-- is done as well.

(In reply to comment #5)
> The second looks better to me, but I'd add some sort of 
> 
>  _cairo_font_map_release_face (font_map, face);
> 
> To avoid the duplication ... even though it's just a few lines of
> code. You probably could consolidate the existing place where
> Done_Font and num_open_facse-- is done as well.

Yes, that's likely the right thing to do.

I think it would be our first function of the must-be-called-with-lock-held
variety. I'll invent a new convention for that and name it:

    _cairo_font_map_release_face_lock_held

Move bugs against "cvs" version to "0.9.3" so we can remove the "cvs" version.

Created attachment 3001 [details] [review]
Put FT_Done_Face and num_open_faces-- in _font_map_release_face_lock_held

Here's the final patch as about to be committed.

2005-08-22  Carl Worth  <cworth@cworth.org>

        Fix for bug #4192:

        * src/cairo-ft-font.c: (_font_map_release_face_lock_held): New
        function to handle both calling FT_Done_Face on unscaled->face and
        decrementing font_map->num_open_faces.

        * src/cairo-ft-font.c:
        (_cairo_ft_unscaled_font_map_destroy),
        (_cairo_ft_unscaled_font_destroy),
        (_cairo_ft_unscaled_font_lock_face): Call new
        _font_map_release_face_lock_held as approporiate.

        * src/cairo-ft-font.c: (_cairo_ft_unscaled_font_map_destroy):
        Assert that (font_map->num_open_faces == 0) when we're done, to
        help guarantee the bug is fixed.

        * src/cairo-ft-font.c: (_cairo_ft_unscaled_font_fini): Don't call
        FT_Done_Face anymore, instead assert that (unscaled->face == NULL)
        by the time this function is called.

        * src/cairo-ft-font.c: (_cairo_ft_unscaled_font_init),
        (_cairo_ft_unscaled_font_set_scale): Prefer TRUE/FALSE as values
        for cairo_bool_t have_scale.