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
Change-Id: Ic93b0ed388ae963267fe242b491c6c941d146b99
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
Reviewed-by: Volker Hilsheimer <volker.hilsheimer@qt.io>
(cherry picked from commit 56644240851443b1259bff2098d221068dd3e8b5)
Reviewed-by: Marc Mutz <marc.mutz@qt.io>

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<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 *)
@@ -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<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
@@ -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)

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;

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);
     });
 }