From c8435823ca532d71c02294b1da4532797405297a Mon Sep 17 00:00:00 2001
From: Jonathan Kew <jkew@mozilla.com>
Date: Wed, 24 Apr 2024 12:54:46 +0000
Subject: [PATCH] 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 | 35 +++++++++++++++++++++++++----------
layout/style/FontFaceImpl.h | 7 ++-----
2 files changed, 27 insertions(+), 15 deletions(-)
diff --git a/layout/style/FontFaceImpl.cpp b/layout/style/FontFaceImpl.cpp
index b927fd3f1b27d..bd7e6a595f3a4 100644
--- a/layout/style/FontFaceImpl.cpp
+++ b/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();
}
}
diff --git a/layout/style/FontFaceImpl.h b/layout/style/FontFaceImpl.h
index ec6783540d860..7f1279a248beb 100644
--- a/layout/style/FontFaceImpl.h
+++ b/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);
--
GitLab