Bug 1878199 - patch 2 - If the FontFaceImpl has a user-font entry, hold its...

Bug 1878199 - patch 2 - If the FontFaceImpl has a user-font entry, hold its lock during Add/Remove font-set operations.  a=dmeehan

This should fix the intermittently-reported race here, by ensuring that access into
the FontFaceImpl from GetUserFontSets(), called by the main thread, cannot race with
changes being made by the AddFontSet()/RemoveFontSet() methods.

(If the FontFaceImpl doesn't have an mUserFontEntry yet, then these methods don't
need to lock, as only the owning thread will be touching it.)

Original Revision: https://phabricator.services.mozilla.com/D207296

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

layout/style/FontFaceImpl.cpp

@@ -676,25 +676,40 @@ bool FontFaceImpl::IsInFontFaceSet(FontFaceSetImpl* aFontFaceSet) const {
 void FontFaceImpl::AddFontFaceSet(FontFaceSetImpl* aFontFaceSet) {
   MOZ_ASSERT(!IsInFontFaceSet(aFontFaceSet));
 
-  if (mFontFaceSet == aFontFaceSet) {
-    mInFontFaceSet = true;
+  auto doAddFontFaceSet = [&]() {
+    if (mFontFaceSet == aFontFaceSet) {
+      mInFontFaceSet = true;
+    } else {
+      mOtherFontFaceSets.AppendElement(aFontFaceSet);
+    }
+  };
+
+  if (mUserFontEntry) {
+    AutoWriteLock lock(mUserFontEntry->Lock());
+    doAddFontFaceSet();
   } else {
-    mOtherFontFaceSets.AppendElement(aFontFaceSet);
+    doAddFontFaceSet();
   }
 }
 
 void FontFaceImpl::RemoveFontFaceSet(FontFaceSetImpl* aFontFaceSet) {
   MOZ_ASSERT(IsInFontFaceSet(aFontFaceSet));
 
-  if (mFontFaceSet == aFontFaceSet) {
-    mInFontFaceSet = false;
-  } else {
-    mOtherFontFaceSets.RemoveElement(aFontFaceSet);
-  }
+  auto doRemoveFontFaceSet = [&]() {
+    if (mFontFaceSet == aFontFaceSet) {
+      mInFontFaceSet = false;
+    } else {
+      mOtherFontFaceSets.RemoveElement(aFontFaceSet);
+    }
+  };
 
-  // The caller should be holding a strong reference to the FontFaceSetImpl.
   if (mUserFontEntry) {
-    mUserFontEntry->CheckUserFontSet();
+    AutoWriteLock lock(mUserFontEntry->Lock());
+    doRemoveFontFaceSet();
+    // The caller should be holding a strong reference to the FontFaceSetImpl.
+    mUserFontEntry->CheckUserFontSetLocked();
+  } else {
+    doRemoveFontFaceSet();
   }
 }
 

layout/style/FontFaceImpl.h

@@ -56,11 +56,6 @@ class FontFaceImpl final {
     void GetUserFontSets(nsTArray<RefPtr<gfxUserFontSet>>& aResult) override;
     already_AddRefed<gfxUserFontSet> GetUserFontSet() const override;
 
-    void CheckUserFontSet() {
-      AutoWriteLock lock(mLock);
-      CheckUserFontSetLocked();
-    }
-
 #ifdef DEBUG
     bool HasUserFontSet(gfxUserFontSet* aFontSet) const {
       AutoReadLock lock(mLock);
@@ -72,6 +67,8 @@ class FontFaceImpl final {
     void RemoveFontFace(FontFaceImpl* aOwner);
     void FindFontFaceOwners(nsTHashSet<FontFace*>& aOwners);
 
+    RWLock& Lock() const MOZ_RETURN_CAPABILITY(mLock) { return mLock; }
+
    protected:
     void CheckUserFontSetLocked() MOZ_REQUIRES(mLock);