Fix memory leak in qquickpixmapcache

QtPdf depends on QQuickPixmap::loadImageFromDevice() to pass a QPdfFile
(which carries a pointer to the QPdfDocument) down into QPdfIOHandler
so that it can avoid needing to reopen the document. But the
introduction of loadImageFromDevice(), and then using the pixmap cache
so intensively to store a lot of rendered PDF pages, revealed a drastic
memory leak: many QQuickPixmapData instances remained in
QQuickPixmapStore::m_cache long after their refCounts became 0.
Quick scrolling through a PdfMultiPageView (especially if the zoom level
is also being changed dynamically) can easily generate a lot of
page-rendering jobs, some of which get cancelled early because the view
has moved on to other pages and/or zoom levels.

Here are several related fixes:

- QQuickPixmapReader::getImage() creates a "job" represented by
  the QQuickPixmapReply that is returned, but now we start the job
  in a separate method, to ensure that
  QQuickPixmap::loadImageFromDevice() succeeds in connecting the
  QQuickPixmapReply::destroyed() signal before the job completes
  and deletes itself.

- Use a QMutex to prevent QQuickPixmapStore::m_cache reentrancy:
  Rarely, a crash was possible when QQuickPixmapStore's QHash m_cache
  was modified between QHash::find() and dereferencing the iterator it
  returns, e.g. in QQuickPixmap::loadImageFromDevice().

- QQuickPixmapData::release() is needed in a few more places.

- QQuickPixmapReply::finished isn't emitted if the job is cancelled, so
  the lambda that called oldD->release() wasn't called, and that was the
  remaining leak. We tried connecting QObject::destroyed() earlier, but
  it was causing a crash because the mutex to protect m_cache from
  simultaneous modification and QHash::find() was not in place. Now it
  seems ok.

- Show "Number of leaked pixmaps: 0" when QML_LEAK_CHECK is set
  to make verification less ambiguous.

Task-number: QTBUG-114953
Change-Id: I11518ccf28ee5a5055ae745a5921837d2f3151b6
Reviewed-by: Axel Spoerl <axel.spoerl@qt.io>
(cherry picked from commit c871a52f94a4836515754e641ee02386a9876a58)

1 files changed, 36 insertions, 11 deletions

diff --git a/src/quick/util/qquickpixmapcache.cpp b/src/quick/util/qquickpixmapcache.cpp
index 5a8334210c..f0d9959d87 100644
--- a/src/quick/util/qquickpixmapcache.cpp
+++ b/src/quick/util/qquickpixmapcache.cpp
@@ -165,6 +165,7 @@ public:
 
     static QQuickPixmapReader *instance(QQmlEngine *engine);
     static QQuickPixmapReader *existingInstance(QQmlEngine *engine);
+    void startJob(QQuickPixmapReply *job);
 
 protected:
     void run() override;
@@ -1012,15 +1013,17 @@ QQuickPixmapReader *QQuickPixmapReader::existingInstance(QQmlEngine *engine)
 
 QQuickPixmapReply *QQuickPixmapReader::getImage(QQuickPixmapData *data)
 {
-    mutex.lock();
     QQuickPixmapReply *reply = new QQuickPixmapReply(data);
     reply->engineForReader = engine;
-    jobs.append(reply);
-    // XXX
+    return reply;
+}
+
+void QQuickPixmapReader::startJob(QQuickPixmapReply *job)
+{
+    QMutexLocker locker(&mutex);
+    jobs.append(job);
     if (threadObject())
         threadObject()->processJobs();
-    mutex.unlock();
-    return reply;
 }
 
 void QQuickPixmapReader::cancel(QQuickPixmapReply *reply)
@@ -1118,6 +1121,7 @@ protected:
 
 public:
     QHash<QQuickPixmapKey, QQuickPixmapData *> m_cache;
+    QMutex m_cacheMutex; // avoid simultaneous iteration and modification
 
 private:
     void shrinkCache(int remove);
@@ -1170,7 +1174,7 @@ QQuickPixmapStore::~QQuickPixmapStore()
     }
 
 #ifndef QT_NO_DEBUG
-    if (leakedPixmaps && _q_sg_leak_check)
+    if (_q_sg_leak_check)
         qDebug("Number of leaked pixmaps: %i", leakedPixmaps);
 #endif
 }
