From f3254dc7efbca6438469052e6cb5c15ad4317f8a Mon Sep 17 00:00:00 2001 From: Lee Salzman Date: Wed, 26 Jul 2017 23:24:44 -0400 Subject: [PATCH] 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 | 2 ++ gfx/2d/Factory.cpp | 26 ++++++++++++++++++++ gfx/cairo/cairo/src/cairo-ft-font.c | 26 ++++++++++++++------ gfx/skia/skia/src/ports/SkFontHost_cairo.cpp | 7 ++++++ 4 files changed, 54 insertions(+), 7 deletions(-) diff --git a/gfx/2d/2D.h b/gfx/2d/2D.h index 5ee7755b85aa..0ecbd1d2b3bd 100644 --- a/gfx/2d/2D.h +++ b/gfx/2d/2D.h @@ -1645,6 +1645,8 @@ public: 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); diff --git a/gfx/2d/Factory.cpp b/gfx/2d/Factory.cpp index a22e4b383e35..ac96e9cc5550 100644 --- a/gfx/2d/Factory.cpp +++ b/gfx/2d/Factory.cpp @@ -186,6 +186,18 @@ 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 @@ -667,6 +679,20 @@ 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) { diff --git a/gfx/cairo/cairo/src/cairo-ft-font.c b/gfx/cairo/cairo/src/cairo-ft-font.c index 57369ca4e16a..94a33557286b 100644 --- a/gfx/cairo/cairo/src/cairo-ft-font.c +++ b/gfx/cairo/cairo/src/cairo-ft-font.c @@ -107,6 +107,8 @@ static setLcdFilterFunc setLcdFilter; 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 @@ -1453,13 +1455,21 @@ _render_glyph_outline (FT_Face face, #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); @@ -2218,7 +2228,7 @@ _cairo_ft_scaled_glyph_init (void *abstract_font, 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; @@ -3276,7 +3286,8 @@ cairo_ft_scaled_font_lock_face (cairo_scaled_font_t *abstract_font) * 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; } @@ -3307,7 +3318,8 @@ cairo_ft_scaled_font_unlock_face (cairo_scaled_font_t *abstract_font) * 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); } diff --git a/gfx/skia/skia/src/ports/SkFontHost_cairo.cpp b/gfx/skia/skia/src/ports/SkFontHost_cairo.cpp index 53efa544ef89..42da19280fce 100644 --- a/gfx/skia/skia/src/ports/SkFontHost_cairo.cpp +++ b/gfx/skia/skia/src/ports/SkFontHost_cairo.cpp @@ -58,6 +58,11 @@ typedef enum FT_LcdFilter_ #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; @@ -744,6 +749,7 @@ void SkScalerContext_CairoFT::generateImage(const SkGlyph& glyph) isLCD(glyph) && gSetLcdFilter; if (useLcdFilter) { + mozilla_LockFTLibrary(face->glyph->library); gSetLcdFilter(face->glyph->library, fLcdFilter); } @@ -758,6 +764,7 @@ void SkScalerContext_CairoFT::generateImage(const SkGlyph& glyph) if (useLcdFilter) { gSetLcdFilter(face->glyph->library, FT_LCD_FILTER_NONE); + mozilla_UnlockFTLibrary(face->glyph->library); } }