Bug 1766875 - Guard access to the mCmapCache in gfxFT2FontEntryBase. r=lsalzman

Differential Revision: https://phabricator.services.mozilla.com/D144986

gfx/thebes/gfxFT2FontBase.cpp

@@ -58,25 +58,6 @@ void gfxFT2FontBase::UnlockFTFace() const
   mFTFace->Unlock();
 }
 
-gfxFT2FontEntryBase::CmapCacheSlot* gfxFT2FontEntryBase::GetCmapCacheSlot(
-    uint32_t aCharCode) {
-  // This cache algorithm and size is based on what is done in
-  // cairo_scaled_font_text_to_glyphs and pango_fc_font_real_get_glyph.  I
-  // think the concept is that adjacent characters probably come mostly from
-  // one Unicode block.  This assumption is probably not so valid with
-  // scripts with large character sets as used for East Asian languages.
-  if (!mCmapCache) {
-    mCmapCache = mozilla::MakeUnique<CmapCacheSlot[]>(kNumCmapCacheSlots);
-
-    // Invalidate slot 0 by setting its char code to something that would
-    // never end up in slot 0.  All other slots are already invalid
-    // because they have mCharCode = 0 and a glyph for char code 0 will
-    // always be in the slot 0.
-    mCmapCache[0].mCharCode = 1;
-  }
-  return &mCmapCache[aCharCode % kNumCmapCacheSlots];
-}
-
 static FT_ULong GetTableSizeFromFTFace(SharedFTFace* aFace,
                                        uint32_t aTableTag) {
   if (!aFace) {
@@ -112,17 +93,44 @@ nsresult gfxFT2FontEntryBase::CopyFaceTable(SharedFTFace* aFace,
   return NS_OK;
 }
 
-uint32_t gfxFT2FontBase::GetGlyph(uint32_t aCharCode) {
-  // FcFreeTypeCharIndex needs to lock the FT_Face and can end up searching
-  // through all the postscript glyph names in the font.  Therefore use a
-  // lightweight cache, which is stored on the font entry.
-  auto* slot = static_cast<gfxFT2FontEntryBase*>(mFontEntry.get())
-                   ->GetCmapCacheSlot(aCharCode);
-  if (slot->mCharCode != aCharCode) {
-    slot->mCharCode = aCharCode;
-    slot->mGlyphIndex = gfxFT2LockedFace(this).GetGlyph(aCharCode);
+uint32_t gfxFT2FontEntryBase::GetGlyph(uint32_t aCharCode,
+                                       gfxFT2FontBase* aFont) {
+  const uint32_t slotIndex = aCharCode % kNumCmapCacheSlots;
+  {
+    // Try to read a cached entry without taking an exclusive lock.
+    AutoReadLock lock(mLock);
+    if (mCmapCache) {
+      const auto& slot = mCmapCache[slotIndex];
+      if (slot.mCharCode == aCharCode) {
+        return slot.mGlyphIndex;
+      }
+    }
   }
-  return slot->mGlyphIndex;
+
+  // Create/update the charcode-to-glyphid cache.
+  AutoWriteLock lock(mLock);
+
+  // This cache algorithm and size is based on what is done in
+  // cairo_scaled_font_text_to_glyphs and pango_fc_font_real_get_glyph.  I
+  // think the concept is that adjacent characters probably come mostly from
+  // one Unicode block.  This assumption is probably not so valid with
+  // scripts with large character sets as used for East Asian languages.
+  if (!mCmapCache) {
+    mCmapCache = mozilla::MakeUnique<CmapCacheSlot[]>(kNumCmapCacheSlots);
+
+    // Invalidate slot 0 by setting its char code to something that would
+    // never end up in slot 0.  All other slots are already invalid
+    // because they have mCharCode = 0 and a glyph for char code 0 will
+    // always be in the slot 0.
+    mCmapCache[0].mCharCode = 1;
+  }
+
+  auto& slot = mCmapCache[slotIndex];
+  if (slot.mCharCode != aCharCode) {
+    slot.mCharCode = aCharCode;
+    slot.mGlyphIndex = gfxFT2LockedFace(aFont).GetGlyph(aCharCode);
+  }
+  return slot.mGlyphIndex;
 }
 
 // aScale is intended for a 16.16 x/y_scale of an FT_Size_Metrics
@@ -743,10 +751,6 @@ const gfxFT2FontBase::GlyphMetrics& gfxFT2FontBase::GetCachedGlyphMetrics(
   });
 }
 
-int32_t gfxFT2FontBase::GetGlyphWidth(uint16_t aGID) {
-  return GetCachedGlyphMetrics(aGID).mAdvance;
-}
-
 bool gfxFT2FontBase::GetGlyphBounds(uint16_t aGID, gfxRect* aBounds,
                                     bool aTight) const {
   IntRect bounds;

gfx/thebes/gfxFT2FontBase.h

@@ -14,18 +14,13 @@
 #include "nsTHashMap.h"
 #include "nsHashKeys.h"
 
+class gfxFT2FontBase;
+
 class gfxFT2FontEntryBase : public gfxFontEntry {
  public:
   explicit gfxFT2FontEntryBase(const nsACString& aName) : gfxFontEntry(aName) {}
 
-  struct CmapCacheSlot {
-    CmapCacheSlot() : mCharCode(0), mGlyphIndex(0) {}
-
-    uint32_t mCharCode;
-    uint32_t mGlyphIndex;
-  };
-
-  CmapCacheSlot* GetCmapCacheSlot(uint32_t aCharCode);
+  uint32_t GetGlyph(uint32_t aCharCode, gfxFT2FontBase* aFont);
 
   static bool FaceHasTable(mozilla::gfx::SharedFTFace*, uint32_t aTableTag);
   static nsresult CopyFaceTable(mozilla::gfx::SharedFTFace*, uint32_t aTableTag,
@@ -34,7 +29,14 @@ class gfxFT2FontEntryBase : public gfxFontEntry {
  private:
   enum { kNumCmapCacheSlots = 256 };
 
-  mozilla::UniquePtr<CmapCacheSlot[]> mCmapCache;
+  struct CmapCacheSlot {
+    CmapCacheSlot() : mCharCode(0), mGlyphIndex(0) {}
+
+    uint32_t mCharCode;
+    uint32_t mGlyphIndex;
+  };
+
+  mozilla::UniquePtr<CmapCacheSlot[]> mCmapCache GUARDED_BY(mLock);
 };
 
 class gfxFT2FontBase : public gfxFont {
@@ -45,12 +47,20 @@ class gfxFT2FontBase : public gfxFont {
       const gfxFontStyle* aFontStyle, int aLoadFlags, bool aEmbolden);
   virtual ~gfxFT2FontBase();
 
-  uint32_t GetGlyph(uint32_t aCharCode);
+  uint32_t GetGlyph(uint32_t aCharCode) {
+    auto* entry = static_cast<gfxFT2FontEntryBase*>(mFontEntry.get());
+    return entry->GetGlyph(aCharCode, this);
+  }
+
   bool ProvidesGetGlyph() const override { return true; }
   virtual uint32_t GetGlyph(uint32_t unicode,
                             uint32_t variation_selector) override;
+
   bool ProvidesGlyphWidths() const override { return true; }
-  int32_t GetGlyphWidth(uint16_t aGID) override;
+  int32_t GetGlyphWidth(uint16_t aGID) override {
+    return GetCachedGlyphMetrics(aGID).mAdvance;
+  }
+
   bool GetGlyphBounds(uint16_t aGID, gfxRect* aBounds,
                       bool aTight) const override;