Skip to content
This repository was archived by the owner on Jul 9, 2025. It is now read-only.

Commit 9f767ee

Browse files
committed
Bug 1779519 - gfxFontCache expiration tracker operations should be more atomic. r=jfkthame
gfxFontCache acquires and releases its mutex during various operations. In order to keep the state internally consistent, we should only release the lock after the full operation is complete. This involves moving the deletion of gfxFont to outside the lock via a temporary discard array. The expiration state should not be protected by the gfxFont's mutex since we don't hold it during most operations. Instead we should hold gfxFontCache's mutex because then we can guarantee the operation is atomic, particularly when a worker wants a font, and the main thread is aging the generations. When a font is returned from gfxFontCache, we now return it already removed from the tracker, and with its refcount updated. This avoids any potential races between the expiration timer and a worker accessing the font, as well as simplying the callers so they don't need to be aware of addref-ing manually in case the result is to be discarded (so that it gets readded to the tracker). Differential Revision: https://phabricator.services.mozilla.com/D151821
1 parent c69c9f0 commit 9f767ee

20 files changed

+280
-277
lines changed

accessible/base/TextAttrs.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -410,7 +410,7 @@ bool TextAttrsMgr::FontFamilyTextAttr::GetFontFamily(nsIFrame* aFrame,
410410
nsLayoutUtils::GetFontMetricsForFrame(aFrame, 1.0f);
411411

412412
gfxFontGroup* fontGroup = fm->GetThebesFontGroup();
413-
gfxFont* font = fontGroup->GetFirstValidFont();
413+
RefPtr<gfxFont> font = fontGroup->GetFirstValidFont();
414414
gfxFontEntry* fontEntry = font->GetFontEntry();
415415
aFamily.Append(NS_ConvertUTF8toUTF16(fontEntry->FamilyName()));
416416
return true;
@@ -551,7 +551,7 @@ FontWeight TextAttrsMgr::FontWeightTextAttr::GetFontWeight(nsIFrame* aFrame) {
551551
nsLayoutUtils::GetFontMetricsForFrame(aFrame, 1.0f);
552552

553553
gfxFontGroup* fontGroup = fm->GetThebesFontGroup();
554-
gfxFont* font = fontGroup->GetFirstValidFont();
554+
RefPtr<gfxFont> font = fontGroup->GetFirstValidFont();
555555

556556
// When there doesn't exist a bold font in the family and so the rendering of
557557
// a non-bold font face is changed so that the user sees what looks like a

dom/canvas/CanvasRenderingContext2D.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4245,9 +4245,9 @@ TextMetrics* CanvasRenderingContext2D::DrawOrMeasureText(
42454245

42464246
processor.mFontgrp
42474247
->UpdateUserFonts(); // ensure user font generation is current
4248+
RefPtr<gfxFont> font = processor.mFontgrp->GetFirstValidFont();
42484249
const gfxFont::Metrics& fontMetrics =
4249-
processor.mFontgrp->GetFirstValidFont()->GetMetrics(
4250-
nsFontMetrics::eHorizontal);
4250+
font->GetMetrics(nsFontMetrics::eHorizontal);
42514251

42524252
// calls bidi algo twice since it needs the full text width and the
42534253
// bounding boxes before rendering anything

gfx/src/nsFontMetrics.cpp

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -159,8 +159,9 @@ void nsFontMetrics::Destroy() { mPresContext = nullptr; }
159159

160160
static const gfxFont::Metrics& GetMetrics(
161161
nsFontMetrics* aFontMetrics, nsFontMetrics::FontOrientation aOrientation) {
162-
return aFontMetrics->GetThebesFontGroup()->GetFirstValidFont()->GetMetrics(
163-
aOrientation);
162+
RefPtr<gfxFont> font =
163+
aFontMetrics->GetThebesFontGroup()->GetFirstValidFont();
164+
return font->GetMetrics(aOrientation);
164165
}
165166

166167
static const gfxFont::Metrics& GetMetrics(nsFontMetrics* aFontMetrics) {

gfx/thebes/gfxFont.cpp

Lines changed: 72 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -231,21 +231,29 @@ bool gfxFontCache::HashEntry::KeyEquals(const KeyTypePointer aKey) const {
231231
aKey->mUnicodeRangeMap->Equals(fontUnicodeRangeMap)));
232232
}
233233

234-
gfxFont* gfxFontCache::Lookup(const gfxFontEntry* aFontEntry,
235-
const gfxFontStyle* aStyle,
236-
const gfxCharacterMap* aUnicodeRangeMap) {
234+
already_AddRefed<gfxFont> gfxFontCache::Lookup(
235+
const gfxFontEntry* aFontEntry, const gfxFontStyle* aStyle,
236+
const gfxCharacterMap* aUnicodeRangeMap) {
237237
MutexAutoLock lock(mMutex);
238238

239239
Key key(aFontEntry, aStyle, aUnicodeRangeMap);
240240
HashEntry* entry = mFonts.GetEntry(key);
241241

242242
Telemetry::Accumulate(Telemetry::FONT_CACHE_HIT, entry != nullptr);
243243

244-
return entry ? entry->mFont : nullptr;
244+
if (!entry) {
245+
return nullptr;
246+
}
247+
248+
RefPtr<gfxFont> font = entry->mFont;
249+
if (font->GetExpirationState()->IsTracked()) {
250+
RemoveObjectLocked(font, lock);
251+
}
252+
return font.forget();
245253
}
246254

247255
void gfxFontCache::AddNew(gfxFont* aFont) {
248-
gfxFont* oldFont;
256+
nsTArray<gfxFont*> discard;
249257
{
250258
MutexAutoLock lock(mMutex);
251259

@@ -255,28 +263,39 @@ void gfxFontCache::AddNew(gfxFont* aFont) {
255263
if (!entry) {
256264
return;
257265
}
258-
oldFont = entry->mFont;
266+
gfxFont* oldFont = entry->mFont;
259267
entry->mFont = aFont;
260268
// Assert that we can find the entry we just put in (this fails if the key
261269
// has a NaN float value in it, e.g. 'sizeAdjust').
262270
MOZ_ASSERT(entry == mFonts.GetEntry(key));
263-
}
264271

265-
// If someone's asked us to replace an existing font entry, then that's a
266-
// bit weird, but let it happen, and expire the old font if it's not used.
267-
if (oldFont && oldFont->GetExpirationState()->IsTracked()) {
268-
// if oldFont == aFont, recount should be > 0,
269-
// so we shouldn't be here.
270-
NS_ASSERTION(aFont != oldFont, "new font is tracked for expiry!");
271-
NotifyExpired(oldFont);
272+
// If someone's asked us to replace an existing font entry, then that's a
273+
// bit weird, but let it happen, and expire the old font if it's not used.
274+
if (oldFont && oldFont->GetExpirationState()->IsTracked()) {
275+
// if oldFont == aFont, recount should be > 0,
276+
// so we shouldn't be here.
277+
NS_ASSERTION(aFont != oldFont, "new font is tracked for expiry!");
278+
NotifyExpiredLocked(oldFont, lock);
279+
discard = std::move(mTrackerDiscard);
280+
}
272281
}
282+
DestroyDiscard(discard);
273283
}
274284

275285
void gfxFontCache::NotifyReleased(gfxFont* aFont) {
276-
if (NS_FAILED(AddObject(aFont))) {
286+
nsTArray<gfxFont*> discard;
287+
{
288+
MutexAutoLock lock(mMutex);
289+
if (NS_SUCCEEDED(AddObjectLocked(aFont, lock))) {
290+
return;
291+
}
292+
277293
// We couldn't track it for some reason. Kill it now.
278-
DestroyFont(aFont);
294+
DestroyFontLocked(aFont);
295+
discard = std::move(mTrackerDiscard);
279296
}
297+
298+
DestroyDiscard(discard);
280299
// Note that we might have fonts that aren't in the hashtable, perhaps because
281300
// of OOM adding to the hashtable or because someone did an AddNew where
282301
// we already had a font. These fonts are added to the expiration tracker
@@ -285,15 +304,17 @@ void gfxFontCache::NotifyReleased(gfxFont* aFont) {
285304
}
286305

287306
void gfxFontCache::NotifyExpiredLocked(gfxFont* aFont, const AutoLock& aLock) {
288-
aFont->ClearCachedWords();
289307
RemoveObjectLocked(aFont, aLock);
290308
DestroyFontLocked(aFont);
291309
}
292310

293-
void gfxFontCache::NotifyExpired(gfxFont* aFont) {
294-
aFont->ClearCachedWords();
295-
RemoveObject(aFont);
296-
DestroyFont(aFont);
311+
void gfxFontCache::NotifyHandlerEnd() {
312+
nsTArray<gfxFont*> discard;
313+
{
314+
MutexAutoLock lock(mMutex);
315+
discard = std::move(mTrackerDiscard);
316+
}
317+
DestroyDiscard(discard);
297318
}
298319

299320
void gfxFontCache::DestroyFontLocked(gfxFont* aFont) {
@@ -305,23 +326,38 @@ void gfxFontCache::DestroyFontLocked(gfxFont* aFont) {
305326
}
306327
NS_ASSERTION(aFont->GetRefCount() == 0,
307328
"Destroying with non-zero ref count!");
308-
MutexAutoUnlock unlock(mMutex);
309-
delete aFont;
329+
mTrackerDiscard.AppendElement(aFont);
330+
}
331+
332+
void gfxFontCache::DestroyDiscard(nsTArray<gfxFont*>& aDiscard) {
333+
for (auto* font : aDiscard) {
334+
NS_ASSERTION(font->GetRefCount() == 0,
335+
"Destroying with non-zero ref count!");
336+
font->ClearCachedWords();
337+
delete font;
338+
}
339+
aDiscard.Clear();
310340
}
311341

312-
void gfxFontCache::DestroyFont(gfxFont* aFont) {
342+
void gfxFontCache::Flush() {
343+
nsTArray<gfxFont*> discard;
313344
{
314345
MutexAutoLock lock(mMutex);
315-
Key key(aFont->GetFontEntry(), aFont->GetStyle(),
316-
aFont->GetUnicodeRangeMap());
317-
HashEntry* entry = mFonts.GetEntry(key);
318-
if (entry && entry->mFont == aFont) {
319-
mFonts.RemoveEntry(entry);
320-
}
346+
mFonts.Clear();
347+
AgeAllGenerationsLocked(lock);
348+
discard = std::move(mTrackerDiscard);
321349
}
322-
NS_ASSERTION(aFont->GetRefCount() == 0,
323-
"Destroying with non-zero ref count!");
324-
delete aFont;
350+
DestroyDiscard(discard);
351+
}
352+
353+
void gfxFontCache::AgeAllGenerations() {
354+
nsTArray<gfxFont*> discard;
355+
{
356+
MutexAutoLock lock(mMutex);
357+
AgeAllGenerationsLocked(lock);
358+
discard = std::move(mTrackerDiscard);
359+
}
360+
DestroyDiscard(discard);
325361
}
326362

327363
/*static*/
@@ -3581,15 +3617,16 @@ bool gfxFont::InitFakeSmallCapsRun(
35813617
aSyntheticUpper);
35823618
}
35833619

3584-
gfxFont* gfxFont::GetSmallCapsFont() const {
3620+
already_AddRefed<gfxFont> gfxFont::GetSmallCapsFont() const {
35853621
gfxFontStyle style(*GetStyle());
35863622
style.size *= SMALL_CAPS_SCALE_FACTOR;
35873623
style.variantCaps = NS_FONT_VARIANT_CAPS_NORMAL;
35883624
gfxFontEntry* fe = GetFontEntry();
35893625
return fe->FindOrMakeFont(&style, mUnicodeRangeMap);
35903626
}
35913627

3592-
gfxFont* gfxFont::GetSubSuperscriptFont(int32_t aAppUnitsPerDevPixel) const {
3628+
already_AddRefed<gfxFont> gfxFont::GetSubSuperscriptFont(
3629+
int32_t aAppUnitsPerDevPixel) const {
35933630
gfxFontStyle style(*GetStyle());
35943631
style.AdjustForSubSuperscript(aAppUnitsPerDevPixel);
35953632
gfxFontEntry* fe = GetFontEntry();

gfx/thebes/gfxFont.h

Lines changed: 15 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -317,8 +317,9 @@ class gfxFontCache final
317317

318318
// Look up a font in the cache. Returns null if there's nothing matching
319319
// in the cache
320-
gfxFont* Lookup(const gfxFontEntry* aFontEntry, const gfxFontStyle* aStyle,
321-
const gfxCharacterMap* aUnicodeRangeMap);
320+
already_AddRefed<gfxFont> Lookup(const gfxFontEntry* aFontEntry,
321+
const gfxFontStyle* aStyle,
322+
const gfxCharacterMap* aUnicodeRangeMap);
322323

323324
// We created a new font (presumably because Lookup returned null);
324325
// put it in the cache. The font's refcount should be nonzero. It is
@@ -334,13 +335,7 @@ class gfxFontCache final
334335
// Cleans out the hashtable and removes expired fonts waiting for cleanup.
335336
// Other gfxFont objects may be still in use but they will be pushed
336337
// into the expiration queues and removed.
337-
void Flush() {
338-
{
339-
mozilla::MutexAutoLock lock(mMutex);
340-
mFonts.Clear();
341-
}
342-
AgeAllGenerations();
343-
}
338+
void Flush();
344339

345340
void FlushShapedWordCaches();
346341
void NotifyGlyphsChanged();
@@ -374,15 +369,7 @@ class gfxFontCache final
374369
void AddSizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf,
375370
FontCacheSizes* aSizes) const;
376371

377-
void AgeAllGenerations() {
378-
AutoLock lock(mMutex);
379-
AgeAllGenerationsLocked(lock);
380-
}
381-
382-
void RemoveObject(gfxFont* aFont) {
383-
AutoLock lock(mMutex);
384-
RemoveObjectLocked(aFont, lock);
385-
}
372+
void AgeAllGenerations();
386373

387374
protected:
388375
class MemoryReporter final : public nsIMemoryReporter {
@@ -411,10 +398,10 @@ class gfxFontCache final
411398
// font; we just delete it.
412399
void NotifyExpiredLocked(gfxFont* aFont, const AutoLock&)
413400
REQUIRES(mMutex) override;
414-
void NotifyExpired(gfxFont* aFont);
401+
void NotifyHandlerEnd() override;
415402

416-
void DestroyFont(gfxFont* aFont);
417403
void DestroyFontLocked(gfxFont* aFont) REQUIRES(mMutex);
404+
void DestroyDiscard(nsTArray<gfxFont*>& aDiscard);
418405

419406
static gfxFontCache* gGlobalCache;
420407

@@ -455,6 +442,8 @@ class gfxFontCache final
455442

456443
nsTHashtable<HashEntry> mFonts GUARDED_BY(mMutex);
457444

445+
nsTArray<gfxFont*> mTrackerDiscard GUARDED_BY(mMutex);
446+
458447
static void WordCacheExpirationTimerCallback(nsITimer* aTimer, void* aCache);
459448

460449
nsCOMPtr<nsITimer> mWordCacheExpirationTimer GUARDED_BY(mMutex);
@@ -1439,14 +1428,6 @@ class gfxFont {
14391428
14401429
nsrefcnt AddRef(void) {
14411430
MOZ_ASSERT(int32_t(mRefCnt) >= 0, "illegal refcnt");
1442-
nsExpirationState state;
1443-
{
1444-
mozilla::AutoReadLock lock(mLock);
1445-
state = mExpirationState;
1446-
}
1447-
if (state.IsTracked()) {
1448-
gfxFontCache::GetCache()->RemoveObject(this);
1449-
}
14501431
++mRefCnt;
14511432
NS_LOG_ADDREF(this, mRefCnt, "gfxFont", sizeof(*this));
14521433
return mRefCnt;
@@ -1966,7 +1947,8 @@ class gfxFont {
19661947
19671948
// Return a cloned font resized and offset to simulate sub/superscript
19681949
// glyphs. This does not add a reference to the returned font.
1969-
gfxFont* GetSubSuperscriptFont(int32_t aAppUnitsPerDevPixel) const;
1950+
already_AddRefed<gfxFont> GetSubSuperscriptFont(
1951+
int32_t aAppUnitsPerDevPixel) const;
19701952
19711953
bool HasColorGlyphFor(uint32_t aCh, uint32_t aNextCh);
19721954
@@ -2017,7 +1999,7 @@ class gfxFont {
20171999
// Return a font that is a "clone" of this one, but reduced to 80% size
20182000
// (and with variantCaps set to normal). This does not add a reference to
20192001
// the returned font.
2020-
gfxFont* GetSmallCapsFont() const;
2002+
already_AddRefed<gfxFont> GetSmallCapsFont() const;
20212003
20222004
// subclasses may provide (possibly hinted) glyph widths (in font units);
20232005
// if they do not override this, harfbuzz will use unhinted widths
@@ -2239,7 +2221,9 @@ class gfxFont {
22392221
// This is OK because we only multiply by this factor, never divide.
22402222
float mFUnitsConvFactor;
22412223
2242-
nsExpirationState mExpirationState GUARDED_BY(mLock);
2224+
// This is guarded by gfxFontCache::GetCache()->GetMutex() but it is difficult
2225+
// to annotate that fact.
2226+
nsExpirationState mExpirationState;
22432227
22442228
// Glyph ID of the font's <space> glyph, zero if missing
22452229
uint16_t mSpaceGlyph = 0;

gfx/thebes/gfxFontEntry.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -266,9 +266,9 @@ nsCString gfxFontEntry::RealFaceName() {
266266
return Name();
267267
}
268268

269-
gfxFont* gfxFontEntry::FindOrMakeFont(const gfxFontStyle* aStyle,
270-
gfxCharacterMap* aUnicodeRangeMap) {
271-
gfxFont* font =
269+
already_AddRefed<gfxFont> gfxFontEntry::FindOrMakeFont(
270+
const gfxFontStyle* aStyle, gfxCharacterMap* aUnicodeRangeMap) {
271+
RefPtr<gfxFont> font =
272272
gfxFontCache::GetCache()->Lookup(this, aStyle, aUnicodeRangeMap);
273273

274274
if (!font) {
@@ -284,7 +284,7 @@ gfxFont* gfxFontEntry::FindOrMakeFont(const gfxFontStyle* aStyle,
284284
font->SetUnicodeRangeMap(aUnicodeRangeMap);
285285
gfxFontCache::GetCache()->AddNew(font);
286286
}
287-
return font;
287+
return font.forget();
288288
}
289289

290290
uint16_t gfxFontEntry::UnitsPerEm() {

gfx/thebes/gfxFontEntry.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -329,8 +329,8 @@ class gfxFontEntry {
329329
// cached instance; but we also don't return already_AddRefed, because
330330
// the caller may only need to use the font temporarily and doesn't need
331331
// a strong reference.
332-
gfxFont* FindOrMakeFont(const gfxFontStyle* aStyle,
333-
gfxCharacterMap* aUnicodeRangeMap = nullptr);
332+
already_AddRefed<gfxFont> FindOrMakeFont(
333+
const gfxFontStyle* aStyle, gfxCharacterMap* aUnicodeRangeMap = nullptr);
334334

335335
// Get an existing font table cache entry in aBlob if it has been
336336
// registered, or return false if not. Callers must call

0 commit comments

Comments
 (0)