Memory and file descriptor leak in QFontCache

Make the cache also use the ref counts
Make everyone who decrements a ref count check for 0 and delete
Move all cache logic to the cache
Same idea as 36cb3f3 and b3dae68 in Qt 5 without the extra stuff

Task-number: QTBUG-38035
Change-Id: I27bea376f4ec0888463b4ec3ed1a6bef00d041f8
Reviewed-by: Konstantin Ritt <ritt.ks@gmail.com>
Reviewed-by: Eskil Abrahamsen Blomfeldt <eskil.abrahamsen-blomfeldt@digia.com>

6 files changed, 55 insertions, 84 deletions

diff --git a/src/gui/text/qfont.cpp b/src/gui/text/qfont.cpp
index 7e94c1e..fa9bb70 100644
--- a/src/gui/text/qfont.cpp
+++ b/src/gui/text/qfont.cpp
@@ -275,8 +275,8 @@ QFontPrivate::QFontPrivate(const QFontPrivate &other)
 
 QFontPrivate::~QFontPrivate()
 {
-    if (engineData)
-        engineData->ref.deref();
+    if (engineData && !engineData->ref.deref())
+        delete engineData;
     engineData = 0;
     if (scFont && scFont != this)
         scFont->ref.deref();
@@ -298,7 +298,8 @@ QFontEngine *QFontPrivate::engineForScript(int script) const
         script = QUnicodeTables::Common;
     if (engineData && engineData->fontCache != QFontCache::instance()) {
         // throw out engineData that came from a different thread
-        engineData->ref.deref();
+        if (!engineData->ref.deref())
+            delete engineData;
         engineData = 0;
     }
     if (!engineData || !QT_FONT_ENGINE_FROM_DATA(engineData, script))
@@ -417,13 +418,13 @@ QFontEngineData::~QFontEngineData()
 {
 #if !defined(Q_WS_MAC)
     for (int i = 0; i < QUnicodeTables::ScriptCount; ++i) {
-        if (engines[i])
-            engines[i]->ref.deref();
+        if (engines[i] && !engines[i]->ref.deref())
+            delete engines[i];
         engines[i] = 0;
     }
 #else
-    if (engine)
-        engine->ref.deref();
+    if (engine && !engine->ref.deref())
+        delete engine;
     engine = 0;
 #endif // Q_WS_X11 || Q_WS_WIN || Q_WS_MAC
 }
@@ -770,8 +771,8 @@ QFont::QFont(QFontPrivate *data)
 void QFont::detach()
 {
     if (d->ref == 1) {
-        if (d->engineData)
-            d->engineData->ref.deref();
+        if (d->engineData && !d->engineData->ref.deref())
+            delete d->engineData;
         d->engineData = 0;
         if (d->scFont && d->scFont != d.data())
             d->scFont->ref.deref();
@@ -2819,7 +2820,7 @@ QFontCache::~QFontCache()
         EngineDataCache::ConstIterator it = engineDataCache.constBegin(),
                                  end = engineDataCache.constEnd();
         while (it != end) {
-            if (it.value()->ref == 0)
+            if (it.value()->ref.deref() == 0)
                 delete it.value();
             else
                 FC_DEBUG("QFontCache::~QFontCache: engineData %p still has refcount %d",
@@ -2827,24 +2828,6 @@ QFontCache::~QFontCache()
             ++it;
         }
     }
-    EngineCache::ConstIterator it = engineCache.constBegin(),
-                         end = engineCache.constEnd();
-    while (it != end) {
-        if (--it.value().data->cache_count == 0) {
-            if (it.value().data->ref == 0) {
-                FC_DEBUG("QFontCache::~QFontCache: deleting engine %p key=(%d / %g %g %d %d %d)",
-                         it.value().data, it.key().script, it.key().def.pointSize,
-                         it.key().def.pixelSize, it.key().def.weight, it.key().def.style,
-                         it.key().def.fixedPitch);
-
-                delete it.value().data;
-            } else {
-                FC_DEBUG("QFontCache::~QFontCache: engine = %p still has refcount %d",
-                         it.value().data, int(it.value().data->ref));
-            }
-        }
-        ++it;
-    }
 }
 
 void QFontCache::clear()
@@ -2856,16 +2839,14 @@ void QFontCache::clear()
             QFontEngineData *data = it.value();
 #if !defined(Q_WS_MAC)
             for (int i = 0; i < QUnicodeTables::ScriptCount; ++i) {
-                if (data->engines[i]) {
-                    data->engines[i]->ref.deref();
-                    data->engines[i] = 0;
-                }
+                if (data->engines[i] && !data->engines[i]->ref.deref())
+                    delete data->engines[i];
+                data->engines[i] = 0;
             }
 #else
-            if (data->engine) {
-                data->engine->ref.deref();
-                data->engine = 0;
-            }
+            if (data->engine && !data->engine->ref.deref())
+                delete data->engine;
+            data->engine = 0;
 #endif
             ++it;
         }
@@ -2873,15 +2854,7 @@ void QFontCache::clear()
 
     for (EngineCache::Iterator it = engineCache.begin(), end = engineCache.end();
          it != end; ++it) {
-        if (it->data->ref == 0) {
-            delete it->data;
-            it->data = 0;
-        }
-    }
-
-    for (EngineCache::Iterator it = engineCache.begin(), end = engineCache.end();
-         it != end; ++it) {
-        if (it->data && it->data->ref == 0) {
+        if (it->data->ref.deref() == 0) {
             delete it->data;
             it->data = 0;
         }
@@ -2916,6 +2889,8 @@ void QFontCache::insertEngineData(const Key &key, QFontEngineData *engineData)
 {
     FC_DEBUG("QFontCache: inserting new engine data %p", engineData);
 
+    Q_ASSERT(!engineDataCache.contains(key));
+    engineData->ref.ref(); // the cache has a reference
     engineDataCache.insert(key, engineData);
     increaseCost(sizeof(QFontEngineData));
 }
@@ -2946,6 +2921,11 @@ void QFontCache::insertEngine(const Key &key, QFontEngine *engine)
     Engine data(engine);
     data.timestamp = ++current_timestamp;
 
+    QFontEngine *oldEngine = engineCache.value(key).data;
+    engine->ref.ref(); // the cache has a reference
+    if (oldEngine && !oldEngine->ref.deref())
+        delete oldEngine;
+
     engineCache.insert(key, data);
 
     // only increase the cost if this is the first time we insert the engine
@@ -3005,12 +2985,11 @@ void QFontCache::cleanupPrinterFonts()
                 continue;
             }
 
-            if(it.value()->ref != 0) {
-                for(int i = 0; i < QUnicodeTables::ScriptCount; ++i) {
-                    if(it.value()->engines[i]) {
-                        it.value()->engines[i]->ref.deref();
-                        it.value()->engines[i] = 0;
-                    }
+            if (it.value()->ref > 1) {
+                for (int i = 0; i < QUnicodeTables::ScriptCount; ++i) {
+                    if (it.value()->engines[i] && !it.value()->engines[i]->ref.deref())
+                        delete it.value()->engines[i];
+                    it.value()->engines[i] = 0;
                 }
                 ++it;
             } else {
@@ -3021,7 +3000,8 @@ void QFontCache::cleanupPrinterFonts()
 
                 FC_DEBUG("    %p", rem.value());
 
-                delete rem.value();
+                if (!rem.value()->ref.deref())
+                    delete rem.value();
                 engineDataCache.erase(rem);
             }
         }
@@ -3030,7 +3010,7 @@ void QFontCache::cleanupPrinterFonts()
     EngineCache::Iterator it = engineCache.begin(),
                          end = engineCache.end();
     while(it != end) {
-        if (it.value().data->ref != 0 || it.key().screen == 0) {
+        if (it.value().data->ref != 1 || it.key().screen == 0) {
             ++it;
             continue;
         }
@@ -3044,7 +3024,8 @@ void QFontCache::cleanupPrinterFonts()
             FC_DEBUG("    DELETE: last occurrence in cache");
 
             decreaseCost(it.value().data->cache_cost);
-            delete it.value().data;
+            if (!it.value().data->ref.deref())
+                delete it.value().data;
         }
 
         engineCache.erase(it++);
@@ -3093,7 +3074,7 @@ void QFontCache::timerEvent(QTimerEvent *)
 #  endif // Q_WS_X11 || Q_WS_WIN
 #endif // QFONTCACHE_DEBUG
 
-            if (it.value()->ref != 0)
+            if (it.value()->ref > 1)
                 in_use_cost += engine_data_cost;
         }
     }
@@ -3109,7 +3090,7 @@ void QFontCache::timerEvent(QTimerEvent *)
                      int(it.value().data->ref), it.value().data->cache_count,
                      it.value().data->cache_cost);
 
-            if (it.value().data->ref != 0)
+            if (it.value().data->ref > 1)
                 in_use_cost += it.value().data->cache_cost / it.value().data->cache_count;
         }
 
