From 53f88aa73719ceab0e263a3ed7e2c6071e4c688b Mon Sep 17 00:00:00 2001 From: Marc Mutz Date: Mon, 12 Jun 2023 14:38:28 +0200 Subject: [PATCH] QPixmapCache: don't leak QString keys of evicted pixmap It's not a real leak in that the string data is being freed on program exit (or, more recently, QPixmapCache::clear()), but it can lead to lots of memory being bound for much longer than expected when users put in new QString keys without attempting to retrive them again. It can also lead to problems with QStringLiterals lingering around until after their underlying data has been freed. A bug in the Fusion style, generating new string keys for identical state, exposed this misbehavior, and one way to fix the resulting issue for the user is to make sure that QPixmapCache doesn't leak QString keys. The Fusion style issue with generating non-repeating keys for use with QPixmapCache should also be fixed, eventually, but this patch relegates that to an optimization issue (the caching is effectively non-existent), the resource exhaustion is gone now. The issue exists because the QString keys are internally mapped to QPixmapCache::Key's by way of a QHash cacheKeys data structure. When the QCache, indexed by Key, not QString, decides to evict an entry, the Key is invalidated, but no-one was removing the corresponding entry from cacheKeys. So make the existing releaseKey(), used to invalidate copies of Keys referring to evicted pixmaps, do that, now. So as not to have to scan the whole cacheKeys QHash for the right Key, store the QString key, if any, inside the Key, so releaseKey() can retrieve it and use it for O(1) erasure from cacheKey. This allows removing the previous work-around in clear() (6ab0d25a09f5aeb7a5a062f7fd44e95ca761e21e), greatly simplify object(QString), and requires to rewrite all code that holds iterators or references into cacheKeys over an insertion into or removal from the QCache. Two (insert() and remove()) have already been done in prequel commits, so only flushDetachedPixmaps() was left. Fixes: QTBUG-112200 Change-Id: Ic93b0ed388ae963267fe242b491c6c941d146b99 Reviewed-by: Qt CI Bot Reviewed-by: Volker Hilsheimer (cherry picked from commit 56644240851443b1259bff2098d221068dd3e8b5) Reviewed-by: Marc Mutz --- src/gui/image/qpixmapcache.cpp | 39 ++++++------------- src/gui/image/qpixmapcache_p.h | 1 + .../image/qpixmapcache/tst_qpixmapcache.cpp | 2 - 3 files changed, 12 insertions(+), 30 deletions(-) diff --git a/src/gui/image/qpixmapcache.cpp b/src/gui/image/qpixmapcache.cpp index e6b1f5ea385..46398582568 100644 --- a/src/gui/image/qpixmapcache.cpp +++ b/src/gui/image/qpixmapcache.cpp @@ -259,25 +259,12 @@ bool QPMCache::flushDetachedPixmaps(bool nt) { auto mc = maxCost(); const qsizetype currentTotal = totalCost(); + const qsizetype oldSize = size(); if (currentTotal) setMaxCost(nt ? currentTotal * 3 / 4 : currentTotal - 1); setMaxCost(mc); ps = totalCost(); - - bool any = false; - QHash::iterator it = cacheKeys.begin(); - while (it != cacheKeys.end()) { - const auto value = it.value(); - if (value.isValid() && !contains(value)) { - releaseKey(value); - it = cacheKeys.erase(it); - any = true; - } else { - ++it; - } - } - - return any; + return size() != oldSize; } void QPMCache::timerEvent(QTimerEvent *) @@ -296,17 +283,9 @@ void QPMCache::timerEvent(QTimerEvent *) QPixmap *QPMCache::object(const QString &key) const { - QPixmapCache::Key cacheKey = cacheKeys.value(key); - if (!cacheKey.d || !cacheKey.d->isValid) { - const_cast(this)->cacheKeys.remove(key); - return nullptr; - } - QPixmap *ptr = QCache::object(cacheKey); - //We didn't find the pixmap in the cache, the key is not valid anymore - if (!ptr) { - const_cast(this)->cacheKeys.remove(key); - } - return ptr; + if (const auto it = cacheKeys.find(key); it != cacheKeys.cend()) + return object(it.value()); + return nullptr; } QPixmap *QPMCache::object(const QPixmapCache::Key &key) const @@ -327,6 +306,7 @@ bool QPMCache::insert(const QString& key, const QPixmap &pixmap, int cost) // this will create a new key; the old one has been removed auto k = insert(pixmap, cost); if (k.isValid()) { + k.d->stringKey = key; cacheKeys[key] = std::move(k); return true; } @@ -383,7 +363,11 @@ QPixmapCache::Key QPMCache::createKey() void QPMCache::releaseKey(const QPixmapCache::Key &key) { QPixmapCache::KeyData *keyData = key.d; - if (!keyData || keyData->key > keyArraySize || keyData->key <= 0) + if (!keyData) + return; + if (!keyData->stringKey.isNull()) + cacheKeys.remove(keyData->stringKey); + if (keyData->key > keyArraySize || keyData->key <= 0) return; keyData->key--; keyArray[keyData->key] = freeKey; @@ -410,7 +394,6 @@ void QPMCache::clear() killTimer(theid); theid = 0; } - cacheKeys.clear(); } QPixmapCache::KeyData* QPMCache::getKeyData(QPixmapCache::Key *key) diff --git a/src/gui/image/qpixmapcache_p.h b/src/gui/image/qpixmapcache_p.h index 38a28281b43..43c4d9784c6 100644 --- a/src/gui/image/qpixmapcache_p.h +++ b/src/gui/image/qpixmapcache_p.h @@ -31,6 +31,7 @@ public: : isValid(other.isValid), key(other.key), ref(1) {} ~KeyData() {} + QString stringKey; bool isValid; int key; int ref; diff --git a/tests/auto/gui/image/qpixmapcache/tst_qpixmapcache.cpp b/tests/auto/gui/image/qpixmapcache/tst_qpixmapcache.cpp index 99eeadb994c..e42cdbb7f1f 100644 --- a/tests/auto/gui/image/qpixmapcache/tst_qpixmapcache.cpp +++ b/tests/auto/gui/image/qpixmapcache/tst_qpixmapcache.cpp @@ -542,7 +542,6 @@ void tst_QPixmapCache::evictionDoesNotLeakStringKeys() pm.fill(Qt::transparent); [[maybe_unused]] auto r = QPixmapCache::insert(pm); } - QEXPECT_FAIL("", "QTBUG-112200", Continue); }); } @@ -550,7 +549,6 @@ void tst_QPixmapCache::reducingCacheLimitDoesNotLeakStringKeys() { stringLeak_impl([] { QPixmapCache::setCacheLimit(0); - QEXPECT_FAIL("", "QTBUG-112200", Continue); }); }