[cairo-commit] 4 commits - src/cairoint.h src/cairo-paginated-surface.c src/cairo-scaled-font.c src/cairo-surface.c src/test-meta-surface.c src/test-paginated-surface.c

Carl Worth cworth at kemper.freedesktop.org
Tue Feb 6 17:53:34 PST 2007


 src/cairo-paginated-surface.c |   21 +++++-
 src/cairo-scaled-font.c       |  142 +++++++++++++++++++++---------------------
 src/cairo-surface.c           |   19 +++--
 src/cairoint.h                |   58 ++++++++++++++++-
 src/test-meta-surface.c       |   21 +++++-
 src/test-paginated-surface.c  |   19 +++++
 6 files changed, 194 insertions(+), 86 deletions(-)

New commits:
diff-tree 5d58e7ee66166b19e673c247fe41eae51f72fd92 (from 765715ad93d3daad1b8b53f6ea9fee82737923ea)
Author: Carl Worth <cworth at cworth.org>
Date:   Mon Feb 5 16:45:25 2007 -0800

    Add scaled_font->mutex to allow locking for all subordinate objects
    
    A cairo_scaled_font_t can be implicitly shared among multiple threads
    as the same cairo_scaled_font_t can be returned from different calls
    to cairo_scaled_font_create. To retain the illusion that these
    different calls produce distinct objects, cairo must internally lock
    access when modifying them.
    
    Each glyph in the scaled font is represented by a cairo_surface_t
    which is used when rendering the glyph. Instead of attempting to push
    fine-grained locking of these surfaces down to the backend rendering
    functions, a simple per-cairo_scaled_font_t lock has been introduced
    which protects the entire rendering path against re-entrancy.
    
    Some care was required to ensure that existing re-entrancy was handled
    appropriately; these cases are in the wrapping surfaces
    (cairo-paginated, test-meta and test-paginated).
    
    Thanks to Vladimir Vukicev and Peter Weilbacher for testing/providing
    the mutex definitions for win32 and os2 (respectively).

diff --git a/src/cairo-paginated-surface.c b/src/cairo-paginated-surface.c
index b313ac1..6c1ff6a 100644
--- a/src/cairo-paginated-surface.c
+++ b/src/cairo-paginated-surface.c
@@ -442,6 +442,7 @@ _cairo_paginated_surface_show_glyphs (vo
 				      cairo_scaled_font_t	*scaled_font)
 {
     cairo_paginated_surface_t *surface = abstract_surface;
+    cairo_int_status_t status;
 
     /* Optimize away erasing of nothing. */
     if (surface->page_is_blank && op == CAIRO_OPERATOR_CLEAR)
@@ -449,9 +450,23 @@ _cairo_paginated_surface_show_glyphs (vo
 
     surface->page_is_blank = FALSE;
 
-    return _cairo_surface_show_glyphs (surface->meta, op, source,
-				       glyphs, num_glyphs,
-				       scaled_font);
+    /* Since this is a "wrapping" surface, we're calling back into
+     * _cairo_surface_show_glyphs from within a call to the same.
+     * Since _cairo_surface_show_glyphs acquires a mutex, we release
+     * and re-acquire the mutex around this nested call.
+     *
+     * Yes, this is ugly, but we consider it pragmatic as compared to
+     * adding locking code to all 18 surface-backend-specific
+     * show_glyphs functions, (which would get less testing and likely
+     * lead to bugs).
+     */
+    CAIRO_MUTEX_UNLOCK (scaled_font->mutex);
+    status = _cairo_surface_show_glyphs (surface->meta, op, source,
+					 glyphs, num_glyphs,
+					 scaled_font);
+    CAIRO_MUTEX_LOCK (scaled_font->mutex);
+
+    return status;
 }
 
 static cairo_surface_t *
diff --git a/src/cairo-scaled-font.c b/src/cairo-scaled-font.c
index ba84316..cdabb62 100755
--- a/src/cairo-scaled-font.c
+++ b/src/cairo-scaled-font.c
@@ -84,6 +84,7 @@ static const cairo_scaled_font_t _cairo_
       CAIRO_HINT_METRICS_DEFAULT} ,
     { 1., 0., 0., 1., 0, 0},	/* scale */
     { 0., 0., 0., 0., 0. },	/* extents */
