Fix race condition when destroying Freetype font engines

If a QFont is moved to a different thread (B) than where it belongs (A),
and the font cache is then purged on thread A, the last remaining
reference to the engine will be on thread B. When this QFontEngine is
later replaced with one created on B, we end up deleting A's QFontEngine
on thread B.

This caused QFreetypeFace::release() to be called on the wrong thread and
the face would never be removed from the thread-local Freetype cache in
A. Hence that cache would contain a dangling pointer to the freetype face
which we would later end up fetching.

To avoid this, we make sure the thread-local cache itself increases
the ref count of the face. That way, the only time it will be deleted
on a different thread is when the cache has been destroyed because the
thread has shut down.

If the last reference (except the cache reference) to a face is cleared
on a different thread, we keep it in the cache until later. It will
then be in a "deferred delete" mode and will be deleted as soon as
possible. This is done either when the thread shuts down, when a lookup
causes the "deferred delete" face to be returned, or when release()
is called on any font on the thread (at which point we will purge all
faces that only have the single cache reference.)

Pick-to: 6.7
Fixes: QTBUG-118867
Change-Id: Ifa07a9cb6f4cd3e783e12a73d8b283e70d6fb474
Reviewed-by: Mårten Nordheim <marten.nordheim@qt.io>

1 files changed, 53 insertions, 19 deletions

diff --git a/src/gui/text/freetype/qfontengine_ft.cpp b/src/gui/text/freetype/qfontengine_ft.cpp
index 44027c23a9..66a46d18cd 100644
--- a/src/gui/text/freetype/qfontengine_ft.cpp
+++ b/src/gui/text/freetype/qfontengine_ft.cpp
@@ -115,8 +115,11 @@ public:
 
 QtFreetypeData::~QtFreetypeData()
 {
-    for (QHash<QFontEngine::FaceId, QFreetypeFace *>::ConstIterator iter = faces.cbegin(); iter != faces.cend(); ++iter)
+    for (auto iter = faces.cbegin(); iter != faces.cend(); ++iter) {
         iter.value()->cleanup();
+        if (!iter.value()->ref.deref())
+            delete iter.value();
+    }
     faces.clear();
     FT_Done_FreeType(library);
     library = nullptr;
@@ -213,10 +216,28 @@ QFreetypeFace *QFreetypeFace::getFace(const QFontEngine::FaceId &face_id,
 
     QtFreetypeData *freetypeData = qt_getFreetypeData();
 
-    QFreetypeFace *freetype = freetypeData->faces.value(face_id, nullptr);
-    if (freetype) {
-        freetype->ref.ref();
-    } else {
+    QFreetypeFace *freetype = nullptr;
+    auto it = freetypeData->faces.find(face_id);
+    if (it != freetypeData->faces.end()) {
+        freetype = *it;
+
+        Q_ASSERT(freetype->ref.loadRelaxed() > 0);
+        if (freetype->ref.loadRelaxed() == 1) {
+            // If there is only one reference left to the face, it means it is only referenced by
+            // the cache itself, and thus it is in cleanup state (but the final outside reference
+            // was removed on a different thread so it could not be deleted right away). We then
+            // complete the cleanup and pretend we didn't find it, so that it can be re-created with
+            // the present state.
+            freetype->cleanup();
+            freetypeData->faces.erase(it);
+            delete freetype;
+            freetype = nullptr;
+        } else {
+            freetype->ref.ref();
+        }
+    }
+
+    if (!freetype) {
         const auto deleter = [](QFreetypeFace *f) { delete f; };
         std::unique_ptr<QFreetypeFace, decltype(deleter)> newFreetype(new QFreetypeFace, deleter);
         FT_Face face;
@@ -322,6 +343,7 @@ QFreetypeFace *QFreetypeFace::getFace(const QFontEngine::FaceId &face_id,
             QT_RETHROW;
         }
         freetype = newFreetype.release();
+        freetype->ref.ref();
     }
     return freetype;
 }
@@ -335,24 +357,36 @@ void QFreetypeFace::cleanup()
 
 void QFreetypeFace::release(const QFontEngine::FaceId &face_id)
 {
-    if (!ref.deref()) {
-        if (face) {
-            QtFreetypeData *freetypeData = qt_getFreetypeData();
-
-            cleanup();
-
-            auto it = freetypeData->faces.constFind(face_id);
-            if (it != freetypeData->faces.constEnd())
-                freetypeData->faces.erase(it);
-
-            if (freetypeData->faces.isEmpty()) {
-                FT_Done_FreeType(freetypeData->library);
-                freetypeData->library = nullptr;
+    Q_UNUSED(face_id);
+    bool deleteThis = !ref.deref();
+
+    // If the only reference left over is the cache's reference, we remove it from the cache,
+    // granted that we are on the correct thread. If not, we leave it there to be cleaned out
+    // later. While we are at it, we also purge all left over faces which are only referenced
+    // from the cache.
+    if (face && ref.loadRelaxed() == 1) {
+        QtFreetypeData *freetypeData = qt_getFreetypeData();
+        for (auto it = freetypeData->faces.constBegin(); it != freetypeData->faces.constEnd(); ) {
+            if (it.value()->ref.loadRelaxed() == 1) {
+                it.value()->cleanup();
+                if (it.value() == this)
+                    deleteThis = true; // This face, delete at end of function for safety
+                else
+                    delete it.value();
+                it = freetypeData->faces.erase(it);
+            } else {
+                ++it;
             }
         }
 
-        delete this;
+        if (freetypeData->faces.isEmpty()) {
+            FT_Done_FreeType(freetypeData->library);
+            freetypeData->library = nullptr;
+        }
     }
+
+    if (deleteThis)
+        delete this;
 }
 
 static int computeFaceIndex(const QString &faceFileName, const QString &styleName)