Bug 1478084 - make cairo_font_face_set_user_data thread-safe. r=jrmuizel

gfx/2d/ScaledFontFontconfig.cpp

@@ -444,11 +444,6 @@ ScaledFontFontconfig::CreateFromInstanceData(const InstanceData& aInstanceData,
     // was freed. To prevent this, we must bind the NativeFontResource to the font face so that
     // it stays alive at least as long as the font face.
     aNativeFontResource->AddRef();
-    // Bug 1412545 - Setting Cairo font user data is not thread-safe. If Fontconfig patterns match,
-    // cairo_ft_font_face_create_for_pattern may share Cairo faces. We need to lock setting user data
-    // to prevent races if multiple threads are thus sharing the same Cairo face.
-    FT_Library library = face ? face->glyph->library : Factory::GetFTLibrary();
-    Factory::LockFTLibrary(library);
     cairo_status_t err = CAIRO_STATUS_SUCCESS;
     bool cleanupFace = false;
     if (varFace) {
@@ -465,7 +460,6 @@ ScaledFontFontconfig::CreateFromInstanceData(const InstanceData& aInstanceData,
                                           aNativeFontResource,
                                           ReleaseNativeFontResource);
     }
-    Factory::UnlockFTLibrary(library);
     if (err != CAIRO_STATUS_SUCCESS) {
       gfxWarning() << "Failed binding NativeFontResource to Cairo font face";
       if (varFace && cleanupFace) {

gfx/2d/ScaledFontFreeType.cpp

@@ -146,11 +146,6 @@ UnscaledFontFreeType::CreateScaledFont(Float aGlyphSize,
     // was freed. To prevent this, we must bind the NativeFontResource to the font face so that
     // it stays alive at least as long as the font face.
     nfr->AddRef();
-    // Bug 1412545 - Setting Cairo font user data is not thread-safe. If FT_Faces match,
-    // cairo_ft_font_face_create_for_ft_face may share Cairo faces. We need to lock setting user data
-    // to prevent races if multiple threads are thus sharing the same Cairo face.
-    FT_Library library = face->glyph->library;
-    Factory::LockFTLibrary(library);
     cairo_status_t err = CAIRO_STATUS_SUCCESS;
     bool cleanupFace = false;
     if (varFace) {
@@ -168,7 +163,6 @@ UnscaledFontFreeType::CreateScaledFont(Float aGlyphSize,
                                           nfr,
                                           ReleaseNativeFontResource);
     }
-    Factory::UnlockFTLibrary(library);
     if (err != CAIRO_STATUS_SUCCESS) {
       gfxWarning() << "Failed binding NativeFontResource to Cairo font face";
       if (varFace && cleanupFace) {

gfx/cairo/cairo/src/cairo-array.c

@@ -108,6 +108,8 @@ _cairo_array_fini (cairo_array_t *array)
     if (array->elements) {
 	free (* array->elements);
 	free (array->elements);
+	array->elements = NULL;
+	array->num_elements = 0;
     }
 }
 

gfx/cairo/cairo/src/cairo-font-face.c

@@ -156,8 +156,14 @@ cairo_font_face_destroy (cairo_font_face_t *font_face)
     if (CAIRO_REFERENCE_COUNT_HAS_REFERENCE (&font_face->ref_count))
 	return;
 
+    if (font_face->backend->lock)
+	font_face->backend->lock (font_face);
+
     _cairo_user_data_array_fini (&font_face->user_data);
 
+    if (font_face->backend->unlock)
+	font_face->backend->unlock (font_face);
+
     free (font_face);
 }
 slim_hidden_def (cairo_font_face_destroy);
@@ -235,8 +241,14 @@ void *
 cairo_font_face_get_user_data (cairo_font_face_t	   *font_face,
 			       const cairo_user_data_key_t *key)
 {
-    return _cairo_user_data_array_get_data (&font_face->user_data,
+    void *result;
+    if (font_face->backend->lock)
+	font_face->backend->lock (font_face);
+    result = _cairo_user_data_array_get_data (&font_face->user_data,
 					      key);
+    if (font_face->backend->unlock)
+	font_face->backend->unlock (font_face);
+    return result;
 }
 slim_hidden_def (cairo_font_face_get_user_data);
 
@@ -265,8 +277,14 @@ cairo_font_face_set_user_data (cairo_font_face_t	   *font_face,
     if (CAIRO_REFERENCE_COUNT_IS_INVALID (&font_face->ref_count))
 	return font_face->status;
 
-    return _cairo_user_data_array_set_data (&font_face->user_data,
+    cairo_status_t status;
+    if (font_face->backend->lock)
+	font_face->backend->lock (font_face);
+    status = _cairo_user_data_array_set_data (&font_face->user_data,
 					      key, user_data, destroy);
+    if (font_face->backend->unlock)
+	font_face->backend->unlock (font_face);
+    return status;
 }
 slim_hidden_def (cairo_font_face_set_user_data);
 

gfx/cairo/cairo/src/cairo-ft-font.c

@@ -657,7 +657,10 @@ _cairo_ft_unscaled_font_destroy (void *abstract_font)
 	 */
 	if (unscaled->faces && unscaled->faces->unscaled == NULL) {
 	    assert (unscaled->faces->next == NULL);
+	    // Guard user-data destruction with the unscaled font mutex.
+	    CAIRO_MUTEX_LOCK (unscaled->mutex);
 	    cairo_font_face_destroy (&unscaled->faces->base);
+	    CAIRO_MUTEX_UNLOCK (unscaled->mutex);
 	}
     } else {
 	_font_map_release_face_lock_held (font_map, unscaled);
@@ -2758,6 +2761,9 @@ _cairo_ft_font_face_destroy (void *abstract_face)
 	    last_face = tmp_face;
 	}
 
+	// Ensure user-data destruction happens within the unscaled font mutex.
+	_cairo_user_data_array_fini (&font_face->base.user_data);
+
 	CAIRO_MUTEX_UNLOCK (font_face->unscaled->mutex);
 	_cairo_unscaled_font_destroy (&font_face->unscaled->base);
 	font_face->unscaled = NULL;
@@ -2828,6 +2834,24 @@ _cairo_ft_font_face_get_implementation (void                     *abstract_face,
     return abstract_face;
 }
 
+static void
+_cairo_ft_font_face_lock (void *abstract_face)
+{
+    cairo_ft_font_face_t *font_face = abstract_face;
+    if (font_face->unscaled) {
+	CAIRO_MUTEX_LOCK (font_face->unscaled->mutex);
+    }
+}
+
+static void
+_cairo_ft_font_face_unlock (void *abstract_face)
+{
+    cairo_ft_font_face_t *font_face = abstract_face;
+    if (font_face->unscaled) {
+	CAIRO_MUTEX_UNLOCK (font_face->unscaled->mutex);
+    }
+}
+
 const cairo_font_face_backend_t _cairo_ft_font_face_backend = {
     CAIRO_FONT_TYPE_FT,
 #if CAIRO_HAS_FC_FONT
@@ -2837,7 +2861,9 @@ const cairo_font_face_backend_t _cairo_ft_font_face_backend = {
 #endif
     _cairo_ft_font_face_destroy,
     _cairo_ft_font_face_scaled_font_create,
-    _cairo_ft_font_face_get_implementation
+    _cairo_ft_font_face_get_implementation,
+    _cairo_ft_font_face_lock,
+    _cairo_ft_font_face_unlock
 };
 
 #if CAIRO_HAS_FC_FONT
@@ -2925,6 +2951,7 @@ _cairo_ft_font_face_create (cairo_ft_unscaled_font_t *unscaled,
 	/* This "zombie" font_face (from _cairo_ft_font_face_destroy)
 	 * is no longer needed. */
 	assert (unscaled->from_face && unscaled->faces->next == NULL);
+	// The unscaled font mutex is held, so destroying user data is safe inside here.
 	cairo_font_face_destroy (&unscaled->faces->base);
 	unscaled->faces = NULL;
     }

gfx/cairo/cairo/src/cairoint.h

@@ -541,6 +541,9 @@ struct _cairo_font_face_backend {
 			   const cairo_matrix_t		*font_matrix,
 			   const cairo_matrix_t		*ctm,
 			   const cairo_font_options_t	*options);
+
+    void (*lock) (void *font_face);
+    void (*unlock) (void *font_face);
 };
 
 extern const cairo_private struct _cairo_font_face_backend _cairo_user_font_face_backend;