Bug 1873577 - Improve locking in MacOSFontEntry. r=lsalzman, a=RyanVM

Differential Revision: https://phabricator.services.mozilla.com/D199380
iaxat · Jan 23, 2024 · 0f3acaa · 0f3acaa
1 parent 1913c82
commit 0f3acaa
Show file tree

Hide file tree

Showing 2 changed files with 34 additions and 15 deletions.
diff --git a/gfx/thebes/gfxMacPlatformFontList.h b/gfx/thebes/gfxMacPlatformFontList.h
@@ -54,7 +54,7 @@ class MacOSFontEntry final : public gfxFontEntry {
   // returns it; if not, the instance returned will be owned solely by the
   // caller.)
   // Note that in the case of a broken font, this could return null.
-  CGFontRef CreateOrCopyFontRef();
+  CGFontRef CreateOrCopyFontRef() MOZ_REQUIRES_SHARED(mLock);
 
   // override gfxFontEntry table access function to bypass table cache,
   // use CGFontRef API to get direct access to system font data
@@ -84,19 +84,19 @@ class MacOSFontEntry final : public gfxFontEntry {
 
   static void DestroyBlobFunc(void* aUserData);
 
-  CGFontRef
-      mFontRef;  // owning reference to the CGFont, released on destruction
+  CGFontRef mFontRef MOZ_GUARDED_BY(mLock);  // owning reference
 
-  double mSizeHint;
+  const double mSizeHint;
 
-  bool mFontRefInitialized;
-  bool mRequiresAAT;
-  bool mIsCFF;
-  bool mIsCFFInitialized;
-  bool mHasVariations;
-  bool mHasVariationsInitialized;
-  bool mHasAATSmallCaps;
-  bool mHasAATSmallCapsInitialized;
+  bool mFontRefInitialized MOZ_GUARDED_BY(mLock);
+
+  mozilla::Atomic<bool> mRequiresAAT;
+  mozilla::Atomic<bool> mIsCFF;
+  mozilla::Atomic<bool> mIsCFFInitialized;
+  mozilla::Atomic<bool> mHasVariations;
+  mozilla::Atomic<bool> mHasVariationsInitialized;
+  mozilla::Atomic<bool> mHasAATSmallCaps;
+  mozilla::Atomic<bool> mHasAATSmallCapsInitialized;
 
   // To work around Core Text's mishandling of the default value for 'opsz',
   // we need to record whether the font has an a optical size axis, what its
@@ -105,10 +105,10 @@ class MacOSFontEntry final : public gfxFontEntry {
   // These fields are used by gfxMacFont, but stored in the font entry so
   // that only a single font instance needs to inspect the available
   // variations.
-  gfxFontVariationAxis mOpszAxis;
-  float mAdjustedDefaultOpsz;
+  gfxFontVariationAxis mOpszAxis MOZ_GUARDED_BY(mLock);
+  float mAdjustedDefaultOpsz MOZ_GUARDED_BY(mLock);
 
-  nsTHashtable<nsUint32HashKey> mAvailableTables;
+  nsTHashtable<nsUint32HashKey> mAvailableTables MOZ_GUARDED_BY(mLock);
 
   mozilla::ThreadSafeWeakPtr<mozilla::gfx::UnscaledFontMac> mUnscaledFont;
 };

diff --git a/gfx/thebes/gfxMacPlatformFontList.mm b/gfx/thebes/gfxMacPlatformFontList.mm
@@ -552,6 +552,13 @@ static void GetStringForNSString(const NSString* aSrc, nsAString& aDest) {
 }
 
 CGFontRef MacOSFontEntry::GetFontRef() {
+  {
+    AutoReadLock lock(mLock);
+    if (mFontRefInitialized) {
+      return mFontRef;
+    }
+  }
+  AutoWriteLock lock(mLock);
   if (!mFontRefInitialized) {
     // Cache the CGFontRef, to be released by our destructor.
     mFontRef = CreateOrCopyFontRef();
@@ -616,7 +623,9 @@ static void GetStringForNSString(const NSString* aSrc, nsAString& aDest) {
 }
 
 hb_blob_t* MacOSFontEntry::GetFontTable(uint32_t aTag) {
+  mLock.ReadLock();
   AutoCFRelease<CGFontRef> fontRef = CreateOrCopyFontRef();
+  mLock.ReadUnlock();
   if (!fontRef) {
     return nullptr;
   }
@@ -637,6 +646,16 @@ new FontTableRec(dataRef),
 }
 
 bool MacOSFontEntry::HasFontTable(uint32_t aTableTag) {
+  {
+    // If we've already initialized mAvailableTables, we can return without
+    // needing to take an exclusive lock.
+    AutoReadLock lock(mLock);
+    if (mAvailableTables.Count()) {
+      return mAvailableTables.GetEntry(aTableTag);
+    }
+  }
+
+  AutoWriteLock lock(mLock);
   if (mAvailableTables.Count() == 0) {
     nsAutoreleasePool localPool;