[user-font] Fix fontmap locking

After consulting with Keith Packard we came up with a farily simple
solution.  Documented in the code.

src/cairo-scaled-font-private.h

@@ -89,6 +89,8 @@ struct _cairo_scaled_font {
     cairo_matrix_t ctm;	          /* user space => device space */
     cairo_font_options_t options;
 
+    cairo_bool_t created; /*  protected by fontmap mutex */
+
     cairo_bool_t finished;
 
     /* "live" scaled_font members */

src/cairo-scaled-font.c

@@ -192,6 +192,7 @@ static const cairo_scaled_font_t _cairo_scaled_font_nil = {
       CAIRO_SUBPIXEL_ORDER_DEFAULT,
       CAIRO_HINT_STYLE_DEFAULT,
       CAIRO_HINT_METRICS_DEFAULT} ,
+    TRUE,			/* created */
     TRUE,			/* finished */
     { 1., 0., 0., 1., 0, 0},	/* scale */
     { 1., 0., 0., 1., 0, 0},	/* scale_inverse */
@@ -386,6 +387,96 @@ _cairo_scaled_font_map_destroy (void)
     CAIRO_MUTEX_UNLOCK (_cairo_scaled_font_map_mutex);
 }
 
+
+/* If a scaled font wants to unlock the font map while still being
+ * created (needed for user-fonts), we need to take extra care not
+ * ending up with multiple identical scaled fonts being created.
+ *
+ * What we do is, we create a dummy identical scaled font, also marked
+ * as being created, lock its mutex, and insert that in the fontmap
+ * hash table.  This makes other code trying to create an identical
+ * scaled font to just wait and retry.
+ *
+ * The reason we have to create a new key instead of just using
+ * scaled_font is for lifecycle management: we need to (or rather,
+ * other code needs to) reference the key.  We can't do that on the
+ * input scaled_font as it may be freed by font backend upon error.
+ */
+
+void
+_cairo_scaled_font_unlock_font_map (cairo_scaled_font_t *scaled_font)
+{
+    if (!scaled_font->created) {
+        cairo_status_t status = CAIRO_STATUS_SUCCESS;
+	cairo_scaled_font_t *key;
+
+	key = malloc (sizeof (cairo_scaled_font_t));
+	if (!key) {
+	    status = CAIRO_STATUS_NO_MEMORY;
+	    goto FREE;
+	}
+
+	/* full initialization is wasteful, but who cares... */
+	status = _cairo_scaled_font_init (key,
+					  scaled_font->font_face,
+					  &scaled_font->font_matrix,
+					  &scaled_font->ctm,
+					  &scaled_font->options,
+					  NULL);
+	if (status)
+	    goto FINI;
+
+	CAIRO_MUTEX_LOCK (key->mutex);
+
+	status = _cairo_hash_table_insert (cairo_scaled_font_map->hash_table,
+					   &key->hash_entry);
+	if (status)
+	    goto UNLOCK_KEY;
+
+	goto UNLOCK;
+
+       UNLOCK_KEY:
+	CAIRO_MUTEX_UNLOCK (key->mutex);
+
+       FINI:
+	_cairo_scaled_font_fini (key);
+
+       FREE:
+	free (key);
+
+	status = _cairo_scaled_font_set_error (scaled_font, status);
+    }
+
+ UNLOCK:
+    CAIRO_MUTEX_UNLOCK (_cairo_scaled_font_map_mutex);
+}
+
+void
+_cairo_scaled_font_lock_font_map (cairo_scaled_font_t *scaled_font)
+{
+    CAIRO_MUTEX_LOCK (_cairo_scaled_font_map_mutex);
+
+    if (!scaled_font->created) {
+	cairo_scaled_font_t *key;
+	cairo_bool_t found;
+
+	found = _cairo_hash_table_lookup (cairo_scaled_font_map->hash_table,
+					  &scaled_font->hash_entry,
+					  (cairo_hash_entry_t**) &key);
+	assert (found);
+
+	_cairo_hash_table_remove (cairo_scaled_font_map->hash_table,
+				  &scaled_font->hash_entry);
+
+	CAIRO_MUTEX_UNLOCK (scaled_font->mutex);
+
+	CAIRO_MUTEX_UNLOCK (_cairo_scaled_font_map_mutex);
+	cairo_scaled_font_destroy (key);
+	CAIRO_MUTEX_LOCK (_cairo_scaled_font_map_mutex);
+    }
+}
+
+
 /* Fowler / Noll / Vo (FNV) Hash (http://www.isthe.com/chongo/tech/comp/fnv/)
  *
  * Not necessarily better than a lot of other hashes, but should be OK, and
@@ -418,6 +509,7 @@ _cairo_scaled_font_init_key (cairo_scaled_font_t        *scaled_font,
     uint32_t hash = FNV1_32_INIT;
 
     scaled_font->status = CAIRO_STATUS_SUCCESS;
+    scaled_font->created = FALSE;
     scaled_font->font_face = font_face;
     scaled_font->font_matrix = *font_matrix;
     scaled_font->ctm = *ctm;
@@ -601,7 +693,7 @@ _cairo_scaled_font_fini (cairo_scaled_font_t *scaled_font)
 	scaled_font->surface_backend->scaled_font_fini != NULL)
 	scaled_font->surface_backend->scaled_font_fini (scaled_font);
 
-    if (scaled_font->backend->fini != NULL)
+    if (scaled_font->backend != NULL && scaled_font->backend->fini != NULL)
 	scaled_font->backend->fini (scaled_font);
 
     _cairo_user_data_array_fini (&scaled_font->user_data);
@@ -655,9 +747,25 @@ cairo_scaled_font_create (cairo_font_face_t          *font_face,
     _cairo_scaled_font_init_key (&key, font_face,
 				 font_matrix, ctm, options);
 
+
+    while (_cairo_hash_table_lookup (font_map->hash_table, &key.hash_entry,
+				     (cairo_hash_entry_t**) &scaled_font))
+    {
+	if (scaled_font->created)
+	    break;
+
+	/* If the scaled font is being created (happens for user-font),
+	 * just wait until it's done, then retry */
+	cairo_scaled_font_reference (scaled_font);
+	CAIRO_MUTEX_UNLOCK (_cairo_scaled_font_map_mutex);
+	CAIRO_MUTEX_LOCK (scaled_font->mutex);
+	CAIRO_MUTEX_UNLOCK (scaled_font->mutex);
+	CAIRO_MUTEX_LOCK (_cairo_scaled_font_map_mutex);
+	cairo_scaled_font_destroy (scaled_font);
+    }
+
     /* Return existing scaled_font if it exists in the hash table. */
