Cleanup freetype data in a thread-safe way

One less obvious part of this patch: the fontCache pointer in engineData was not
safe. It isn't safe to rely on pointer addresses to verify we're cleaning up the
right thing, as a sequence of malloc()/free()/malloc() can return the same
pointer, and nothing was cleaning up the dangling pointer in engineData.

With this, it is possible to safely drop OpenGL contexts in QtQuick under all
conditions with no possibility of crashes.

Done-with: Aaron Kennedy <aaron.kennedy@jollamobile.com>
Change-Id: I7b91384251593730124323a74737d41333a05f59
Reviewed-by: Konstantin Ritt <ritt.ks@gmail.com>
Reviewed-by: Eskil Abrahamsen Blomfeldt <eskil.abrahamsen-blomfeldt@digia.com>

4 files changed, 45 insertions, 15 deletions

diff --git a/src/gui/text/qfont.cpp b/src/gui/text/qfont.cpp
index a410004c06..49b5a9ba46 100644
--- a/src/gui/text/qfont.cpp
+++ b/src/gui/text/qfont.cpp
@@ -208,7 +208,7 @@ QFontEngine *QFontPrivate::engineForScript(int script) const
     QMutexLocker locker(qt_fontdatabase_mutex());
     if (script <= QChar::Script_Latin)
         script = QChar::Script_Common;
-    if (engineData && engineData->fontCache != QFontCache::instance()) {
+    if (engineData && engineData->fontCacheId != QFontCache::instance()->id()) {
         // throw out engineData that came from a different thread
         if (!engineData->ref.deref())
             delete engineData;
@@ -317,7 +317,7 @@ void QFontPrivate::resolve(uint mask, const QFontPrivate *other)
 
 
 QFontEngineData::QFontEngineData()
-    : ref(0), fontCache(QFontCache::instance())
+    : ref(0), fontCacheId(QFontCache::instance()->id())
 {
     memset(engines, 0, QChar::ScriptCount * sizeof(QFontEngine *));
 }
@@ -2638,9 +2638,12 @@ void QFontCache::cleanup()
 }
 #endif // QT_NO_THREAD
 
+QBasicAtomicInt font_cache_id = Q_BASIC_ATOMIC_INITIALIZER(1);
+
 QFontCache::QFontCache()
     : QObject(), total_cost(0), max_cost(min_cost),
-      current_timestamp(0), fast(false), timer_id(-1)
+      current_timestamp(0), fast(false), timer_id(-1),
+      m_id(font_cache_id.fetchAndAddRelaxed(1))
 {
 }
 
diff --git a/src/gui/text/qfont_p.h b/src/gui/text/qfont_p.h
index 115e866f24..5b7f918e21 100644
--- a/src/gui/text/qfont_p.h
+++ b/src/gui/text/qfont_p.h
@@ -140,7 +140,7 @@ public:
     ~QFontEngineData();
 
     QAtomicInt ref;
-    QFontCache *fontCache;
+    const int fontCacheId;
 
     QFontEngine *engines[QChar::ScriptCount];
 
@@ -206,6 +206,8 @@ public:
     QFontCache();
     ~QFontCache();
 
+    int id() const { return m_id; }
+
     void clear();
 
     struct Key {
@@ -263,6 +265,7 @@ private:
     uint current_timestamp;
     bool fast;
     int timer_id;
+    const int m_id;
 };
 
 Q_GUI_EXPORT int qt_defaultDpiX();
diff --git a/src/gui/text/qfontengine_ft.cpp b/src/gui/text/qfontengine_ft.cpp
index 5f53a36629..6af97145d6 100644
--- a/src/gui/text/qfontengine_ft.cpp
+++ b/src/gui/text/qfontengine_ft.cpp
@@ -124,11 +124,21 @@ public:
     QtFreetypeData()
         : library(0)
     { }
+    ~QtFreetypeData();
 
     FT_Library library;
     QHash<QFontEngine::FaceId, QFreetypeFace *> faces;
 };
 
+QtFreetypeData::~QtFreetypeData()
+{
+    for (QHash<QFontEngine::FaceId, QFreetypeFace *>::ConstIterator iter = faces.begin(); iter != faces.end(); ++iter)
+        iter.value()->cleanup();
+    faces.clear();
+    FT_Done_FreeType(library);
+    library = 0;
+}
+
 #ifdef QT_NO_THREAD
 Q_GLOBAL_STATIC(QtFreetypeData, theFreetypeData)
 
@@ -292,23 +302,35 @@ QFreetypeFace *QFreetypeFace::getFace(const QFontEngine::FaceId &face_id,
     return freetype;
 }
 
+void QFreetypeFace::cleanup()
+{
+    if (hbFace && hbFace_destroy_func) {
+        hbFace_destroy_func(hbFace);
+        hbFace = 0;
+    }
+    FT_Done_Face(face);
+    face = 0;
+}
+
 void QFreetypeFace::release(const QFontEngine::FaceId &face_id)
 {
-    QtFreetypeData *freetypeData = qt_getFreetypeData();
     if (!ref.deref()) {
-        if (hbFace && hbFace_destroy_func) {
-            hbFace_destroy_func(hbFace);
-            hbFace = 0;
+        if (face) {
+            QtFreetypeData *freetypeData = qt_getFreetypeData();
+
+            cleanup();
+
+            if (freetypeData->faces.contains(face_id))
+                freetypeData->faces.take(face_id);
+
+            if (freetypeData->faces.isEmpty()) {
+                FT_Done_FreeType(freetypeData->library);
+                freetypeData->library = 0;
+            }
         }
-        FT_Done_Face(face);
-        if(freetypeData->faces.contains(face_id))
-            freetypeData->faces.take(face_id);
+
         delete this;
     }
-    if (freetypeData->faces.isEmpty()) {
-        FT_Done_FreeType(freetypeData->library);
-        freetypeData->library = 0;
-    }
 }
 
 
diff --git a/src/gui/text/qfontengine_ft_p.h b/src/gui/text/qfontengine_ft_p.h
index e64fec2f27..7df66b9678 100644
--- a/src/gui/text/qfontengine_ft_p.h
+++ b/src/gui/text/qfontengine_ft_p.h
@@ -114,9 +114,11 @@ public:
 
 private:
     friend class QFontEngineFT;
+    friend class QtFreetypeData;
     friend struct QScopedPointerDeleter<QFreetypeFace>;
     QFreetypeFace() : _lock(QMutex::Recursive) {}
     ~QFreetypeFace() {}
+    void cleanup();
     QAtomicInt ref;
     QMutex _lock;
     QByteArray fontData;