Bug 1383767 - guarantee FreeType thread-safety by holding Cairo per-face lock and locking down rasterization. r=jrmuizel
authorLee Salzman <lsalzman@mozilla.com>
Wed, 26 Jul 2017 23:24:44 -0400
changeset 420088 b95ff4c0ba0a48d0fdcc13cb9f80ca033a6c4d13
parent 420087 b17eb8514a105e24fbb30c3979aa734bf2e044c0
child 420089 312e9e393fd30569fb1cdc3f40a9e9f926794a14
push id7566
push usermtabara@mozilla.com
push dateWed, 02 Aug 2017 08:25:16 +0000
treeherdermozilla-beta@86913f512c3c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjrmuizel
bugs1383767
milestone56.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1383767 - guarantee FreeType thread-safety by holding Cairo per-face lock and locking down rasterization. r=jrmuizel MozReview-Commit-ID: DuPRIUBgw4W
gfx/2d/2D.h
gfx/2d/Factory.cpp
gfx/cairo/cairo/src/cairo-ft-font.c
gfx/skia/skia/src/ports/SkFontHost_cairo.cpp
--- a/gfx/2d/2D.h
+++ b/gfx/2d/2D.h
@@ -1640,16 +1640,18 @@ public:
 #endif
 
 #ifdef MOZ_ENABLE_FREETYPE
   static void SetFTLibrary(FT_Library aFTLibrary);
   static FT_Library GetFTLibrary();
 
   static FT_Library NewFTLibrary();
   static void ReleaseFTLibrary(FT_Library aFTLibrary);
+  static void LockFTLibrary(FT_Library aFTLibrary);
+  static void UnlockFTLibrary(FT_Library aFTLibrary);
 
   static FT_Face NewFTFace(FT_Library aFTLibrary, const char* aFileName, int aFaceIndex);
   static FT_Face NewFTFaceFromData(FT_Library aFTLibrary, const uint8_t* aData, size_t aDataSize, int aFaceIndex);
   static void ReleaseFTFace(FT_Face aFace);
 
 private:
   static FT_Library mFTLibrary;
   static Mutex* mFTLock;
--- a/gfx/2d/Factory.cpp
+++ b/gfx/2d/Factory.cpp
@@ -181,16 +181,28 @@ mozilla_NewFTFaceFromData(FT_Library aFT
 }
 
 void
 mozilla_ReleaseFTFace(FT_Face aFace)
 {
   mozilla::gfx::Factory::ReleaseFTFace(aFace);
 }
 