-    if (_cairo_hash_table_lookup (font_map->hash_table, &key.hash_entry,
-				  (cairo_hash_entry_t**) &scaled_font))
+    if (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
@@ -704,6 +812,7 @@ cairo_scaled_font_create (cairo_font_face_t          *font_face,
 	return _cairo_scaled_font_create_in_error (status);
     }
 
+    scaled_font->created = TRUE;
     status = _cairo_hash_table_insert (font_map->hash_table,
 				       &scaled_font->hash_entry);
     _cairo_scaled_font_map_unlock ();
@@ -828,7 +937,7 @@ cairo_scaled_font_destroy (cairo_scaled_font_t *scaled_font)
 	CAIRO_MUTEX_LOCK (_cairo_scaled_font_map_mutex);
 	font_map = cairo_scaled_font_map;
 
-	if (font_map && scaled_font->hash_entry.hash != ZOMBIE) {
+	if (scaled_font->created && font_map && scaled_font->hash_entry.hash != ZOMBIE) {
 	    /* Rather than immediately destroying this object, we put it into
 	     * the font_map->holdovers array in case it will get used again
 	     * soon (and is why we must hold the lock over the atomic op on

src/cairo-user-font.c

@@ -327,21 +327,19 @@ _cairo_user_font_face_scaled_font_create (void                        *abstract_
     }
 
     if (font_face->scaled_font_methods.init != NULL) {
-	/* XXX by releasing fontmap lock here, we open up the case for having
-	 * two scaled fonts with the same key to be inserted in the fontmap.
-	 *
-	 * To fix that, we should make _cairo_scaled_font_init() insert
-	 * scaled_font in the fontmap, then lock the scaled_font lock here and
-	 * release the fontmap's.
-	 */
+
 	/* Lock the scaled_font mutex such that user doesn't accidentally try
-	 * to use it just yet.
-	 */
+         * to use it just yet. */
 	CAIRO_MUTEX_LOCK (user_scaled_font->base.mutex);
-	CAIRO_MUTEX_UNLOCK (_cairo_scaled_font_map_mutex);
+
+	/* Give away fontmap lock such that user-font can use other fonts */
+	_cairo_scaled_font_unlock_font_map (&user_scaled_font->base);
+
 	status = font_face->scaled_font_methods.init (&user_scaled_font->base,
 						      &font_extents);
-	CAIRO_MUTEX_LOCK (_cairo_scaled_font_map_mutex);
+
+	_cairo_scaled_font_lock_font_map (&user_scaled_font->base);
+
 	CAIRO_MUTEX_UNLOCK (user_scaled_font->base.mutex);
     }
 

src/cairoint.h

@@ -1471,6 +1471,11 @@ _cairo_scaled_font_create_in_error (cairo_status_t status);
 cairo_private void
 _cairo_scaled_font_reset_static_data (void);
 
+cairo_private void
+_cairo_scaled_font_unlock_font_map (cairo_scaled_font_t *scaled_font);
+
+cairo_private void
+_cairo_scaled_font_lock_font_map (cairo_scaled_font_t *scaled_font);
 
 cairo_private cairo_status_t
 _cairo_scaled_font_init (cairo_scaled_font_t               *scaled_font,