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).

src/cairo-paginated-surface.c

@@ -442,6 +442,7 @@ _cairo_paginated_surface_show_glyphs (void			*abstract_surface,
 				      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 (void			*abstract_surface,
 
     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 *

src/cairo-scaled-font.c

@@ -84,6 +84,7 @@ static const cairo_scaled_font_t _cairo_scaled_font_nil = {
       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_font_t               *scaled_font,
 			   &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_font_t *scaled_font)
     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_scaled_font_t   *scaled_font,
     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_scaled_font_t   *scaled_font,
 					     &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_scaled_font_t   *scaled_font,
 	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_scaled_glyph_t *scaled_glyph,
  * 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_font_t *scaled_font,
     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_font_t *scaled_font,
 	*scaled_glyph_ret = scaled_glyph;
     }
 
-    CAIRO_MUTEX_UNLOCK (cairo_scaled_font_map_mutex);
-
     return status;
 }
 

src/cairo-surface.c

@@ -1846,19 +1846,22 @@ _cairo_surface_show_glyphs (cairo_surface_t	*surface,
 	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);
 

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(void*);
 # 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 */
 
     /*

src/test-meta-surface.c

@@ -252,12 +252,27 @@ _test_meta_surface_show_glyphs (void			*abstract_surface,
 				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 *

src/test-paginated-surface.c

@@ -233,12 +233,27 @@ _test_paginated_surface_show_glyphs (void			*abstract_surface,
 				     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