+void
+mozilla_LockFTLibrary(FT_Library aFTLibrary)
+{
+  mozilla::gfx::Factory::LockFTLibrary(aFTLibrary);
+}
+
+void
+mozilla_UnlockFTLibrary(FT_Library aFTLibrary)
+{
+  mozilla::gfx::Factory::UnlockFTLibrary(aFTLibrary);
+}
+
 }
 #endif
 
 namespace mozilla {
 namespace gfx {
 
 // In Gecko, this value is managed by gfx.logging.level in gfxPrefs.
 int32_t LoggingPrefs::sGfxLogLevel = LOG_DEFAULT;
@@ -662,16 +674,30 @@ Factory::NewFTLibrary()
 }
 
 void
 Factory::ReleaseFTLibrary(FT_Library aFTLibrary)
 {
   FT_Done_FreeType(aFTLibrary);
 }
 
+void
+Factory::LockFTLibrary(FT_Library aFTLibrary)
+{
+  MOZ_ASSERT(mFTLock);
+  mFTLock->Lock();
+}
+
+void
+Factory::UnlockFTLibrary(FT_Library aFTLibrary)
+{
+  MOZ_ASSERT(mFTLock);
+  mFTLock->Unlock();
+}
+
 FT_Face
 Factory::NewFTFace(FT_Library aFTLibrary, const char* aFileName, int aFaceIndex)
 {
   MOZ_ASSERT(mFTLock);
   MutexAutoLock lock(*mFTLock);
   if (!aFTLibrary) {
     aFTLibrary = mFTLibrary;
   }
--- a/gfx/cairo/cairo/src/cairo-ft-font.c
+++ b/gfx/cairo/cairo/src/cairo-ft-font.c
@@ -102,16 +102,18 @@ static setLcdFilterFunc setLcdFilter;
 #define MAX_OPEN_FACES 10
 /* This is the maximum font size we allow to be passed to FT_Set_Char_Size
  */
 #define MAX_FONT_SIZE 1000
 
 extern FT_Face mozilla_NewFTFace(FT_Library aFTLibrary, const char* aFileName, int aFaceIndex);
 extern FT_Face mozilla_NewFTFaceFromData(FT_Library aFTLibrary, const uint8_t* aData, size_t aDataSize, int aFaceIndex);
 extern void mozilla_ReleaseFTFace(FT_Face aFace);
+extern void mozilla_LockFTLibrary(FT_Library aFTLibrary);
+extern void mozilla_UnlockFTLibrary(FT_Library aFTLibrary);
 
 /**
  * SECTION:cairo-ft
  * @Title: FreeType Fonts
  * @Short_Description: Font support for FreeType
  * @See_Also: #cairo_font_face_t
  *
  * The FreeType font backend is primarily used to render text on GNU/Linux
@@ -1448,23 +1450,31 @@ static cairo_status_t
           initialized_setLcdFilter = 1;
 #ifdef HAVE_FT_LIBRARY_SETLCDFILTER
 	  setLcdFilter = &FT_Library_SetLcdFilter;
 #else
           setLcdFilter = (setLcdFilterFunc) dlsym(RTLD_DEFAULT, "FT_Library_SetLcdFilter");
 #endif
         }
 
-	if (setLcdFilter)
-          setLcdFilter (library, lcd_filter);
+	if (setLcdFilter &&
+	    (render_mode == FT_RENDER_MODE_LCD ||
+	     render_mode == FT_RENDER_MODE_LCD_V)) {
+	    mozilla_LockFTLibrary (library);
+	    setLcdFilter (library, lcd_filter);
+	}
 
 	fterror = FT_Render_Glyph (face->glyph, render_mode);
 
-	if (setLcdFilter)
-          setLcdFilter (library, FT_LCD_FILTER_NONE);
+	if (setLcdFilter &&
+	    (render_mode == FT_RENDER_MODE_LCD ||
+	     render_mode == FT_RENDER_MODE_LCD_V)) {
+	    setLcdFilter (library, FT_LCD_FILTER_NONE);
+	    mozilla_UnlockFTLibrary (library);
+	}
 
 	if (fterror != 0)
 		return _cairo_error (CAIRO_STATUS_NO_MEMORY);
 
 	bitmap_size = _compute_xrender_bitmap_size (&bitmap,
 						    face->glyph,
 						    render_mode);
 	if (bitmap_size < 0)
@@ -2213,17 +2223,17 @@ static cairo_int_status_t
 _cairo_ft_scaled_glyph_init (void			*abstract_font,
 			     cairo_scaled_glyph_t	*scaled_glyph,
 			     cairo_scaled_glyph_info_t	 info)
 {
     cairo_text_extents_t    fs_metrics;
     cairo_ft_scaled_font_t *scaled_font = abstract_font;
     cairo_ft_unscaled_font_t *unscaled = scaled_font->unscaled;
     FT_GlyphSlot glyph;
-    FT_Face face;
+    FT_Face face = NULL;
     FT_Error error;
     int load_flags = scaled_font->ft_options.load_flags;
     FT_Glyph_Metrics *metrics;
     double x_factor, y_factor;
     cairo_bool_t vertical_layout = FALSE;
     cairo_status_t status;
 
     face = _cairo_ft_unscaled_font_lock_face (unscaled);
@@ -3271,17 +3281,18 @@ cairo_ft_scaled_font_lock_face (cairo_sc
     }
 
     /* Note: We deliberately release the unscaled font's mutex here,
      * so that we are not holding a lock across two separate calls to
      * cairo function, (which would give the application some
      * opportunity for creating deadlock. This is obviously unsafe,
      * but as documented, the user must add manual locking when using
      * this function. */
-     CAIRO_MUTEX_UNLOCK (scaled_font->unscaled->mutex);
+    // BEWARE: Mozilla's tree Cairo keeps the lock across calls for thread-safety.
+    // CAIRO_MUTEX_UNLOCK (scaled_font->unscaled->mutex);
 
     return face;
 }
 
 /**
  * cairo_ft_scaled_font_unlock_face:
  * @scaled_font: A #cairo_scaled_font_t from the FreeType font backend. Such an
  *   object can be created by calling cairo_scaled_font_create() on a
@@ -3302,17 +3313,18 @@ cairo_ft_scaled_font_unlock_face (cairo_
 
     if (scaled_font->base.status)
 	return;
 
     /* Note: We released the unscaled font's mutex at the end of
      * cairo_ft_scaled_font_lock_face, so we have to acquire it again
      * as _cairo_ft_unscaled_font_unlock_face expects it to be held
      * when we call into it. */
-    CAIRO_MUTEX_LOCK (scaled_font->unscaled->mutex);
+    // BEWARE: Mozilla's tree Cairo keeps the lock across calls for thread-safety.
+    //CAIRO_MUTEX_LOCK (scaled_font->unscaled->mutex);
 
     _cairo_ft_unscaled_font_unlock_face (scaled_font->unscaled);
 }
 
 /* We expose our unscaled font implementation internally for the the
  * PDF backend, which needs to keep track of the the different
  * fonts-on-disk used by a document, so it can embed them.
  */
