From: Chris Adams Date: Mon, 21 Nov 2011 23:59:33 +0000 (+1000) Subject: Release pixmap cache data to avoid leaking memory X-Git-Url: http://git.silmor.de/gitweb/?a=commitdiff_plain;h=b99c9490c398fb4d285d5f28edd553445b76d79f;p=konrad%2Fqtdeclarative.git Release pixmap cache data to avoid leaking memory Previously, QDeclarativePixmapStore did not release cache entries on destruction. This commit ensures that all cache entries are released properly. Note that while any QDeclarativePixmapData which contains a texture will be deleted, the texture itself will be scheduled for cleanup rather than released directly. Task-number: QTBUG-22742 Change-Id: I62ddf57f2b55b732ab369321eb9ed0d7af01c135 Reviewed-by: Martin Jones --- diff --git a/src/declarative/util/qdeclarativepixmapcache.cpp b/src/declarative/util/qdeclarativepixmapcache.cpp index 7ff84ab..e366e6b 100644 --- a/src/declarative/util/qdeclarativepixmapcache.cpp +++ b/src/declarative/util/qdeclarativepixmapcache.cpp @@ -189,43 +189,54 @@ public: class QDeclarativePixmapData { public: - QDeclarativePixmapData(const QUrl &u, const QSize &s, const QString &e) + QDeclarativePixmapData(QDeclarativePixmap *pixmap, const QUrl &u, const QSize &s, const QString &e) : refCount(1), inCache(false), pixmapStatus(QDeclarativePixmap::Error), - url(u), errorString(e), requestSize(s), texture(0), context(0), reply(0), prevUnreferenced(0), - prevUnreferencedPtr(0), nextUnreferenced(0) + url(u), errorString(e), requestSize(s), texture(0), context(0), + reply(0), prevUnreferenced(0), prevUnreferencedPtr(0), nextUnreferenced(0) { + declarativePixmaps.insert(pixmap); } - QDeclarativePixmapData(const QUrl &u, const QSize &r) + QDeclarativePixmapData(QDeclarativePixmap *pixmap, const QUrl &u, const QSize &r) : refCount(1), inCache(false), pixmapStatus(QDeclarativePixmap::Loading), - url(u), requestSize(r), texture(0), context(0), reply(0), prevUnreferenced(0), prevUnreferencedPtr(0), - nextUnreferenced(0) + url(u), requestSize(r), texture(0), context(0), + reply(0), prevUnreferenced(0), prevUnreferencedPtr(0), nextUnreferenced(0) { + declarativePixmaps.insert(pixmap); } - QDeclarativePixmapData(const QUrl &u, const QPixmap &p, const QSize &s, const QSize &r) + QDeclarativePixmapData(QDeclarativePixmap *pixmap, const QUrl &u, const QPixmap &p, const QSize &s, const QSize &r) : refCount(1), inCache(false), privatePixmap(false), pixmapStatus(QDeclarativePixmap::Ready), - url(u), pixmap(p), implicitSize(s), requestSize(r), texture(0), context(0), reply(0), prevUnreferenced(0), - prevUnreferencedPtr(0), nextUnreferenced(0) + url(u), pixmap(p), implicitSize(s), requestSize(r), texture(0), context(0), + reply(0), prevUnreferenced(0), prevUnreferencedPtr(0), nextUnreferenced(0) { + declarativePixmaps.insert(pixmap); } - QDeclarativePixmapData(const QUrl &u, QSGTexture *t, QSGContext *c, const QPixmap &p, const QSize &s, const QSize &r) + QDeclarativePixmapData(QDeclarativePixmap *pixmap, const QUrl &u, QSGTexture *t, QSGContext *c, const QPixmap &p, const QSize &s, const QSize &r) : refCount(1), inCache(false), privatePixmap(false), pixmapStatus(QDeclarativePixmap::Ready), - url(u), pixmap(p), implicitSize(s), requestSize(r), texture(t), context(c), reply(0), prevUnreferenced(0), - prevUnreferencedPtr(0), nextUnreferenced(0) + url(u), pixmap(p), implicitSize(s), requestSize(r), texture(t), context(c), + reply(0), prevUnreferenced(0), prevUnreferencedPtr(0), nextUnreferenced(0) { + declarativePixmaps.insert(pixmap); } - QDeclarativePixmapData(const QPixmap &p) + QDeclarativePixmapData(QDeclarativePixmap *pixmap, const QPixmap &p) : refCount(1), inCache(false), privatePixmap(true), pixmapStatus(QDeclarativePixmap::Ready), - pixmap(p), implicitSize(p.size()), requestSize(p.size()), texture(0), context(0), reply(0), prevUnreferenced(0), - prevUnreferencedPtr(0), nextUnreferenced(0) + pixmap(p), implicitSize(p.size()), requestSize(p.size()), texture(0), context(0), + reply(0), prevUnreferenced(0), prevUnreferencedPtr(0), nextUnreferenced(0) { + declarativePixmaps.insert(pixmap); } ~QDeclarativePixmapData() { + while (!declarativePixmaps.isEmpty()) { + QDeclarativePixmap *referencer = declarativePixmaps.first(); + declarativePixmaps.remove(referencer); + referencer->d = 0; + } + if (texture && context) { context->scheduleTextureForCleanup(texture); } @@ -252,6 +263,7 @@ public: QSGTexture *texture; QSGContext *context; + QIntrusiveList declarativePixmaps; QDeclarativePixmapReply *reply; QDeclarativePixmapData *prevUnreferenced; @@ -705,6 +717,7 @@ class QDeclarativePixmapStore : public QObject Q_OBJECT public: QDeclarativePixmapStore(); + ~QDeclarativePixmapStore(); void unreferencePixmap(QDeclarativePixmapData *); void referencePixmap(QDeclarativePixmapData *); @@ -741,6 +754,35 @@ QDeclarativePixmapStore::QDeclarativePixmapStore() { } +QDeclarativePixmapStore::~QDeclarativePixmapStore() +{ + int leakedPixmaps = 0; + int leakedTextures = 0; + QList cachedData = m_cache.values(); + + // unreference all (leaked) pixmaps + foreach (QDeclarativePixmapData* pixmap, cachedData) { + int currRefCount = pixmap->refCount; + if (currRefCount) { + leakedPixmaps++; + if (pixmap->texture) + leakedTextures++; + while (currRefCount > 0) { + pixmap->release(); + currRefCount--; + } + } + } + + // free all unreferenced pixmaps + while (m_lastUnreferencedPixmap) { + shrinkCache(20); + } + + if (leakedPixmaps) + qDebug("Number of leaked pixmaps: %i (of which %i are leaked textures)", leakedPixmaps, leakedTextures); +} + void QDeclarativePixmapStore::cleanTextureForContext(QDeclarativePixmapData *data) { if (data->context) { @@ -953,7 +995,7 @@ void QDeclarativePixmapData::removeFromCache() } } -static QDeclarativePixmapData* createPixmapDataSync(QDeclarativeEngine *engine, const QUrl &url, const QSize &requestSize, bool *ok) +static QDeclarativePixmapData* createPixmapDataSync(QDeclarativePixmap *declarativePixmap, QDeclarativeEngine *engine, const QUrl &url, const QSize &requestSize, bool *ok) { QSGContext *sgContext = QDeclarativeEnginePrivate::get(engine)->sgContext; @@ -964,14 +1006,14 @@ static QDeclarativePixmapData* createPixmapDataSync(QDeclarativeEngine *engine, switch (imageType) { case QDeclarativeImageProvider::Invalid: - return new QDeclarativePixmapData(url, requestSize, + return new QDeclarativePixmapData(declarativePixmap, url, requestSize, QDeclarativePixmap::tr("Invalid image provider: %1").arg(url.toString())); case QDeclarativeImageProvider::Texture: { QSGTexture *texture = ep->getTextureFromProvider(url, &readSize, requestSize); if (texture) { *ok = true; - return new QDeclarativePixmapData(url, texture, sgContext, QPixmap(), readSize, requestSize); + return new QDeclarativePixmapData(declarativePixmap, url, texture, sgContext, QPixmap(), readSize, requestSize); } } @@ -982,9 +1024,9 @@ static QDeclarativePixmapData* createPixmapDataSync(QDeclarativeEngine *engine, *ok = true; if (sgContext) { QSGTexture *t = sgContext->createTexture(image); - return new QDeclarativePixmapData(url, t, sgContext, QPixmap::fromImage(image), readSize, requestSize); + return new QDeclarativePixmapData(declarativePixmap, url, t, sgContext, QPixmap::fromImage(image), readSize, requestSize); } - return new QDeclarativePixmapData(url, QPixmap::fromImage(image), readSize, requestSize); + return new QDeclarativePixmapData(declarativePixmap, url, QPixmap::fromImage(image), readSize, requestSize); } } case QDeclarativeImageProvider::Pixmap: @@ -994,15 +1036,15 @@ static QDeclarativePixmapData* createPixmapDataSync(QDeclarativeEngine *engine, *ok = true; if (sgContext) { QSGTexture *t = sgContext->createTexture(pixmap.toImage()); - return new QDeclarativePixmapData(url, t, sgContext, pixmap, readSize, requestSize); + return new QDeclarativePixmapData(declarativePixmap, url, t, sgContext, pixmap, readSize, requestSize); } - return new QDeclarativePixmapData(url, pixmap, readSize, requestSize); + return new QDeclarativePixmapData(declarativePixmap, url, pixmap, readSize, requestSize); } } } // provider has bad image type, or provider returned null image - return new QDeclarativePixmapData(url, requestSize, + return new QDeclarativePixmapData(declarativePixmap, url, requestSize, QDeclarativePixmap::tr("Failed to get image from provider: %1").arg(url.toString())); } @@ -1034,14 +1076,14 @@ static QDeclarativePixmapData* createPixmapDataSync(QDeclarativeEngine *engine, } if (texture) - return new QDeclarativePixmapData(url, texture, ctx, QPixmap::fromImage(image), readSize, requestSize); + return new QDeclarativePixmapData(declarativePixmap, url, texture, ctx, QPixmap::fromImage(image), readSize, requestSize); else - return new QDeclarativePixmapData(url, QPixmap::fromImage(image), readSize, requestSize); + return new QDeclarativePixmapData(declarativePixmap, url, QPixmap::fromImage(image), readSize, requestSize); } else { errorString = QDeclarativePixmap::tr("Cannot open: %1").arg(url.toString()); } - return new QDeclarativePixmapData(url, requestSize, errorString); + return new QDeclarativePixmapData(declarativePixmap, url, requestSize, errorString); } @@ -1072,6 +1114,7 @@ QDeclarativePixmap::QDeclarativePixmap(QDeclarativeEngine *engine, const QUrl &u QDeclarativePixmap::~QDeclarativePixmap() { if (d) { + d->declarativePixmaps.remove(this); d->release(); d = 0; } @@ -1164,7 +1207,7 @@ void QDeclarativePixmap::setPixmap(const QPixmap &p) clear(); if (!p.isNull()) - d = new QDeclarativePixmapData(p); + d = new QDeclarativePixmapData(this, p); } int QDeclarativePixmap::width() const @@ -1208,7 +1251,11 @@ void QDeclarativePixmap::load(QDeclarativeEngine *engine, const QUrl &url, const void QDeclarativePixmap::load(QDeclarativeEngine *engine, const QUrl &url, const QSize &requestSize, QDeclarativePixmap::Options options) { - if (d) { d->release(); d = 0; } + if (d) { + d->declarativePixmaps.remove(this); + d->release(); + d = 0; + } QDeclarativePixmapKey key = { &url, &requestSize }; QDeclarativePixmapStore *store = pixmapStore(); @@ -1226,7 +1273,7 @@ void QDeclarativePixmap::load(QDeclarativeEngine *engine, const QUrl &url, const if (!(options & QDeclarativePixmap::Asynchronous)) { bool ok = false; - d = createPixmapDataSync(engine, url, requestSize, &ok); + d = createPixmapDataSync(this, engine, url, requestSize, &ok); if (ok) { if (options & QDeclarativePixmap::Cache) d->addToCache(); @@ -1239,7 +1286,7 @@ void QDeclarativePixmap::load(QDeclarativeEngine *engine, const QUrl &url, const if (!engine) return; - d = new QDeclarativePixmapData(url, requestSize); + d = new QDeclarativePixmapData(this, url, requestSize); if (options & QDeclarativePixmap::Cache) d->addToCache(); @@ -1249,12 +1296,14 @@ void QDeclarativePixmap::load(QDeclarativeEngine *engine, const QUrl &url, const } else { d = *iter; d->addref(); + d->declarativePixmaps.insert(this); } } void QDeclarativePixmap::clear() { if (d) { + d->declarativePixmaps.remove(this); d->release(); d = 0; } @@ -1265,6 +1314,7 @@ void QDeclarativePixmap::clear(QObject *obj) if (d) { if (d->reply) QObject::disconnect(d->reply, 0, obj, 0); + d->declarativePixmaps.remove(this); d->release(); d = 0; } diff --git a/src/declarative/util/qdeclarativepixmapcache_p.h b/src/declarative/util/qdeclarativepixmapcache_p.h index c1256d7..542ac3e 100644 --- a/src/declarative/util/qdeclarativepixmapcache_p.h +++ b/src/declarative/util/qdeclarativepixmapcache_p.h @@ -47,6 +47,8 @@ #include #include +#include + QT_BEGIN_HEADER QT_BEGIN_NAMESPACE @@ -111,6 +113,8 @@ public: private: Q_DISABLE_COPY(QDeclarativePixmap) QDeclarativePixmapData *d; + QIntrusiveListNode dataListNode; + friend class QDeclarativePixmapData; }; inline QDeclarativePixmap::operator const QPixmap &() const diff --git a/tests/auto/declarative/qdeclarativepixmapcache/data/dataLeak.qml b/tests/auto/declarative/qdeclarativepixmapcache/data/dataLeak.qml new file mode 100644 index 0000000..724ce5d --- /dev/null +++ b/tests/auto/declarative/qdeclarativepixmapcache/data/dataLeak.qml @@ -0,0 +1,18 @@ +import QtQuick 2.0 + +Item { + id: root + width: 800 + height: 800 + + Image { + id: i1 + source: "exists1.png"; + anchors.top: parent.top; + } + Image { + id: i2 + source: "exists2.png" + anchors.top: i1.bottom; + } +} diff --git a/tests/auto/declarative/qdeclarativepixmapcache/tst_qdeclarativepixmapcache.cpp b/tests/auto/declarative/qdeclarativepixmapcache/tst_qdeclarativepixmapcache.cpp index 5224de3..1bd0e70 100644 --- a/tests/auto/declarative/qdeclarativepixmapcache/tst_qdeclarativepixmapcache.cpp +++ b/tests/auto/declarative/qdeclarativepixmapcache/tst_qdeclarativepixmapcache.cpp @@ -44,6 +44,7 @@ #include #include #include +#include "../shared/util.h" #include "testhttpserver.h" #ifndef QT_NO_CONCURRENT @@ -51,15 +52,19 @@ #include #endif +inline QUrl TEST_FILE(const QString &filename) +{ + return QUrl::fromLocalFile(TESTDATA(filename)); +} + class tst_qdeclarativepixmapcache : public QObject { Q_OBJECT public: tst_qdeclarativepixmapcache() : - thisfile(QUrl::fromLocalFile(QCoreApplication::applicationFilePath())), server(14452) { - server.serveDirectory(QCoreApplication::applicationDirPath() + "/data/http"); + server.serveDirectory(TESTDATA("http")); } private slots: @@ -74,13 +79,12 @@ private slots: void networkCrash(); #endif void lockingCrash(); + void dataLeak(); private: QDeclarativeEngine engine; - QUrl thisfile; TestHTTPServer server; }; - static int slotters=0; class Slotter : public QObject @@ -121,8 +125,8 @@ void tst_qdeclarativepixmapcache::single_data() QTest::addColumn("neterror"); // File URLs are optimized - QTest::newRow("local") << thisfile.resolved(QUrl("data/exists.png")) << localfile_optimized << true << false; - QTest::newRow("local") << thisfile.resolved(QUrl("data/notexists.png")) << localfile_optimized << false << false; + QTest::newRow("local") << TEST_FILE("exists.png") << localfile_optimized << true << false; + QTest::newRow("local") << TEST_FILE("notexists.png") << localfile_optimized << false << false; QTest::newRow("remote") << QUrl("http://127.0.0.1:14452/exists.png") << false << true << false; QTest::newRow("remote") << QUrl("http://127.0.0.1:14452/notexists.png") << false << false << true; } @@ -185,8 +189,8 @@ void tst_qdeclarativepixmapcache::parallel_data() QTest::addColumn("cancel"); // which one to cancel QTest::newRow("local") - << thisfile.resolved(QUrl("data/exists1.png")) - << thisfile.resolved(QUrl("data/exists2.png")) + << TEST_FILE("exists1.png") + << TEST_FILE("exists2.png") << (localfile_optimized ? 2 : 0) << -1; @@ -282,7 +286,7 @@ void tst_qdeclarativepixmapcache::parallel() void tst_qdeclarativepixmapcache::massive() { QDeclarativeEngine engine; - QUrl url = thisfile.resolved(QUrl("data/massive.png")); + QUrl url = TEST_FILE("massive.png"); // Confirm that massive images remain in the cache while they are // in use by the application. @@ -360,7 +364,7 @@ void createNetworkServer() { QEventLoop eventLoop; TestHTTPServer server(14453); - server.serveDirectory(QCoreApplication::applicationDirPath() + "/data/http"); + server.serveDirectory(TESTDATA("http")); QTimer::singleShot(100, &eventLoop, SLOT(quit())); eventLoop.exec(); } @@ -388,7 +392,7 @@ void tst_qdeclarativepixmapcache::networkCrash() void tst_qdeclarativepixmapcache::lockingCrash() { TestHTTPServer server(14453); - server.serveDirectory(QCoreApplication::applicationDirPath() + "/data/http", TestHTTPServer::Delay); + server.serveDirectory(TESTDATA("http"), TestHTTPServer::Delay); { QDeclarativePixmap* p = new QDeclarativePixmap; @@ -402,6 +406,53 @@ void tst_qdeclarativepixmapcache::lockingCrash() } } +#include +class DataLeakView : public QQuickView +{ + Q_OBJECT + +public: + explicit DataLeakView() : QQuickView() + { + setSource(TEST_FILE("dataLeak.qml")); + } + + void showFor2Seconds() + { + showFullScreen(); + QTimer::singleShot(2000, this, SIGNAL(ready())); + } + +signals: + void ready(); +}; + +// QTBUG-22742 +Q_GLOBAL_STATIC(QDeclarativePixmap, dataLeakPixmap) +void tst_qdeclarativepixmapcache::dataLeak() +{ + // Should not leak cached QDeclarativePixmapData. + // Unfortunately, since the QDeclarativePixmapStore + // is a global static, and it releases the cache + // entries on dtor (application exit), we must use + // valgrind to determine whether it leaks or not. + QDeclarativePixmap *p1 = new QDeclarativePixmap; + QDeclarativePixmap *p2 = new QDeclarativePixmap; + { + QScopedPointer test(new DataLeakView); + test->showFor2Seconds(); + dataLeakPixmap()->load(test->engine(), TEST_FILE("exists.png")); + p1->load(test->engine(), TEST_FILE("exists.png")); + p2->load(test->engine(), TEST_FILE("exists2.png")); + QTest::qWait(2005); // 2 seconds + a few more millis. + } + + // When the (global static) dataLeakPixmap is deleted, it + // shouldn't attempt to dereference a QDeclarativePixmapData + // which has been deleted by the QDeclarativePixmapStore + // destructor. +} + QTEST_MAIN(tst_qdeclarativepixmapcache) #include "tst_qdeclarativepixmapcache.moc"