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

Commit c634fc4

Browse files
Ben WagnerSkia Commit-Bot
Ben Wagner
authored and
Skia Commit-Bot
committed
Remove FCLocker::Suspend.
It's a bad idea, difficult to reason about, and may be causing deadlocks. Change-Id: Id9749661f4f3f942ee983e9c1fdab2bd7f287edb Reviewed-on: https://skia-review.googlesource.com/c/skia/+/335242 Reviewed-by: Herb Derby <herb@google.com> Commit-Queue: Ben Wagner <bungeman@google.com>
1 parent 5dfa02b commit c634fc4

File tree

2 files changed

+113
-101
lines changed

2 files changed

+113
-101
lines changed

‎include/private/SkTemplates.h‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -66,9 +66,11 @@ template <typename T, T* P> struct SkFunctionWrapper {
6666
*/
6767
template <typename T, void (*P)(T*)> class SkAutoTCallVProc
6868
: public std::unique_ptr<T, SkFunctionWrapper<std::remove_pointer_t<decltype(P)>, P>> {
69+
using inherited = std::unique_ptr<T, SkFunctionWrapper<std::remove_pointer_t<decltype(P)>, P>>;
6970
public:
70-
SkAutoTCallVProc(T* obj)
71-
: std::unique_ptr<T, SkFunctionWrapper<std::remove_pointer_t<decltype(P)>, P>>(obj) {}
71+
using inherited::inherited;
72+
SkAutoTCallVProc(const SkAutoTCallVProc&) = delete;
73+
SkAutoTCallVProc(SkAutoTCallVProc&& that) : inherited(std::move(that)) {}
7274

7375
operator T*() const { return this->get(); }
7476
};

‎src/ports/SkFontMgr_fontconfig.cpp‎

Lines changed: 109 additions & 99 deletions
Original file line numberDiff line numberDiff line change
@@ -85,16 +85,6 @@ class FCLocker {
8585
FCLocker() { lock(); }
8686
~FCLocker() { unlock(); }
8787

88-
/** If acquire and release were free, FCLocker would be used around each call into FontConfig.
89-
* Instead a much more granular approach is taken, but this means there are times when the
90-
* mutex is held when it should not be. A Suspend will drop the lock until it is destroyed.
91-
* While a Suspend exists, FontConfig should not be used without re-taking the lock.
92-
*/
93-
struct Suspend {
94-
Suspend() { unlock(); }
95-
~Suspend() { lock(); }
96-
};
97-
9888
static void AssertHeld() { SkDEBUGCODE(
9989
if (FcGetVersion() < FontConfigThreadSafeVersion) {
10090
f_c_mutex().assertHeld();
@@ -109,13 +99,16 @@ template<typename T, void (*D)(T*)> void FcTDestroy(T* t) {
10999
D(t);
110100
}
111101
template <typename T, T* (*C)(), void (*D)(T*)> class SkAutoFc
112-
: public SkAutoTCallVProc<T, FcTDestroy<T, D> > {
102+
: public SkAutoTCallVProc<T, FcTDestroy<T, D>> {
103+
using inherited = SkAutoTCallVProc<T, FcTDestroy<T, D>>;
113104
public:
114-
SkAutoFc() : SkAutoTCallVProc<T, FcTDestroy<T, D> >(C()) {
105+
SkAutoFc() : SkAutoTCallVProc<T, FcTDestroy<T, D>>( C() ) {
115106
T* obj = this->operator T*();
116107
SkASSERT_RELEASE(nullptr != obj);
117108
}
118-
explicit SkAutoFc(T* obj) : SkAutoTCallVProc<T, FcTDestroy<T, D> >(obj) {}
109+
explicit SkAutoFc(T* obj) : inherited(obj) {}
110+
SkAutoFc(const SkAutoFc&) = delete;
111+
SkAutoFc(SkAutoFc&& that) : inherited(std::move(that)) {}
119112
};
120113

121114
typedef SkAutoFc<FcCharSet, FcCharSetCreate, FcCharSetDestroy> SkAutoFcCharSet;
@@ -603,30 +596,34 @@ class SkFontMgr_fontconfig : public SkFontMgr {
603596
}
604597

605598
SkTypeface* createTypeface(int index) override {
606-
FCLocker lock;
607-
608-
FcPattern* match = fFontSet->fonts[index];
609-
return fFontMgr->createTypefaceFromFcPattern(match).release();
610-
}
611-
612-
SkTypeface* matchStyle(const SkFontStyle& style) override {
613-
FCLocker lock;
614-
615-
SkAutoFcPattern pattern;
616-
fcpattern_from_skfontstyle(style, pattern);
617-
FcConfigSubstitute(fFontMgr->fFC, pattern, FcMatchPattern);
618-
FcDefaultSubstitute(pattern);
619-
620-
FcResult result;
621-
FcFontSet* fontSets[1] = { fFontSet };
622-
SkAutoFcPattern match(FcFontSetMatch(fFontMgr->fFC,
623-
fontSets, SK_ARRAY_COUNT(fontSets),
624-
pattern, &result));
625-
if (nullptr == match) {
599+
if (index < 0 || fFontSet->nfont <= index) {
626600
return nullptr;
627601
}
602+
SkAutoFcPattern match([this, &index]() {
603+
FCLocker lock;
604+
FcPatternReference(fFontSet->fonts[index]);
605+
return fFontSet->fonts[index];
606+
}());
607+
return fFontMgr->createTypefaceFromFcPattern(std::move(match)).release();
608+
}
628609

629-
return fFontMgr->createTypefaceFromFcPattern(match).release();
610+
SkTypeface* matchStyle(const SkFontStyle& style) override {
611+
SkAutoFcPattern match([this, &style]() {
612+
FCLocker lock;
613+
614+
SkAutoFcPattern pattern;
615+
fcpattern_from_skfontstyle(style, pattern);
616+
FcConfigSubstitute(fFontMgr->fFC, pattern, FcMatchPattern);
617+
FcDefaultSubstitute(pattern);
618+
619+
FcResult result;
620+
FcFontSet* fontSets[1] = { fFontSet };
621+
return FcFontSetMatch(fFontMgr->fFC,
622+
fontSets, SK_ARRAY_COUNT(fontSets),
623+
pattern, &result);
624+
625+
}());
626+
return fFontMgr->createTypefaceFromFcPattern(std::move(match)).release();
630627
}
631628

632629
private:
@@ -693,16 +690,25 @@ class SkFontMgr_fontconfig : public SkFontMgr {
693690
/** Creates a typeface using a typeface cache.
694691
* @param pattern a complete pattern from FcFontRenderPrepare.
695692
*/
696-
sk_sp<SkTypeface> createTypefaceFromFcPattern(FcPattern* pattern) const {
697-
FCLocker::AssertHeld();
693+
sk_sp<SkTypeface> createTypefaceFromFcPattern(SkAutoFcPattern pattern) const {
694+
if (!pattern) {
695+
return nullptr;
696+
}
697+
// Cannot hold FCLocker when calling fTFCache.add; an evicted typeface may need to lock.
698+
// Must hold fTFCacheMutex when interacting with fTFCache.
698699
SkAutoMutexExclusive ama(fTFCacheMutex);
699-
sk_sp<SkTypeface> face = fTFCache.findByProcAndRef(FindByFcPattern, pattern);
700+
sk_sp<SkTypeface> face = [&]() {
701+
FCLocker lock;
702+
sk_sp<SkTypeface> face = fTFCache.findByProcAndRef(FindByFcPattern, pattern);
703+
if (face) {
704+
pattern.reset();
705+
}
706+
return face;
707+
}();
700708
if (!face) {
701-
FcPatternReference(pattern);
702-
face = SkTypeface_fontconfig::Make(SkAutoFcPattern(pattern), fSysroot);
709+
face = SkTypeface_fontconfig::Make(std::move(pattern), fSysroot);
703710
if (face) {
704-
// Cannot hold the lock when calling add; an evicted typeface may need to lock.
705-
FCLocker::Suspend suspend;
711+
// Cannot hold FCLocker in fTFCache.add; evicted typefaces may need to lock.
706712
fTFCache.add(face);
707713
}
708714
}
@@ -865,39 +871,41 @@ class SkFontMgr_fontconfig : public SkFontMgr {
865871
SkTypeface* onMatchFamilyStyle(const char familyName[],
866872
const SkFontStyle& style) const override
867873
{
868-
FCLocker lock;
869-
870-
SkAutoFcPattern pattern;
871-
FcPatternAddString(pattern, FC_FAMILY, (FcChar8*)familyName);
872-
fcpattern_from_skfontstyle(style, pattern);
873-
FcConfigSubstitute(fFC, pattern, FcMatchPattern);
874-
FcDefaultSubstitute(pattern);
874+
SkAutoFcPattern font([this, &familyName, &style]() {
875+
FCLocker lock;
875876

876-
// We really want to match strong (prefered) and same (acceptable) only here.
877-
// If a family name was specified, assume that any weak matches after the last strong match
878-
// are weak (default) and ignore them.
879-
// The reason for is that after substitution the pattern for 'sans-serif' looks like
880-
// "wwwwwwwwwwwwwwswww" where there are many weak but preferred names, followed by defaults.
881-
// So it is possible to have weakly matching but preferred names.
882-
// In aliases, bindings are weak by default, so this is easy and common.
883-
// If no family name was specified, we'll probably only get weak matches, but that's ok.
884-
FcPattern* matchPattern;
885-
SkAutoFcPattern strongPattern(nullptr);
886-
if (familyName) {
887-
strongPattern.reset(FcPatternDuplicate(pattern));
888-
remove_weak(strongPattern, FC_FAMILY);
889-
matchPattern = strongPattern;
890-
} else {
891-
matchPattern = pattern;
892-
}
877+
SkAutoFcPattern pattern;
878+
FcPatternAddString(pattern, FC_FAMILY, (FcChar8*)familyName);
879+
fcpattern_from_skfontstyle(style, pattern);
880+
FcConfigSubstitute(fFC, pattern, FcMatchPattern);
881+
FcDefaultSubstitute(pattern);
893882

894-
FcResult result;
895-
SkAutoFcPattern font(FcFontMatch(fFC, pattern, &result));
896-
if (nullptr == font || !FontAccessible(font) || !FontFamilyNameMatches(font, matchPattern)) {
897-
return nullptr;
898-
}
883+
// We really want to match strong (preferred) and same (acceptable) only here.
884+
// If a family name was specified, assume that any weak matches after the last strong
885+
// match are weak (default) and ignore them.
886+
// After substitution the pattern for 'sans-serif' looks like "wwwwwwwwwwwwwwswww" where
887+
// there are many weak but preferred names, followed by defaults.
888+
// So it is possible to have weakly matching but preferred names.
889+
// In aliases, bindings are weak by default, so this is easy and common.
890+
// If no family name was specified, we'll probably only get weak matches, but that's ok.
891+
FcPattern* matchPattern;
892+
SkAutoFcPattern strongPattern(nullptr);
893+
if (familyName) {
894+
strongPattern.reset(FcPatternDuplicate(pattern));
895+
remove_weak(strongPattern, FC_FAMILY);
896+
matchPattern = strongPattern;
897+
} else {
898+
matchPattern = pattern;
899+
}
899900

900-
return createTypefaceFromFcPattern(font).release();
901+
FcResult result;
902+
SkAutoFcPattern font(FcFontMatch(fFC, pattern, &result));
903+
if (!font || !FontAccessible(font) || !FontFamilyNameMatches(font, matchPattern)) {
904+
font.reset();
905+
}
906+
return font;
907+
}());
908+
return createTypefaceFromFcPattern(std::move(font)).release();
901909
}
902910

903911
SkTypeface* onMatchFamilyStyleCharacter(const char familyName[],
@@ -906,40 +914,42 @@ class SkFontMgr_fontconfig : public SkFontMgr {
906914
int bcp47Count,
907915
SkUnichar character) const override
908916
{
909-
FCLocker lock;
917+
SkAutoFcPattern font([&](){
918+
FCLocker lock;
910919

911-
SkAutoFcPattern pattern;
912-
if (familyName) {
913-
FcValue familyNameValue;
914-
familyNameValue.type = FcTypeString;
915-
familyNameValue.u.s = reinterpret_cast<const FcChar8*>(familyName);
916-
FcPatternAddWeak(pattern, FC_FAMILY, familyNameValue, FcFalse);
917-
}
918-
fcpattern_from_skfontstyle(style, pattern);
920+
SkAutoFcPattern pattern;
921+
if (familyName) {
922+
FcValue familyNameValue;
923+
familyNameValue.type = FcTypeString;
924+
familyNameValue.u.s = reinterpret_cast<const FcChar8*>(familyName);
925+
FcPatternAddWeak(pattern, FC_FAMILY, familyNameValue, FcFalse);
926+
}
927+
fcpattern_from_skfontstyle(style, pattern);
919928

920-
SkAutoFcCharSet charSet;
921-
FcCharSetAddChar(charSet, character);
922-
FcPatternAddCharSet(pattern, FC_CHARSET, charSet);
929+
SkAutoFcCharSet charSet;
930+
FcCharSetAddChar(charSet, character);
931+
FcPatternAddCharSet(pattern, FC_CHARSET, charSet);
923932

924-
if (bcp47Count > 0) {
925-
SkASSERT(bcp47);
926-
SkAutoFcLangSet langSet;
927-
for (int i = bcp47Count; i --> 0;) {
928-
FcLangSetAdd(langSet, (const FcChar8*)bcp47[i]);
933+
if (bcp47Count > 0) {
934+
SkASSERT(bcp47);
935+
SkAutoFcLangSet langSet;
936+
for (int i = bcp47Count; i --> 0;) {
937+
FcLangSetAdd(langSet, (const FcChar8*)bcp47[i]);
938+
}
939+
FcPatternAddLangSet(pattern, FC_LANG, langSet);
929940
}
930-
FcPatternAddLangSet(pattern, FC_LANG, langSet);
931-
}
932-
933-
FcConfigSubstitute(fFC, pattern, FcMatchPattern);
934-
FcDefaultSubstitute(pattern);
935941

936-
FcResult result;
937-
SkAutoFcPattern font(FcFontMatch(fFC, pattern, &result));
938-
if (nullptr == font || !FontAccessible(font) || !FontContainsCharacter(font, character)) {
939-
return nullptr;
940-
}
942+
FcConfigSubstitute(fFC, pattern, FcMatchPattern);
943+
FcDefaultSubstitute(pattern);
941944

942-
return createTypefaceFromFcPattern(font).release();
945+
FcResult result;
946+
SkAutoFcPattern font(FcFontMatch(fFC, pattern, &result));
947+
if (!font || !FontAccessible(font) || !FontContainsCharacter(font, character)) {
948+
font.reset();
949+
}
950+
return font;
951+
}());
952+
return createTypefaceFromFcPattern(std::move(font)).release();
943953
}
944954

945955
SkTypeface* onMatchFaceStyle(const SkTypeface* typeface,

0 commit comments

Comments
 (0)