From 45693cc638d10890f2816a38d38de6ddaf04ffd3 Mon Sep 17 00:00:00 2001 From: Simon Yuan Date: Wed, 2 Apr 2014 16:02:04 +1300 Subject: 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 Reviewed-by: Eskil Abrahamsen Blomfeldt --- src/gui/text/qfont.cpp | 102 +++++++++++++++++------------------------- src/gui/text/qfontengine.cpp | 7 +-- src/gui/text/qrawfont.cpp | 13 +++--- src/gui/text/qrawfont_win.cpp | 4 +- src/gui/text/qstatictext.cpp | 6 +-- src/gui/text/qtextengine.cpp | 7 +-- 6 files changed, 55 insertions(+), 84 deletions(-) (limited to 'src/gui/text') diff --git a/src/gui/text/qfont.cpp b/src/gui/text/qfont.cpp index 7e94c1ea74..fa9bb709c9 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 9de475cec6..bf108c4dff 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 2b7554a745..cb2bcb3eae 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 6923aaeb82..9b66886774 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(fontEngine)->uniqueFamilyName = uniqueFamilyName; diff --git a/src/gui/text/qstatictext.cpp b/src/gui/text/qstatictext.cpp index 657da33f25..b11120096e 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 b371237966..f4b86b01ca 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() -- cgit v1.2.3