--- a/gfx/skia/skia/src/ports/SkFontHost_cairo.cpp
+++ b/gfx/skia/skia/src/ports/SkFontHost_cairo.cpp
@@ -53,16 +53,21 @@ typedef enum FT_LcdFilter_
 #if SK_CAN_USE_DLOPEN
 #include <dlfcn.h>
 #endif
 
 #ifndef SK_FONTHOST_CAIRO_STANDALONE
 #define SK_FONTHOST_CAIRO_STANDALONE 1
 #endif
 
+extern "C" {
+    extern void mozilla_LockFTLibrary(FT_Library aFTLibrary);
+    extern void mozilla_UnlockFTLibrary(FT_Library aFTLibrary);
+}
+
 static cairo_user_data_key_t kSkTypefaceKey;
 
 static bool gFontHintingEnabled = true;
 static FT_Error (*gSetLcdFilter)(FT_Library, FT_LcdFilter) = nullptr;
 static void (*gGlyphSlotEmbolden)(FT_GlyphSlot) = nullptr;
 
 void SkInitCairoFT(bool fontHintingEnabled)
 {
@@ -739,30 +744,32 @@ void SkScalerContext_CairoFT::generateIm
 
     prepareGlyph(face->glyph);
 
     bool useLcdFilter =
         face->glyph->format == FT_GLYPH_FORMAT_OUTLINE &&
         isLCD(glyph) &&
         gSetLcdFilter;
     if (useLcdFilter) {
+        mozilla_LockFTLibrary(face->glyph->library);
         gSetLcdFilter(face->glyph->library, fLcdFilter);
     }
 
     SkMatrix matrix;
     if (face->glyph->format == FT_GLYPH_FORMAT_BITMAP &&
         fHaveShape) {
         matrix = fShapeMatrix;
     } else {
         matrix.setIdentity();
     }
     generateGlyphImage(face, glyph, matrix);
 
     if (useLcdFilter) {
         gSetLcdFilter(face->glyph->library, FT_LCD_FILTER_NONE);
+        mozilla_UnlockFTLibrary(face->glyph->library);
     }
 }
 
 void SkScalerContext_CairoFT::generatePath(const SkGlyphID glyphID, SkPath* path)
 {
     SkASSERT(fScaledFont != nullptr);
     CairoLockedFTFace faceLock(fScaledFont);
     FT_Face face = faceLock.getFace();