+    CAIRO_MUTEX_NIL_INITIALIZER,/* mutex */
     NULL,			/* glyphs */
     NULL,			/* surface_backend */
     NULL,			/* surface_private */
@@ -361,6 +362,7 @@ _cairo_scaled_font_init (cairo_scaled_fo
 			   &scaled_font->font_matrix,
 			   &scaled_font->ctm);
 
+    CAIRO_MUTEX_INIT (&scaled_font->mutex);
     scaled_font->glyphs = _cairo_cache_create (_cairo_scaled_glyph_keys_equal,
 					       _cairo_scaled_glyph_destroy,
 					       max_glyphs_cached_per_font);
@@ -416,6 +418,8 @@ _cairo_scaled_font_fini (cairo_scaled_fo
     if (scaled_font->glyphs != NULL)
 	_cairo_cache_destroy (scaled_font->glyphs);
 
+    CAIRO_MUTEX_FINI (&scaled_font->mutex);
+
     if (scaled_font->surface_backend != NULL &&
 	scaled_font->surface_backend->scaled_font_fini != NULL)
 	scaled_font->surface_backend->scaled_font_fini (scaled_font);
@@ -710,6 +714,8 @@ cairo_scaled_font_glyph_extents (cairo_s
     if (scaled_font->status)
 	return;
 
+    CAIRO_MUTEX_LOCK (scaled_font->mutex);
+
     for (i = 0; i < num_glyphs; i++) {
 	double			left, top, right, bottom;
 
@@ -719,7 +725,7 @@ cairo_scaled_font_glyph_extents (cairo_s
 					     &scaled_glyph);
 	if (status) {
 	    _cairo_scaled_font_set_error (scaled_font, status);
-	    return;
+	    goto UNLOCK;
 	}
 
 	/* "Ink" extents should skip "invisible" glyphs */
@@ -773,6 +779,9 @@ cairo_scaled_font_glyph_extents (cairo_s
 	extents->x_advance = 0.0;
 	extents->y_advance = 0.0;
     }
+
+ UNLOCK:
+    CAIRO_MUTEX_UNLOCK (scaled_font->mutex);
 }
 slim_hidden_def (cairo_scaled_font_glyph_extents);
 
@@ -1337,6 +1346,8 @@ _cairo_scaled_glyph_set_path (cairo_scal
  * If the desired info is not available, (for example, when trying to
  * get INFO_PATH with a bitmapped font), this function will return
  * CAIRO_INT_STATUS_UNSUPPORTED.
+ *
+ * NOTE: This function must be called with scaled_font->mutex held.
  **/
 cairo_int_status_t
 _cairo_scaled_glyph_lookup (cairo_scaled_font_t *scaled_font,
@@ -1352,8 +1363,6 @@ _cairo_scaled_glyph_lookup (cairo_scaled
     if (scaled_font->status)
 	return scaled_font->status;
 
-    CAIRO_MUTEX_LOCK (cairo_scaled_font_map_mutex);
-
     key.hash = index;
     /*
      * Check cache for glyph
@@ -1424,8 +1433,6 @@ _cairo_scaled_glyph_lookup (cairo_scaled
 	*scaled_glyph_ret = scaled_glyph;
     }
 
-    CAIRO_MUTEX_UNLOCK (cairo_scaled_font_map_mutex);
-
     return status;
 }
 
diff --git a/src/cairo-surface.c b/src/cairo-surface.c
index 1541e6d..6f48a68 100644
--- a/src/cairo-surface.c
+++ b/src/cairo-surface.c
@@ -1846,19 +1846,22 @@ _cairo_surface_show_glyphs (cairo_surfac
 	cairo_font_options_destroy (font_options);
     }
 
-    if (surface->backend->show_glyphs) {
+    CAIRO_MUTEX_LOCK (dev_scaled_font->mutex);
+
+    status = CAIRO_INT_STATUS_UNSUPPORTED;
+
+    if (surface->backend->show_glyphs)
 	status = surface->backend->show_glyphs (surface, op, &dev_source.base,
 						glyphs, num_glyphs,
                                                 dev_scaled_font);
-	if (status != CAIRO_INT_STATUS_UNSUPPORTED)
-            goto FINISH;
-    }
 
-    status = _cairo_surface_fallback_show_glyphs (surface, op, &dev_source.base,
-                                                  glyphs, num_glyphs,
-                                                  dev_scaled_font);
+    if (status == CAIRO_INT_STATUS_UNSUPPORTED)
+	status = _cairo_surface_fallback_show_glyphs (surface, op, &dev_source.base,
+						      glyphs, num_glyphs,
+						      dev_scaled_font);
+
+    CAIRO_MUTEX_UNLOCK (dev_scaled_font->mutex);
 
-FINISH:
     if (dev_scaled_font != scaled_font)
 	cairo_scaled_font_destroy (dev_scaled_font);
 
diff --git a/src/cairoint.h b/src/cairoint.h
index 060b988..16f0316 100755
--- a/src/cairoint.h
+++ b/src/cairoint.h
@@ -137,9 +137,14 @@ CAIRO_BEGIN_DECLS
 #if HAVE_PTHREAD_H
 # include <pthread.h>
 # define CAIRO_MUTEX_DECLARE(name) static pthread_mutex_t name = PTHREAD_MUTEX_INITIALIZER
-#define CAIRO_MUTEX_DECLARE_GLOBAL(name) pthread_mutex_t name = PTHREAD_MUTEX_INITIALIZER
-# define CAIRO_MUTEX_LOCK(name) pthread_mutex_lock (&name)
+# define CAIRO_MUTEX_DECLARE_GLOBAL(name) pthread_mutex_t name = PTHREAD_MUTEX_INITIALIZER
+# define CAIRO_MUTEX_LOCK(name) \
+do { pthread_mutex_lock (&name); CAIRO_LOCK_FILE = __FILE__; CAIRO_LOCK_LINE = __LINE__; } while (0)
 # define CAIRO_MUTEX_UNLOCK(name) pthread_mutex_unlock (&name)
+typedef pthread_mutex_t cairo_mutex_t;
+# define CAIRO_MUTEX_INIT(mutex) pthread_mutex_init ((mutex), NULL)
+# define CAIRO_MUTEX_FINI(mutex) pthread_mutex_destroy (mutex)
+# define CAIRO_MUTEX_NIL_INITIALIZER PTHREAD_MUTEX_INITIALIZER
 #endif
 
 #if !defined(CAIRO_MUTEX_DECLARE) && defined CAIRO_HAS_WIN32_SURFACE
@@ -159,6 +164,10 @@ CAIRO_BEGIN_DECLS
 # define CAIRO_MUTEX_DECLARE_GLOBAL(name) extern LPCRITICAL_SECTION name;
 # define CAIRO_MUTEX_LOCK(name) EnterCriticalSection (&name)
 # define CAIRO_MUTEX_UNLOCK(name) LeaveCriticalSection (&name)
+typedef CRITICAL_SECTION cairo_mutex_t;
+# define CAIRO_MUTEX_INIT(mutex) InitializeCriticalSection (mutex)
+# define CAIRO_MUTEX_FINI(mutex) DeleteCriticalSection (mutex)
+# define CAIRO_MUTEX_NIL_INITIALIZER { 0 }
 #endif
 
 #if defined(__OS2__) && !defined(CAIRO_MUTEX_DECLARE)
@@ -170,6 +179,10 @@ CAIRO_BEGIN_DECLS
 # define CAIRO_MUTEX_DECLARE_GLOBAL(name) extern HMTX name
 # define CAIRO_MUTEX_LOCK(name) DosRequestMutexSem(name, SEM_INDEFINITE_WAIT)
 # define CAIRO_MUTEX_UNLOCK(name) DosReleaseMutexSem(name)
+typedef HMTX cairo_mutex_t;
+# define CAIRO_MUTEX_INIT(mutex) DosCreateMutexSem (NULL, mutex, 0L, TRUE)
+# define CAIRO_MUTEX_FINI(mutex) DosCloseMutexSem (*(mutex))
+# define CAIRO_MUTEX_NIL_INITIALIZER { 0 }
 #endif
 
 #if !defined(CAIRO_MUTEX_DECLARE) && defined CAIRO_HAS_BEOS_SURFACE
@@ -180,6 +193,13 @@ cairo_private void _cairo_beos_unlock(vo
 # define CAIRO_MUTEX_DECLARE_GLOBAL(name) extern void* name;
 # define CAIRO_MUTEX_LOCK(name) _cairo_beos_lock (&name)
 # define CAIRO_MUTEX_UNLOCK(name) _cairo_beos_unlock (&name)
+# error "XXX: Someone who understands BeOS needs to add definitions for" \
+        "     cairo_mutex_t, CAIRO_MUTEX_INIT, and CAIRO_MUTEX_FINI," \
+        "     to cairoint.h"
+typedef ??? cairo_mutex_t;
+# define CAIRO_MUTEX_INIT(mutex) ???
+# define CAIRO_MUTEX_FINI(mutex) ???
+# define CAIRO_MUTEX_NIL_INITIALIZER {}
 #endif
 
 #ifndef CAIRO_MUTEX_DECLARE
@@ -523,6 +543,36 @@ typedef struct _cairo_scaled_glyph {
 #define _cairo_scaled_glyph_set_index(g,i)  ((g)->cache_entry.hash = (i))
 
 struct _cairo_scaled_font {
+    /* For most cairo objects, the rule for multiple threads is that
+     * the user is responsible for any locking if the same object is
+     * manipulated from multiple threads simultaneously.
+     *
+     * However, with the caching that cairo does for scaled fonts, a
+     * user can easily end up with the same cairo_scaled_font object
+     * being manipulated from multiple threads without the user ever
+     * being aware of this, (and in fact, unable to control it).
+     *
+     * So, as a special exception, the cairo implementation takes care
+     * of all locking needed for cairo_scaled_font_t. Most of what is
+     * in the scaled font is immutable, (which is what allows for the
+     * sharing in the first place). The things that are modified and
+     * the locks protecting them are as follows:
+     *
+     * 1. The reference count (scaled_font->ref_count)
+     *
+     *    Modifications to the reference count are protected by the
+     *    cairo_scaled_font_map_mutex. This is because the reference
+     *    count of a scaled font is intimately related with the font
+     *    map itself, (and the magic holdovers array).
+     *
+     * 2. The cache of glyphs (scaled_font->glyphs)
+     * 3. The backend private data (scaled_font->surface_backend,
+     *				    scaled_font->surface_private)
+     *
+     *    Modifications to these fields are protected with locks on
+     *    scaled_font->mutex in the generic scaled_font code.
+     */
+
     /* must be first to be stored in a hash table */
     cairo_hash_entry_t hash_entry;
 
@@ -539,6 +589,10 @@ struct _cairo_scaled_font {
     /* "live" scaled_font members */
     cairo_matrix_t scale;	  /* font space => device space */
     cairo_font_extents_t extents; /* user space */
+
+    /* The mutex protects modification to all subsequent fields. */
+    cairo_mutex_t mutex;
+
     cairo_cache_t *glyphs;	  /* glyph index -> cairo_scaled_glyph_t */
 
     /*
diff --git a/src/test-meta-surface.c b/src/test-meta-surface.c
index 7939b6e..5fea766 100644
--- a/src/test-meta-surface.c
+++ b/src/test-meta-surface.c
@@ -252,12 +252,27 @@ _test_meta_surface_show_glyphs (void			*
 				cairo_scaled_font_t	*scaled_font)
 {
     test_meta_surface_t *surface = abstract_surface;
+    cairo_int_status_t status;
 
     surface->image_reflects_meta = FALSE;
 
-    return _cairo_surface_show_glyphs (surface->meta, op, source,
-				       glyphs, num_glyphs,
-				       scaled_font);
+    /* Since this is a "wrapping" surface, we're calling back into
+     * _cairo_surface_show_glyphs from within a call to the same.
+     * Since _cairo_surface_show_glyphs acquires a mutex, we release
+     * and re-acquire the mutex around this nested call.
+     *
+     * Yes, this is ugly, but we consider it pragmatic as compared to
+     * adding locking code to all 18 surface-backend-specific
+     * show_glyphs functions, (which would get less testing and likely
+     * lead to bugs).
+     */
+    CAIRO_MUTEX_UNLOCK (scaled_font->mutex);
+    status = _cairo_surface_show_glyphs (surface->meta, op, source,
+					 glyphs, num_glyphs,
+					 scaled_font);
+    CAIRO_MUTEX_LOCK (scaled_font->mutex);
+
+    return status;
 }
 
 static cairo_surface_t *
diff --git a/src/test-paginated-surface.c b/src/test-paginated-surface.c
index 544efcf..1950052 100644
--- a/src/test-paginated-surface.c
+++ b/src/test-paginated-surface.c
@@ -233,12 +233,27 @@ _test_paginated_surface_show_glyphs (voi
 				     cairo_scaled_font_t	*scaled_font)
 {
     test_paginated_surface_t *surface = abstract_surface;
+    cairo_int_status_t status;
 
     if (surface->paginated_mode == CAIRO_PAGINATED_MODE_ANALYZE)
 	return CAIRO_STATUS_SUCCESS;
 
-    return _cairo_surface_show_glyphs (surface->target, op, source,
-				       glyphs, num_glyphs, scaled_font);
+    /* Since this is a "wrapping" surface, we're calling back into
+     * _cairo_surface_show_glyphs from within a call to the same.
+     * Since _cairo_surface_show_glyphs acquires a mutex, we release
+     * and re-acquire the mutex around this nested call.
+     *
+     * Yes, this is ugly, but we consider it pragmatic as compared to
+     * adding locking code to all 18 surface-backend-specific
+     * show_glyphs functions, (which would get less testing and likely
+     * lead to bugs).
+     */
+    CAIRO_MUTEX_UNLOCK (scaled_font->mutex);
+    status = _cairo_surface_show_glyphs (surface->target, op, source,
+					 glyphs, num_glyphs, scaled_font);
+    CAIRO_MUTEX_LOCK (scaled_font->mutex);
+
+    return status;
 }
 
 static void
diff-tree 765715ad93d3daad1b8b53f6ea9fee82737923ea (from 9c359d61fc5df3e925e4b63503d60dc8fe8be6b3)
Author: Carl Worth <cworth at cworth.org>
Date:   Tue Feb 6 17:29:03 2007 -0800

    Move scaled font holdovers magic from reference to create to fix race condition
    
    Previously, with the magic in _cairo_scaled_font_reference(),
    cairo_scaled_font_create() was releasing its lock just before
    calling into reference() which would re-acquire the lock.
    That left a window open during which a font we just discovered
    in the holdovers table could be destroyed before we had a chance
    to give it its initial reference back.

diff --git a/src/cairo-scaled-font.c b/src/cairo-scaled-font.c
index ed47778..ba84316 100755
--- a/src/cairo-scaled-font.c
+++ b/src/cairo-scaled-font.c
@@ -468,12 +468,34 @@ cairo_scaled_font_create (cairo_font_fac
     if (_cairo_hash_table_lookup (font_map->hash_table, &key.hash_entry,
 				  (cairo_hash_entry_t**) &scaled_font))
     {
+	/* If the original reference count is 0, then this font must have
+	 * been found in font_map->holdovers, (which means this caching is
+	 * actually working). So now we remove it from the holdovers
+	 * array. */
+	if (scaled_font->ref_count == 0) {
+	    int i;
+
+	    for (i = 0; i < font_map->num_holdovers; i++)
+		if (font_map->holdovers[i] == scaled_font)
+		    break;
+	    assert (i < font_map->num_holdovers);
+
+	    font_map->num_holdovers--;
+	    memmove (&font_map->holdovers[i],
+		     &font_map->holdovers[i+1],
+		     (font_map->num_holdovers - i) * sizeof (cairo_scaled_font_t*));
+	}
+
+	/* We increment the reference count manually here, (rather
+	 * than calling into cairo_scaled_font_reference), since we
+	 * must modify the reference count while our lock is still
+	 * held. */
+	scaled_font->ref_count++;
 	_cairo_scaled_font_map_unlock ();
-	cairo_scaled_font_reference (scaled_font);
     } else {
 	/* We don't want any mutex held while calling into the backend
 	 * function. */
-        _cairo_scaled_font_map_unlock ();
+	_cairo_scaled_font_map_unlock ();
 
 	/* Otherwise create it and insert it into the hash table. */
 	status = font_face->backend->scaled_font_create (font_face, font_matrix,
@@ -514,44 +536,17 @@ slim_hidden_def (cairo_scaled_font_creat
 cairo_scaled_font_t *
 cairo_scaled_font_reference (cairo_scaled_font_t *scaled_font)
 {
-    cairo_scaled_font_map_t *font_map;
-
     if (scaled_font == NULL)
 	return NULL;
 
     if (scaled_font->ref_count == CAIRO_REF_COUNT_INVALID)
 	return scaled_font;
 
-    /* We would normally assert (scaled_font->ref_count > 0) here, but
-     * we are using ref_count == 0 as a legitimate case for the
-     * holdovers array. See below. */
-
-    /* cairo_scaled_font_t objects are cached and shared between
-     * threads. This works because these objects are immutable. Except
-     * that the reference count is mutable, so we have to do locking
-     * around any modification of the reference count. */
-    font_map = _cairo_scaled_font_map_lock ();
+    _cairo_scaled_font_map_lock ();
     {
-	/* If the original reference count is 0, then this font must have
-	 * been found in font_map->holdovers, (which means this caching is
-	 * actually working). So now we remove it from the holdovers
-	 * array. */
-	if (scaled_font->ref_count == 0) {
-	    int i;
-
-	    for (i = 0; i < font_map->num_holdovers; i++)
-		if (font_map->holdovers[i] == scaled_font)
-		    break;
-	    assert (i < font_map->num_holdovers);
-
-	    font_map->num_holdovers--;
-	    memmove (&font_map->holdovers[i],
-		     &font_map->holdovers[i+1],
-		     (font_map->num_holdovers - i) * sizeof (cairo_scaled_font_t*));
-	}
+	assert (scaled_font->ref_count > 0);
 
 	scaled_font->ref_count++;
-
     }
     _cairo_scaled_font_map_unlock ();
 
diff-tree 9c359d61fc5df3e925e4b63503d60dc8fe8be6b3 (from 258175ffcd89dcc949c3dc6ee3cd660d057a1966)
Author: Carl Worth <cworth at cworth.org>
Date:   Tue Feb 6 17:40:17 2007 -0800

    Avoid holding lock when calling _cairo_scaled_font_fini
    
    As in the previous commit with the backend->scaled_font_create
    function, we also don't want to hold the lock when calling into
    the backend's fini function.

diff --git a/src/cairo-scaled-font.c b/src/cairo-scaled-font.c
index c5b296b..ed47778 100755
--- a/src/cairo-scaled-font.c
+++ b/src/cairo-scaled-font.c
@@ -571,6 +571,7 @@ void
 cairo_scaled_font_destroy (cairo_scaled_font_t *scaled_font)
 {
     cairo_scaled_font_map_t *font_map;
+    cairo_scaled_font_t *lru = NULL;
 
     if (scaled_font == NULL)
 	return;
@@ -578,10 +579,6 @@ cairo_scaled_font_destroy (cairo_scaled_
     if (scaled_font->ref_count == CAIRO_REF_COUNT_INVALID)
 	return;
 
-    /* cairo_scaled_font_t objects are cached and shared between
-     * threads. This works because these objects are immutable. Except
-     * that the reference count is mutable, so we have to do locking
-     * around any modification of the reference count. */
     font_map = _cairo_scaled_font_map_lock ();
     {
 	assert (font_map != NULL);
@@ -595,17 +592,13 @@ cairo_scaled_font_destroy (cairo_scaled_
 	     * soon. To make room for it, we do actually destroy the
 	     * least-recently-used holdover.
 	     */
-	    if (font_map->num_holdovers == CAIRO_SCALED_FONT_MAX_HOLDOVERS) {
-		cairo_scaled_font_t *lru;
-
+	    if (font_map->num_holdovers == CAIRO_SCALED_FONT_MAX_HOLDOVERS)
+	    {
 		lru = font_map->holdovers[0];
 		assert (lru->ref_count == 0);
 
 		_cairo_hash_table_remove (font_map->hash_table, &lru->hash_entry);
 
-		_cairo_scaled_font_fini (lru);
-		free (lru);
-
 		font_map->num_holdovers--;
 		memmove (&font_map->holdovers[0],
 			 &font_map->holdovers[1],
@@ -617,6 +610,17 @@ cairo_scaled_font_destroy (cairo_scaled_
 	}
     }
     _cairo_scaled_font_map_unlock ();
+
+    /* If we pulled an item from the holdovers array, (while the font
+     * map lock was held, of course), then there is no way that anyone
+     * else could have acquired a reference to it. So we can now
+     * safely call fini on it without any lock held. This is desirable
+     * as we never want to call into any backend function with a lock
+     * held. */
+    if (lru) {
+	_cairo_scaled_font_fini (lru);
+	free (lru);
+    }
 }
 slim_hidden_def (cairo_scaled_font_destroy);
 
diff-tree 258175ffcd89dcc949c3dc6ee3cd660d057a1966 (from fc660511ec7a51be909e5ed940354d39ef7ad633)
Author: Carl Worth <cworth at cworth.org>
Date:   Tue Feb 6 17:21:22 2007 -0800

    Don't hold mutex over backend->scaled_font_create
    
    This also allows some cleanup of the error-handling in
    cairo_scaled_font_create, (no more goto statements).

diff --git a/src/cairo-scaled-font.c b/src/cairo-scaled-font.c
index 92d1d4f..c5b296b 100755
--- a/src/cairo-scaled-font.c
+++ b/src/cairo-scaled-font.c
@@ -459,7 +459,7 @@ cairo_scaled_font_create (cairo_font_fac
 
     font_map = _cairo_scaled_font_map_lock ();
     if (font_map == NULL)
-	goto UNWIND;
+	return NULL;
 
     _cairo_scaled_font_init_key (&key, font_face,
 				 font_matrix, ctm, options);
@@ -469,34 +469,34 @@ cairo_scaled_font_create (cairo_font_fac
 				  (cairo_hash_entry_t**) &scaled_font))
     {
 	_cairo_scaled_font_map_unlock ();
-	return cairo_scaled_font_reference (scaled_font);
-    }
-
-    /* Otherwise create it and insert it into the hash table. */
-    status = font_face->backend->scaled_font_create (font_face, font_matrix,
-						     ctm, options, &scaled_font);
-    if (status)
-	goto UNWIND_FONT_MAP_LOCK;
+	cairo_scaled_font_reference (scaled_font);
+    } else {
+	/* We don't want any mutex held while calling into the backend
+	 * function. */
+        _cairo_scaled_font_map_unlock ();
+
+	/* Otherwise create it and insert it into the hash table. */
+	status = font_face->backend->scaled_font_create (font_face, font_matrix,
+							 ctm, options, &scaled_font);
+	if (status)
+	    return NULL;
 
-    status = _cairo_hash_table_insert (font_map->hash_table,
-				       &scaled_font->hash_entry);
-    if (status)
-	goto UNWIND_SCALED_FONT_CREATE;
+	font_map = _cairo_scaled_font_map_lock ();
+	status = _cairo_hash_table_insert (font_map->hash_table,
+					   &scaled_font->hash_entry);
+	_cairo_scaled_font_map_unlock ();
 
-    _cairo_scaled_font_map_unlock ();
+	if (status) {
+	    /* We can't call _cairo_scaled_font_destroy here since it expects
+	     * that the font has already been successfully inserted into the
+	     * hash table. */
+	    _cairo_scaled_font_fini (scaled_font);
+	    free (scaled_font);
+	    return NULL;
+	}
+    }
 
     return scaled_font;
-
-UNWIND_SCALED_FONT_CREATE:
-    /* We can't call _cairo_scaled_font_destroy here since it expects
-     * that the font has already been successfully inserted into the
-     * hash table. */
-    _cairo_scaled_font_fini (scaled_font);
-    free (scaled_font);
-UNWIND_FONT_MAP_LOCK:
-    _cairo_scaled_font_map_unlock ();
-UNWIND:
-    return NULL;
 }
 slim_hidden_def (cairo_scaled_font_create);
 


More information about the cairo-commit mailing list