Bug 4192 - _cairo_ft_unscaled_font_fini does not decrement font_map->num_open_faces
Summary: _cairo_ft_unscaled_font_fini does not decrement font_map->num_open_faces
Status: RESOLVED FIXED
Alias: None
Product: cairo
Classification: Unclassified
Component: freetype font backend (show other bugs)
Version: 0.9.3
Hardware: x86 (IA32) Linux (All)
: high normal
Assignee: Carl Worth
QA Contact: cairo-bugs mailing list
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2005-08-22 13:43 UTC by Carl Worth
Modified: 2005-08-22 07:00 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
Detect num_open_faces bug (560 bytes, patch)
2005-08-22 13:54 UTC, Carl Worth
Details | Splinter Review
Adjust num_open_faces before each call to _cairo_ft_unscaled_font_fini (1.09 KB, patch)
2005-08-22 14:44 UTC, Carl Worth
Details | Splinter Review
Pull FT_Done_Face up out of _cairo_ft_unscaled_font_fini (2.89 KB, patch)
2005-08-22 16:41 UTC, Carl Worth
Details | Splinter Review
Put FT_Done_Face and num_open_faces-- in _font_map_release_face_lock_held (4.63 KB, patch)
2005-08-23 00:00 UTC, Carl Worth
Details | Splinter Review

Description Carl Worth 2005-08-22 13:43:06 UTC
There's a logic bug in the handling of font_map->num_open_faces.

It should get decremented for every call to FT_Done_Face, but
_cairo_ft_unscaled_font_fini calls FT_Done_Face without decrementing
num_open_faces.

I think I introduced this bug with the hash rewrite. I also think an
assert statement in the cairo_debug_reset_static_data path should catch
it. I'll look into both detecting and fixing this bug.
Comment 1 Carl Worth 2005-08-22 13:49:29 UTC
I'll take this bug over.
Comment 2 Carl Worth 2005-08-22 13:54:53 UTC
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.
Comment 3 Carl Worth 2005-08-22 14:44:55 UTC
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).
Comment 4 Carl Worth 2005-08-22 16:41:34 UTC
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.
Comment 5 Owen Taylor 2005-08-22 17:03:55 UTC
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.
Comment 6 Carl Worth 2005-08-22 17:11:55 UTC
(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


Comment 7 Carl Worth 2005-08-22 17:15:55 UTC
Move bugs against "cvs" version to "0.9.3" so we can remove the "cvs" version.
Comment 8 Carl Worth 2005-08-23 00:00:18 UTC
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.
Comment 9 Carl Worth 2005-08-23 00:00:53 UTC
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.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.