@@ -1367,6 +1371,7 @@ void QQuickPixmapData::addToCache()
 {
     if (!inCache) {
         QQuickPixmapKey key = { &url, &requestRegion, &requestSize, frame, providerOptions };
+        QMutexLocker locker(&pixmapStore()->m_cacheMutex);
         pixmapStore()->m_cache.insert(key, this);
         inCache = true;
         PIXMAP_PROFILE(pixmapCountChanged<QQuickProfiler::PixmapCacheCountChanged>(
@@ -1381,6 +1386,7 @@ void QQuickPixmapData::removeFromCache(QQuickPixmapStore *store)
         if (!store)
             store = pixmapStore();
         QQuickPixmapKey key = { &url, &requestRegion, &requestSize, frame, providerOptions };
+        QMutexLocker locker(&pixmapStore()->m_cacheMutex);
         store->m_cache.remove(key);
         inCache = false;
         PIXMAP_PROFILE(pixmapCountChanged<QQuickProfiler::PixmapCacheCountChanged>(
@@ -1639,15 +1645,22 @@ void QQuickPixmap::setImage(const QImage &p)
 {
     clear();
 
-    if (!p.isNull())
+    if (!p.isNull()) {
+        if (d)
+            d->release();
         d = new QQuickPixmapData(this, QQuickTextureFactory::textureFactoryForImage(p));
+    }
 }
 
 void QQuickPixmap::setPixmap(const QQuickPixmap &other)
 {
+    if (d == other.d)
+        return;
     clear();
 
     if (other.d) {
+        if (d)
+            d->release();
         d = other.d;
         d->addref();
         d->declarativePixmaps.insert(this);
@@ -1711,6 +1724,7 @@ void QQuickPixmap::load(QQmlEngine *engine, const QUrl &url, const QRect &reques
     QQuickPixmapKey key = { &url, &requestRegion, &requestSize, frame, providerOptions };
     QQuickPixmapStore *store = pixmapStore();
 
+    QMutexLocker locker(&pixmapStore()->m_cacheMutex);
     QHash<QQuickPixmapKey, QQuickPixmapData *>::Iterator iter = store->m_cache.end();
 
 #ifdef Q_OS_WEBOS
@@ -1734,6 +1748,7 @@ void QQuickPixmap::load(QQmlEngine *engine, const QUrl &url, const QRect &reques
         iter = store->m_cache.find(key);
 
     if (iter == store->m_cache.end()) {
+        locker.unlock();
         if (url.scheme() == QLatin1String("image")) {
             QQmlEnginePrivate *enginePrivate = QQmlEnginePrivate::get(engine);
             if (auto provider = enginePrivate->imageProvider(imageProviderId(url)).staticCast<QQuickImageProvider>()) {
@@ -1779,7 +1794,9 @@ void QQuickPixmap::load(QQmlEngine *engine, const QUrl &url, const QRect &reques
 #endif
 
         QQuickPixmapReader::readerMutex.lock();
-        d->reply = QQuickPixmapReader::instance(engine)->getImage(d);
+        QQuickPixmapReader *reader = QQuickPixmapReader::instance(engine);
+        d->reply = reader->getImage(d);
+        reader->startJob(d->reply);
         QQuickPixmapReader::readerMutex.unlock();
     } else {
         d = *iter;
@@ -1803,28 +1820,36 @@ void QQuickPixmap::loadImageFromDevice(QQmlEngine *engine, QIODevice *device, co
     QQuickPixmapKey key = { &url, &requestRegion, &requestSize, frame, providerOptions };
     QQuickPixmapStore *store = pixmapStore();
     QHash<QQuickPixmapKey, QQuickPixmapData *>::Iterator iter = store->m_cache.end();
+    QMutexLocker locker(&store->m_cacheMutex);
     iter = store->m_cache.find(key);
     if (iter == store->m_cache.end()) {
         if (!engine)
             return;
 
+        locker.unlock();
         d = new QQuickPixmapData(this, url, requestRegion, requestSize, providerOptions,
                                  QQuickImageProviderOptions::UsePluginDefaultTransform, frame, frameCount);
         d->specialDevice = device;
         d->addToCache();
 
         QQuickPixmapReader::readerMutex.lock();
-        d->reply = QQuickPixmapReader::instance(engine)->getImage(d);
+        QQuickPixmapReader *reader = QQuickPixmapReader::instance(engine);
+        d->reply = reader->getImage(d);
         if (oldD) {
-            QObject::connect(d->reply, &QQuickPixmapReply::finished, [oldD]() {
+            QObject::connect(d->reply, &QQuickPixmapReply::destroyed, store, [oldD]() {
                 oldD->release();
-            });
+            }, Qt::QueuedConnection);
         }
+        reader->startJob(d->reply);
         QQuickPixmapReader::readerMutex.unlock();
     } else {
         d = *iter;
         d->addref();
         d->declarativePixmaps.insert(this);
+        qCDebug(lcImg) << "loaded from cache" << url << "frame" << frame << "refCount" << d->refCount;
+        locker.unlock();
+        if (oldD)
+            oldD->release();
     }
 }