Don't re-enter mutex in SkFontMgr_fontconfig.

The FCLocker lock cannot be held while calling fTFCache::add. The
~SkTypeface_fontconfig method must take the FCLocker lock to destroy its
FcPattern since a typeface may be destroyed by any last owner. However,
fTFCache may be the last owner of some of its cached typefaces, and so a
purge when calling ::add to make space for the new entry may cause a
typeface to be destroyed. As a result, createTypefaceFromFcPattern must
not hold the FCLocker lock when calling fTFCache::add.

Fortunately, the FCLocker lock is only needed to serialize calls into
FontConfig. If acquire and release were free then they would be used
around each individual call to FontConfig. As a result it is fine to
give up the lock at any point in Skia code so long as no FontConfig
calls are made while the FCLocker lock is not held.

Change-Id: I37224d4b38bf88ace482496ce7530c84158d2d2e
Reviewed-on: https://skia-review.googlesource.com/27663
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Ben Wagner <bungeman@google.com>

1 files changed, 19 insertions, 5 deletions

diff --git a/src/ports/SkFontMgr_fontconfig.cpp b/src/ports/SkFontMgr_fontconfig.cpp
index be4b17dd12..95c963d69f 100644
--- a/src/ports/SkFontMgr_fontconfig.cpp
+++ b/src/ports/SkFontMgr_fontconfig.cpp
@@ -73,10 +73,9 @@ void DeleteThreadFcLocked(void* v) { delete static_cast<bool*>(v); }
         static_cast<bool*>(SkTLS::Get(CreateThreadFcLocked, DeleteThreadFcLocked))
 #endif
 
-struct FCLocker {
+class FCLocker {
     // Assume FcGetVersion() has always been thread safe.
-
-    FCLocker() {
+    static void lock() {
         if (FcGetVersion() < 21091) {
             gFCMutex.acquire();
         } else {
@@ -85,8 +84,7 @@ struct FCLocker {
             SkDEBUGCODE(*threadLocked = true);
         }
     }
-
-    ~FCLocker() {
+    static void unlock() {
         AssertHeld();
         if (FcGetVersion() < 21091) {
             gFCMutex.release();
@@ -95,6 +93,20 @@ struct FCLocker {
         }
     }
 
+public:
+    FCLocker() { lock(); }
+    ~FCLocker() { unlock(); }
+
+    /** If acquire and release were free, FCLocker would be used around each call into FontConfig.
+     *  Instead a much more granular approach is taken, but this means there are times when the
+     *  mutex is held when it should not be. A Suspend will drop the lock until it is destroyed.
+     *  While a Suspend exists, FontConfig should not be used without re-taking the lock.
+     */
+    struct Suspend {
+        Suspend() { unlock(); }
+        ~Suspend() { lock(); }
+    };
+
     static void AssertHeld() { SkDEBUGCODE(
         if (FcGetVersion() < 21091) {
             gFCMutex.assertHeld();
@@ -652,6 +664,8 @@ class SkFontMgr_fontconfig : public SkFontMgr {
             FcPatternReference(pattern);
             face = SkTypeface_fontconfig::Create(pattern);
             if (face) {
+                // Cannot hold the lock when calling add; an evicted typeface may need to lock.
+                FCLocker::Suspend suspend;
                 fTFCache.add(face);
             }
         }