From 328d1d2fd6cba7368230a1232e080d3f3310a7f1 Mon Sep 17 00:00:00 2001 From: Chris Adams Date: Wed, 16 Nov 2011 13:20:45 +1000 Subject: Properly protect access to pixmap reader thread with mutex Previously, access to the reader thread wasn't guarded properly, causing a crash when the reader thread was deleted prior to QDeclarativePixmapData (which then attempted to dereference the thread pointer to cancel the request). This commit adds locking to ensure that access to the reader is protected properly. This commit also fixes an issue where if a QDeclarativePixmapData was deleted after its QDeclarativePixmapReply was removed from the jobs queue but prior to processing, a pointer to the data could be dereferenced as part of processing, but caching the required information in the reply. Task-number: QTBUG-22125 Change-Id: I32aa2ca41a297b48e68cb358f4aa4fafd999f215 Reviewed-by: Martin Jones --- src/declarative/util/qdeclarativepixmapcache.cpp | 53 ++++++++++++++++----- .../qdeclarativepixmapcache/data/http/exists6.png | Bin 0 -> 2738 bytes .../qdeclarativepixmapcache/data/http/exists7.png | Bin 0 -> 2738 bytes .../qdeclarativepixmapcache/data/http/exists8.png | Bin 0 -> 2738 bytes .../tst_qdeclarativepixmapcache.cpp | 19 ++++++++ .../declarative/qquickimage/data/qtbug_22125.qml | 44 +++++++++++++++++ .../declarative/qquickimage/tst_qquickimage.cpp | 22 +++++++++ 7 files changed, 126 insertions(+), 12 deletions(-) create mode 100644 tests/auto/declarative/qdeclarativepixmapcache/data/http/exists6.png create mode 100644 tests/auto/declarative/qdeclarativepixmapcache/data/http/exists7.png create mode 100644 tests/auto/declarative/qdeclarativepixmapcache/data/http/exists8.png create mode 100644 tests/auto/declarative/qquickimage/data/qtbug_22125.qml diff --git a/src/declarative/util/qdeclarativepixmapcache.cpp b/src/declarative/util/qdeclarativepixmapcache.cpp index 937dd4cda5..7ff84abcf1 100644 --- a/src/declarative/util/qdeclarativepixmapcache.cpp +++ b/src/declarative/util/qdeclarativepixmapcache.cpp @@ -90,8 +90,9 @@ public: ~QDeclarativePixmapReply(); QDeclarativePixmapData *data; - QDeclarativePixmapReader *reader; + QDeclarativeEngine *engineForReader; // always access reader inside readerMutex QSize requestSize; + QUrl url; bool loading; int redirectCount; @@ -151,6 +152,7 @@ public: void cancel(QDeclarativePixmapReply *rep); static QDeclarativePixmapReader *instance(QDeclarativeEngine *engine); + static QDeclarativePixmapReader *existingInstance(QDeclarativeEngine *engine); protected: void run(); @@ -180,6 +182,7 @@ private: static int downloadProgress; static int threadNetworkRequestDone; static QHash readers; +public: static QMutex readerMutex; }; @@ -360,6 +363,22 @@ QDeclarativePixmapReader::~QDeclarativePixmapReader() readers.remove(engine); readerMutex.unlock(); + mutex.lock(); + // manually cancel all outstanding jobs. + foreach (QDeclarativePixmapReply *reply, jobs) { + delete reply; + } + jobs.clear(); + QList activeJobs = replies.values(); + foreach (QDeclarativePixmapReply *reply, activeJobs) { + if (reply->loading) { + cancelled.append(reply); + reply->data = 0; + } + } + if (threadObject) threadObject->processJobs(); + mutex.unlock(); + eventLoopQuitHack->deleteLater(); wait(); } @@ -483,8 +502,8 @@ void QDeclarativePixmapReader::processJobs() QDeclarativePixmapReply *runningJob = jobs.takeLast(); runningJob->loading = true; - QUrl url = runningJob->data->url; - QSize requestSize = runningJob->data->requestSize; + QUrl url = runningJob->url; + QSize requestSize = runningJob->requestSize; locker.unlock(); processJob(runningJob, url, requestSize); locker.relock(); @@ -596,22 +615,27 @@ void QDeclarativePixmapReader::processJob(QDeclarativePixmapReply *runningJob, c QDeclarativePixmapReader *QDeclarativePixmapReader::instance(QDeclarativeEngine *engine) { - readerMutex.lock(); + // XXX NOTE: must be called within readerMutex locking. QDeclarativePixmapReader *reader = readers.value(engine); if (!reader) { reader = new QDeclarativePixmapReader(engine); readers.insert(engine, reader); } - readerMutex.unlock(); return reader; } +QDeclarativePixmapReader *QDeclarativePixmapReader::existingInstance(QDeclarativeEngine *engine) +{ + // XXX NOTE: must be called within readerMutex locking. + return readers.value(engine, 0); +} + QDeclarativePixmapReply *QDeclarativePixmapReader::getImage(QDeclarativePixmapData *data) { mutex.lock(); QDeclarativePixmapReply *reply = new QDeclarativePixmapReply(data); - reply->reader = this; + reply->engineForReader = engine; jobs.append(reply); // XXX if (threadObject) threadObject->processJobs(); @@ -827,7 +851,7 @@ void QDeclarativePixmapStore::timerEvent(QTimerEvent *) } QDeclarativePixmapReply::QDeclarativePixmapReply(QDeclarativePixmapData *d) -: data(d), reader(0), requestSize(d->requestSize), loading(false), redirectCount(0) +: data(d), engineForReader(0), requestSize(d->requestSize), url(d->url), loading(false), redirectCount(0) { if (finishedIndex == -1) { finishedIndex = QDeclarativePixmapReply::staticMetaObject.indexOfSignal("finished()"); @@ -890,11 +914,16 @@ void QDeclarativePixmapData::release() { Q_ASSERT(refCount > 0); --refCount; - if (refCount == 0) { if (reply) { - reply->reader->cancel(reply); + QDeclarativePixmapReply *cancelReply = reply; + reply->data = 0; reply = 0; + QDeclarativePixmapReader::readerMutex.lock(); + QDeclarativePixmapReader *reader = QDeclarativePixmapReader::existingInstance(cancelReply->engineForReader); + if (reader) + reader->cancel(cancelReply); + QDeclarativePixmapReader::readerMutex.unlock(); } if (pixmapStatus == QDeclarativePixmap::Ready) { @@ -1210,13 +1239,13 @@ void QDeclarativePixmap::load(QDeclarativeEngine *engine, const QUrl &url, const if (!engine) return; - QDeclarativePixmapReader *reader = QDeclarativePixmapReader::instance(engine); - d = new QDeclarativePixmapData(url, requestSize); if (options & QDeclarativePixmap::Cache) d->addToCache(); - d->reply = reader->getImage(d); + QDeclarativePixmapReader::readerMutex.lock(); + d->reply = QDeclarativePixmapReader::instance(engine)->getImage(d); + QDeclarativePixmapReader::readerMutex.unlock(); } else { d = *iter; d->addref(); diff --git a/tests/auto/declarative/qdeclarativepixmapcache/data/http/exists6.png b/tests/auto/declarative/qdeclarativepixmapcache/data/http/exists6.png new file mode 100644 index 0000000000..399bd0b1d9 Binary files /dev/null and b/tests/auto/declarative/qdeclarativepixmapcache/data/http/exists6.png differ diff --git a/tests/auto/declarative/qdeclarativepixmapcache/data/http/exists7.png b/tests/auto/declarative/qdeclarativepixmapcache/data/http/exists7.png new file mode 100644 index 0000000000..399bd0b1d9 Binary files /dev/null and b/tests/auto/declarative/qdeclarativepixmapcache/data/http/exists7.png differ diff --git a/tests/auto/declarative/qdeclarativepixmapcache/data/http/exists8.png b/tests/auto/declarative/qdeclarativepixmapcache/data/http/exists8.png new file mode 100644 index 0000000000..399bd0b1d9 Binary files /dev/null and b/tests/auto/declarative/qdeclarativepixmapcache/data/http/exists8.png differ diff --git a/tests/auto/declarative/qdeclarativepixmapcache/tst_qdeclarativepixmapcache.cpp b/tests/auto/declarative/qdeclarativepixmapcache/tst_qdeclarativepixmapcache.cpp index 662b0d09ec..5224de3e56 100644 --- a/tests/auto/declarative/qdeclarativepixmapcache/tst_qdeclarativepixmapcache.cpp +++ b/tests/auto/declarative/qdeclarativepixmapcache/tst_qdeclarativepixmapcache.cpp @@ -73,6 +73,7 @@ private slots: #ifndef QT_NO_CONCURRENT void networkCrash(); #endif + void lockingCrash(); private: QDeclarativeEngine engine; QUrl thisfile; @@ -383,6 +384,24 @@ void tst_qdeclarativepixmapcache::networkCrash() #endif +// QTBUG-22125 +void tst_qdeclarativepixmapcache::lockingCrash() +{ + TestHTTPServer server(14453); + server.serveDirectory(QCoreApplication::applicationDirPath() + "/data/http", TestHTTPServer::Delay); + + { + QDeclarativePixmap* p = new QDeclarativePixmap; + { + QDeclarativeEngine e; + p->load(&e, QUrl(QString("http://127.0.0.1:14453/exists6.png"))); + } + p->clear(); + QVERIFY(p->isNull()); + delete p; + } +} + QTEST_MAIN(tst_qdeclarativepixmapcache) #include "tst_qdeclarativepixmapcache.moc" diff --git a/tests/auto/declarative/qquickimage/data/qtbug_22125.qml b/tests/auto/declarative/qquickimage/data/qtbug_22125.qml new file mode 100644 index 0000000000..9b68c0a125 --- /dev/null +++ b/tests/auto/declarative/qquickimage/data/qtbug_22125.qml @@ -0,0 +1,44 @@ +import QtQuick 2.0 + +Item { + id: root + width: 800 + height: 800 + + GridView { + anchors.fill: parent + delegate: Image { + source: imagePath; + asynchronous: true + smooth: true + width: 200 + height: 200 + } + model: ListModel { + ListElement { + imagePath: "http://127.0.0.1:14451/big256.png" + } + ListElement { + imagePath: "http://127.0.0.1:14451/big256.png" + } + ListElement { + imagePath: "http://127.0.0.1:14451/big256.png" + } + ListElement { + imagePath: "http://127.0.0.1:14451/colors.png" + } + ListElement { + imagePath: "http://127.0.0.1:14451/colors1.png" + } + ListElement { + imagePath: "http://127.0.0.1:14451/big.jpeg" + } + ListElement { + imagePath: "http://127.0.0.1:14451/heart.png" + } + ListElement { + imagePath: "http://127.0.0.1:14451/green.png" + } + } + } +} diff --git a/tests/auto/declarative/qquickimage/tst_qquickimage.cpp b/tests/auto/declarative/qquickimage/tst_qquickimage.cpp index 1dd88ca743..3223f0be1d 100644 --- a/tests/auto/declarative/qquickimage/tst_qquickimage.cpp +++ b/tests/auto/declarative/qquickimage/tst_qquickimage.cpp @@ -88,6 +88,7 @@ private slots: void sourceSize_QTBUG_14303(); void sourceSize_QTBUG_16389(); void nullPixmapPaint(); + void imageCrash_QTBUG_22125(); private: template @@ -653,6 +654,27 @@ void tst_qquickimage::nullPixmapPaint() delete image; } +void tst_qquickimage::imageCrash_QTBUG_22125() +{ + TestHTTPServer server(SERVER_PORT); + QVERIFY(server.isValid()); + server.serveDirectory(TESTDATA(""), TestHTTPServer::Delay); + + { + QQuickView view(QUrl::fromLocalFile(TESTDATA("qtbug_22125.qml"))); + view.show(); + qApp->processEvents(); + qApp->processEvents(); + // shouldn't crash when the view drops out of scope due to + // QDeclarativePixmapData attempting to dereference a pointer to + // the destroyed reader. + } + + // shouldn't crash when deleting cancelled QDeclarativePixmapReplys. + QTest::qWait(520); // Delay mode delays for 500 ms. + qApp->processEvents(QEventLoop::DeferredDeletion); +} + /* Find an item with the specified objectName. If index is supplied then the item must also evaluate the {index} expression equal to index -- cgit v1.2.3