[cairo] [PATCH] Do not hold mutexes when calling destructors.

[Jeff Muizelaar]

On Tue, May 13, 2008 at 10:04:15AM -0400, Kristian Høgsberg wrote:
> On Fri, May 9, 2008 at 6:15 PM, Jeff Muizelaar <jeff at infidigm.net> wrote:
> > cairo_surface_destroy and _cairo_scaled_font_fini will call destroy closures
> >  which may call functions that attempt to acquire the mutex resulting in a
> >  deadlock. We fix this by releasing the lock for the call to
> >  cairo_surface_destroy or _cairo_scaled_font_fini.
> >  ---
> >
> >   src/cairo-pattern.c     |   17 ++++++++++++-----
> >   src/cairo-scaled-font.c |   14 +++++++++++---
> >   2 files changed, 23 insertions(+), 8 deletions(-)
> >
> >
> >  diff --git a/src/cairo-pattern.c b/src/cairo-pattern.c
> >  index 0f2eb3e..903d81f 100644
> >  --- a/src/cairo-pattern.c
> >  +++ b/src/cairo-pattern.c
> >  @@ -1515,13 +1515,20 @@ UNLOCK:
> >   static void
> >   _cairo_pattern_reset_solid_surface_cache (void)
> >   {
> >  -    int i;
> >  -
> >      CAIRO_MUTEX_LOCK (_cairo_pattern_solid_surface_cache_lock);
> >
> >  -    for (i = 0; i < solid_surface_cache.size; i++)
> >  -       cairo_surface_destroy (solid_surface_cache.cache[i].surface);
> >  -    solid_surface_cache.size = 0;
> >  +    /* remove surfaces starting from the end so that solid_surface_cache.cache
> >  +     * is always in a consistent state when we release the mutex. */
> >  +    while (solid_surface_cache.size) {
> >  +       cairo_surface_t *surface = solid_surface_cache.cache[solid_surface_cache.size-1].surface;
> >  +       solid_surface_cache.size--;
> >  +
> >  +       /* release the lock to avoid the possibility of a recursive
> >  +        * deadlock when the scaled font destroy closure gets called */
> >  +       CAIRO_MUTEX_UNLOCK (_cairo_pattern_solid_surface_cache_lock);
> >  +       cairo_surface_destroy (surface);
> >  +       CAIRO_MUTEX_LOCK (_cairo_pattern_solid_surface_cache_lock);
> >  +    }
> 
> Would it be better to say:
> 
>     struct solid_surface_cache copy;
> 
>     CAIRO_MUTEX_LOCK (_cairo_pattern_solid_surface_cache_lock);
>     copy = solid_surface_cache;
>     solid_surface_cache.size = 0;
>     CAIRO_MUTEX_UNLOCK (_cairo_pattern_solid_surface_cache_lock);
> 
>     for (i = 0; i < copy.size; i++)
>         cairo_surface_destroy (copy.cache[i].surface);

Perhaps. Is there an advantage to clearing the whole cache atomically?
If not, I don't really see what making a copy buys us.

-Jeff