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<QString, Key> 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
Pick-to: 6.6 6.5
Change-Id: Ic93b0ed388ae963267fe242b491c6c941d146b99
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
Reviewed-by: Volker Hilsheimer <volker.hilsheimer@qt.io>

3 files changed, 12 insertions, 30 deletions

diff --git a/src/gui/image/qpixmapcache.cpp b/src/gui/image/qpixmapcache.cpp
index 6cc5fd28a3..a57f144464 100644
--- a/src/gui/image/qpixmapcache.cpp
+++ b/src/gui/image/qpixmapcache.cpp
@@ -254,25 +254,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<QString, QPixmapCache::Key>::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 *)
@@ -291,17 +278,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<QPMCache *>(this)->cacheKeys.remove(key);
-        return nullptr;
-    }
-    QPixmap *ptr = QCache<QPixmapCache::Key, QPixmapCacheEntry>::object(cacheKey);
-     //We didn't find the pixmap in the cache, the key is not valid anymore
-    if (!ptr) {
-        const_cast<QPMCache *>(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
@@ -322,6 +301,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;
     }
@@ -378,7 +358,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;
@@ -405,7 +389,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 6418296f56..c9a9056301 100644
--- a/src/gui/image/qpixmapcache_p.h
+++ b/src/gui/image/qpixmapcache_p.h
@@ -33,6 +33,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 99eeadb994..e42cdbb7f1 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);
     });
 }