Bug 1842674 - Rework gfxCharacterMap management. r=aosmond a=pascalc

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

gfx/thebes/gfxFontEntry.cpp

@@ -54,24 +54,10 @@ using namespace mozilla;
 using namespace mozilla::gfx;
 using namespace mozilla::unicode;
 
-nsrefcnt gfxCharacterMap::NotifyMaybeReleased() {
-  auto* pfl = gfxPlatformFontList::PlatformFontList();
-  pfl->Lock();
-
-  // Something may have pulled our raw pointer out of gfxPlatformFontList before
-  // we were able to complete the release.
-  if (mRefCnt > 0) {
-    pfl->Unlock();
-    return mRefCnt;
-  }
-
-  if (mShared) {
-    pfl->RemoveCmap(this);
-  }
-
-  pfl->Unlock();
-  delete this;
-  return 0;
+void gfxCharacterMap::NotifyMaybeReleased(gfxCharacterMap* aCmap) {
+  // Tell gfxPlatformFontList that a charmap's refcount was decremented,
+  // so it should check whether the object is to be deleted.
+  gfxPlatformFontList::PlatformFontList()->MaybeRemoveCmap(aCmap);
 }
 
 gfxFontEntry::gfxFontEntry(const nsACString& aName, bool aIsStandardFace)

gfx/thebes/gfxFontEntry.h

@@ -68,56 +68,108 @@ typedef struct FT_MM_Var_ FT_MM_Var;
 
 class gfxCharacterMap : public gfxSparseBitSet {
  public:
-  nsrefcnt AddRef() {
+  // gfxCharacterMap instances may be shared across multiple threads via a
+  // global table managed by gfxPlatformFontList. Once a gfxCharacterMap is
+  // inserted in the global table, its mShared flag will be TRUE, and we
+  // cannot safely delete it except from gfxPlatformFontList (which will
+  // use a lock to ensure entries are removed from its table and deleted
+  // safely).
+
+  // AddRef() is pretty much standard. We don't return the refcount as our
+  // users don't care about it.
+  void AddRef() {
+    MOZ_ASSERT_TYPE_OK_FOR_REFCOUNTING(gfxCharacterMap);
     MOZ_ASSERT(int32_t(mRefCnt) >= 0, "illegal refcnt");
-    ++mRefCnt;
-    NS_LOG_ADDREF(this, mRefCnt, "gfxCharacterMap", sizeof(*this));
-    return mRefCnt;
+    [[maybe_unused]] nsrefcnt count = ++mRefCnt;
+    NS_LOG_ADDREF(this, count, "gfxCharacterMap", sizeof(*this));
   }
 
-  nsrefcnt Release() {
-    MOZ_ASSERT(0 != mRefCnt, "dup release");
-    --mRefCnt;
-    NS_LOG_RELEASE(this, mRefCnt, "gfxCharacterMap");
-    if (mRefCnt == 0) {
-      // Because we have a raw pointer in gfxPlatformFontList that we may race
-      // access with, we may not release here.
-      return NotifyMaybeReleased();
+  // Custom Release(): if the object is referenced from the global shared
+  // table, and we're releasing the last *other* reference to it, then we
+  // notify the global table to consider also releasing its ref. (That may
+  // not actually happen, if another thread is racing with us and takes a
+  // new reference, or completes the release first!)
+  void Release() {
+    MOZ_ASSERT(int32_t(mRefCnt) > 0, "dup release");
+    // We can't safely read this after we've decremented mRefCnt, so save it
+    // in a local variable here. Note that the value is never reset to false
+    // once it has been set to true (when recording the cmap in the shared
+    // table), so there's no risk of this resulting in a "false positive" when
+    // tested later. A "false negative" is possible but harmless; it would
+    // just mean we miss an opportunity to release a reference from the shared
+    // cmap table.
+    bool isShared = mShared;
+
+    // Ensure we only access mRefCnt once, for consistency if the object is
+    // being used by multiple threads.
+    nsrefcnt count = --mRefCnt;
+    NS_LOG_RELEASE(this, count, "gfxCharacterMap");
+
+    // If isShared was true, this object has been shared across threads. In
+    // that case, if the refcount went to 1, we notify the shared table so
+    // it can drop its reference and delete the object.
+    if (isShared) {
+      MOZ_ASSERT(count > 0);
+      if (count == 1) {
+        NotifyMaybeReleased(this);
+      }
+      return;
+    }
+
+    // Otherwise, this object hasn't been shared and we can safely delete it
+    // as we must have been holding the only reference. (Note that if we were
+    // holding the only reference, there's no other owner who can have set
+    // mShared to true since we read it above.)
+    if (count == 0) {
+      delete this;
     }
-    return mRefCnt;
   }
 
-  gfxCharacterMap() : mHash(0), mBuildOnTheFly(false), mShared(false) {}
+  gfxCharacterMap() = default;
 
   explicit gfxCharacterMap(const gfxSparseBitSet& aOther)
-      : gfxSparseBitSet(aOther),
-        mHash(0),
-        mBuildOnTheFly(false),
-        mShared(false) {}
-
-  void CalcHash() { mHash = GetChecksum(); }
+      : gfxSparseBitSet(aOther) {}
 
   size_t SizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf) const {
     return gfxSparseBitSet::SizeOfExcludingThis(aMallocSizeOf);
   }
 
   // hash of the cmap bitvector
-  uint32_t mHash;
+  uint32_t mHash = 0;
 
   // if cmap is built on the fly it's never shared
-  bool mBuildOnTheFly;
+  bool mBuildOnTheFly = false;
 
-  // cmap is shared globally
-  bool mShared;
+  // Character map is shared globally. This can only be set by the thread that
+  // originally created the map, as no other thread can get a reference until
+  // it has been shared via the global table.
+  bool mShared = false;
 
  protected:
-  nsrefcnt NotifyMaybeReleased();
+  friend class gfxPlatformFontList;
+
+  // Destructor should not be called except via Release().
+  // (Note that our "friend" gfxPlatformFontList also accesses this from its
+  // MaybeRemoveCmap method.)
+  ~gfxCharacterMap() = default;
+
+  nsrefcnt RefCount() const { return mRefCnt; }
+
+  void CalcHash() { mHash = GetChecksum(); }
+
+  static void NotifyMaybeReleased(gfxCharacterMap* aCmap);
+
+  // Only used when clearing the shared-cmap hashtable during shutdown.
+  void ClearSharedFlag() {
+    MOZ_ASSERT(NS_IsMainThread());
+    mShared = false;
+  }
 
   mozilla::ThreadSafeAutoRefCnt mRefCnt;
 
  private:
-  gfxCharacterMap(const gfxCharacterMap&);
-  gfxCharacterMap& operator=(const gfxCharacterMap&);
+  gfxCharacterMap(const gfxCharacterMap&) = delete;
+  gfxCharacterMap& operator=(const gfxCharacterMap&) = delete;
 };
 
 // Info on an individual font feature, for reporting available features

gfx/thebes/gfxPlatformFontList.cpp

@@ -349,7 +349,17 @@ gfxPlatformFontList::~gfxPlatformFontList() {
   // the initialization thread is using.
   AutoLock lock(mLock);
 
+  // We can't just do mSharedCmaps.Clear() here because removing each item from
+  // the table would drop its last reference, and its Release() method would
+  // then call back to MaybeRemoveCmap to search for it, which we can't do
+  // while in the middle of clearing the table.
+  // So we first clear the "shared" flag in each entry, so Release() won't try
+  // to re-find them in the table.
+  for (auto iter = mSharedCmaps.ConstIter(); !iter.Done(); iter.Next()) {
+    iter.Get()->mCharMap->ClearSharedFlag();
+  }
   mSharedCmaps.Clear();
+
   ClearLangGroupPrefFontsLocked();
 
   NS_ASSERTION(gFontListPrefObserver, "There is no font list pref observer");
@@ -1899,28 +1909,63 @@ fontlist::Pointer gfxPlatformFontList::GetShmemCharMapLocked(
   return entry->GetCharMap();
 }
 
-// lookup cmap in the shared cmap set, adding if not already present
+// Lookup aCmap in the shared cmap set, adding if not already present.
+// This is the only way for a reference to a gfxCharacterMap to be acquired
+// by another thread than its original creator.
 already_AddRefed<gfxCharacterMap> gfxPlatformFontList::FindCharMap(
     gfxCharacterMap* aCmap) {
+  // Lock to prevent potentially racing against MaybeRemoveCmap.
   AutoLock lock(mLock);
+
+  // Find existing entry or insert a new one (which will add a reference).
   aCmap->CalcHash();
-  gfxCharacterMap* cmap = mSharedCmaps.PutEntry(aCmap)->GetKey();
-  cmap->mShared = true;
-  return do_AddRef(cmap);
+  aCmap->mShared = true;  // Set the shared flag in preparation for adding
+                          // to the global table.
+  RefPtr cmap = mSharedCmaps.PutEntry(aCmap)->GetKey();
+
+  // If we ended up finding a different, pre-existing entry, clear the
+  // shared flag on this one so that it'll get deleted on Release().
+  if (cmap.get() != aCmap) {
+    aCmap->mShared = false;
+  }
+
+  return cmap.forget();
 }
 
-// remove the cmap from the shared cmap set
-void gfxPlatformFontList::RemoveCmap(const gfxCharacterMap* aCharMap) {
+// Potentially remove the charmap from the shared cmap set. This is called
+// when a user of the charmap drops a reference and the refcount goes to 1;
+// in that case, it is possible our shared set is the only remaining user
+// of the object, and we should remove it.
+// Note that aCharMap might have already been freed, so we must not try to
+// dereference it until we have checked that it's still present in our table.
+void gfxPlatformFontList::MaybeRemoveCmap(gfxCharacterMap* aCharMap) {
+  // Lock so that nobody else can get a reference via FindCharMap while we're
+  // checking here.
   AutoLock lock(mLock);
-  // skip lookups during teardown
-  if (mSharedCmaps.Count() == 0) {
+
+  // Skip lookups during teardown.
+  if (!mSharedCmaps.Count()) {
     return;
   }
 
-  // cmap needs to match the entry *and* be the same ptr before removing
+  // aCharMap needs to match the entry and be the same ptr and still have a
+  // refcount of exactly 1 (i.e. we hold the only reference) before removing.
+  // If we're racing another thread, it might already have been removed, in
+  // which case GetEntry will not find it and we won't try to dereference the
+  // already-freed pointer.
   CharMapHashKey* found =
       mSharedCmaps.GetEntry(const_cast<gfxCharacterMap*>(aCharMap));
-  if (found && found->GetKey() == aCharMap) {
+  if (found && found->GetKey() == aCharMap && aCharMap->RefCount() == 1) {
+    // Forget our reference to the object that's being deleted, without
+    // calling Release() on it.
+    Unused << found->mCharMap.forget();
+
+    // Do the deletion.
+    delete aCharMap;
+
+    // Log this as a "Release" to keep leak-checking correct.
+    NS_LOG_RELEASE(aCharMap, 0, "gfxCharacterMap");
+
     mSharedCmaps.RemoveEntry(found);
   }
 }

gfx/thebes/gfxPlatformFontList.h

@@ -50,11 +50,11 @@ class CharMapHashKey : public PLDHashEntryHdr {
   }
   MOZ_COUNTED_DTOR(CharMapHashKey)
 
-  gfxCharacterMap* GetKey() const { return mCharMap; }
+  gfxCharacterMap* GetKey() const { return mCharMap.get(); }
 
   bool KeyEquals(const gfxCharacterMap* aCharMap) const {
-    NS_ASSERTION(!aCharMap->mBuildOnTheFly && !mCharMap->mBuildOnTheFly,
-                 "custom cmap used in shared cmap hashtable");
+    MOZ_ASSERT(!aCharMap->mBuildOnTheFly && !mCharMap->mBuildOnTheFly,
+               "custom cmap used in shared cmap hashtable");
     // cmaps built on the fly never match
     if (aCharMap->mHash != mCharMap->mHash) {
       return false;
@@ -72,9 +72,13 @@ class CharMapHashKey : public PLDHashEntryHdr {
   enum { ALLOW_MEMMOVE = true };
 
  protected:
-  // charMaps are not owned by the shared cmap cache, but it will be notified
-  // by gfxCharacterMap::Release() when an entry is about to be deleted
-  gfxCharacterMap* MOZ_NON_OWNING_REF mCharMap;
+  friend class gfxPlatformFontList;
+
+  // gfxCharacterMap::Release() will notify us when the refcount of a
+  // charmap drops to 1; at that point, we'll lock the cache, check if
+  // the charmap is owned by the cache and this is still the only ref,
+  // and if so, delete it.
+  RefPtr<gfxCharacterMap> mCharMap;
 };
 
 /**
@@ -499,8 +503,9 @@ class gfxPlatformFontList : public gfxFontInfoLoader {
   // match is found.
   already_AddRefed<gfxCharacterMap> FindCharMap(gfxCharacterMap* aCmap);
 
-  // Remove the cmap from the shared cmap set.
-  void RemoveCmap(const gfxCharacterMap* aCharMap);
+  // Remove the cmap from the shared cmap set if it holds the only remaining
+  // reference to the object.
+  void MaybeRemoveCmap(gfxCharacterMap* aCharMap);
 
   // Keep track of userfont sets to notify when global fontlist changes occur.
   void AddUserFontSet(gfxUserFontSet* aUserFontSet) {