@@ -3159,7 +3140,7 @@ void QFontCache::timerEvent(QTimerEvent *)
         EngineDataCache::Iterator it = engineDataCache.begin(),
                                  end = engineDataCache.end();
         while (it != end) {
-            if (it.value()->ref != 0) {
+            if (it.value()->ref > 1) {
                 ++it;
                 continue;
             }
@@ -3187,7 +3168,7 @@ void QFontCache::timerEvent(QTimerEvent *)
         uint least_popular = ~0u;
 
         for (; it != end; ++it) {
-            if (it.value().data->ref != 0)
+            if (it.value().data->ref > 1)
                 continue;
 
             if (it.value().timestamp < oldest &&
@@ -3200,7 +3181,7 @@ void QFontCache::timerEvent(QTimerEvent *)
         FC_DEBUG("    oldest %u least popular %u", oldest, least_popular);
 
         for (it = engineCache.begin(); it != end; ++it) {
-            if (it.value().data->ref == 0 &&
+            if (it.value().data->ref == 1 &&
                  it.value().timestamp == oldest &&
                  it.value().hits == least_popular)
                 break;
@@ -3216,7 +3197,8 @@ void QFontCache::timerEvent(QTimerEvent *)
                 FC_DEBUG("    DELETE: last occurrence in cache");
 
                 decreaseCost(it.value().data->cache_cost);
-                delete it.value().data;
+                if (!it.value().data->ref.deref())
+                    delete it.value().data;
             } else {
                 /*
                   this particular font engine is in the cache multiple
diff --git a/src/gui/text/qfontengine.cpp b/src/gui/text/qfontengine.cpp
index 9de475c..bf108c4 100644
--- a/src/gui/text/qfontengine.cpp
+++ b/src/gui/text/qfontengine.cpp
@@ -1325,11 +1325,8 @@ QFontEngineMulti::~QFontEngineMulti()
 {
     for (int i = 0; i < engines.size(); ++i) {
         QFontEngine *fontEngine = engines.at(i);
-        if (fontEngine) {
-            fontEngine->ref.deref();
-            if (fontEngine->cache_count == 0 && fontEngine->ref == 0)
-                delete fontEngine;
-        }
+        if (fontEngine && !fontEngine->ref.deref())
+            delete fontEngine;
     }
 }
 
diff --git a/src/gui/text/qrawfont.cpp b/src/gui/text/qrawfont.cpp
index 2b7554a..cb2bcb3 100644
--- a/src/gui/text/qrawfont.cpp
+++ b/src/gui/text/qrawfont.cpp
@@ -682,8 +682,7 @@ void QRawFont::setPixelSize(qreal pixelSize)
     if (d->fontEngine != 0)
         d->fontEngine->ref.ref();
 
-    oldFontEngine->ref.deref();
-    if (oldFontEngine->cache_count == 0 && oldFontEngine->ref == 0)
+    if (!oldFontEngine->ref.deref())
         delete oldFontEngine;
 }
 
@@ -693,12 +692,10 @@ void QRawFont::setPixelSize(qreal pixelSize)
 void QRawFontPrivate::cleanUp()
 {
     platformCleanUp();
-    if (fontEngine != 0) {
-        fontEngine->ref.deref();
-        if (fontEngine->cache_count == 0 && fontEngine->ref == 0)
-            delete fontEngine;
-        fontEngine = 0;
-    }
+    if (fontEngine != 0 && !fontEngine->ref.deref())
+        delete fontEngine;
+    fontEngine = 0;
+
     hintingPreference = QFont::PreferDefaultHinting;
 }
 
diff --git a/src/gui/text/qrawfont_win.cpp b/src/gui/text/qrawfont_win.cpp
index 6923aae..9b66886 100644
--- a/src/gui/text/qrawfont_win.cpp
+++ b/src/gui/text/qrawfont_win.cpp
@@ -600,11 +600,11 @@ void QRawFontPrivate::platformLoadFromData(const QByteArray &fontData,
             if (request.family != fontEngine->fontDef.family) {
                 qWarning("QRawFont::platformLoadFromData: Failed to load font. "
                          "Got fallback instead: %s", qPrintable(fontEngine->fontDef.family));
-                if (fontEngine->cache_count == 0 && fontEngine->ref == 0)
+                if (fontEngine->ref == 0)
                     delete fontEngine;
                 fontEngine = 0;
             } else {
-                Q_ASSERT(fontEngine->cache_count == 0 && fontEngine->ref == 0);
+                Q_ASSERT(fontEngine->ref == 0);
 
                 // Override the generated font name
                 static_cast<QFontEngineWin *>(fontEngine)->uniqueFamilyName = uniqueFamilyName;
diff --git a/src/gui/text/qstatictext.cpp b/src/gui/text/qstatictext.cpp
index 657da33..b111200 100644
--- a/src/gui/text/qstatictext.cpp
+++ b/src/gui/text/qstatictext.cpp
@@ -724,10 +724,8 @@ QStaticTextItem::~QStaticTextItem()
 
 void QStaticTextItem::setFontEngine(QFontEngine *fe)
 {
-    if (m_fontEngine != 0) {
-        if (!m_fontEngine->ref.deref())
-            delete m_fontEngine;
-    }
+    if (m_fontEngine != 0 && !m_fontEngine->ref.deref())
+        delete m_fontEngine;
 
     m_fontEngine = fe;
     if (m_fontEngine != 0)
diff --git a/src/gui/text/qtextengine.cpp b/src/gui/text/qtextengine.cpp
index b371237..f4b86b0 100644
--- a/src/gui/text/qtextengine.cpp
+++ b/src/gui/text/qtextengine.cpp
@@ -1453,11 +1453,8 @@ void QTextEngine::shape(int item) const
 
 static inline void releaseCachedFontEngine(QFontEngine *fontEngine)
 {
-    if (fontEngine) {
-        fontEngine->ref.deref();
-        if (fontEngine->cache_count == 0 && fontEngine->ref == 0)
-            delete fontEngine;
-    }
+    if (fontEngine && !fontEngine->ref.deref())
+        delete fontEngine;
 }
 
 void QTextEngine::resetFontEngineCache()