Properly protect access to pixmap reader thread with mutex

Previously, access to the data from 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), or in the case where a
QDeclarativePixmapData was deleted after its QDeclarativePixmapReply
was removed from the jobs queue but prior to processing.

Reviewed-by: Martin Jones
Task-number: QTBUG-22125

1 files changed, 41 insertions, 16 deletions

diff --git a/src/declarative/util/qdeclarativepixmapcache.cpp b/src/declarative/util/qdeclarativepixmapcache.cpp
index 3557425..b23ac73 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;
@@ -147,6 +148,7 @@ public:
     void cancel(QDeclarativePixmapReply *rep);
 
     static QDeclarativePixmapReader *instance(QDeclarativeEngine *engine);
+    static QDeclarativePixmapReader *existingInstance(QDeclarativeEngine *engine);
 
 protected:
     void run();
@@ -176,6 +178,7 @@ private:
     static int downloadProgress;
     static int threadNetworkRequestDone;
     static QHash<QDeclarativeEngine *,QDeclarativePixmapReader*> readers;
+public:
     static QMutex readerMutex;
 };
 
@@ -326,6 +329,22 @@ QDeclarativePixmapReader::~QDeclarativePixmapReader()
     readers.remove(engine);
     readerMutex.unlock();
 
+    mutex.lock();
+    // manually cancel all outstanding jobs.
+    foreach (QDeclarativePixmapReply *reply, jobs) {
+        delete reply;
+    }
+    jobs.clear();
+    QList<QDeclarativePixmapReply*> 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();
 }
@@ -433,9 +452,8 @@ void QDeclarativePixmapReader::processJobs()
         if (!jobs.isEmpty() && replies.count() < IMAGEREQUEST_MAX_REQUEST_COUNT) {
             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();
@@ -459,7 +477,6 @@ void QDeclarativePixmapReader::processJob(QDeclarativePixmapReply *runningJob, c
             errorCode = QDeclarativePixmapReply::Loading;
             errorStr = QDeclarativePixmap::tr("Failed to get image from provider: %1").arg(url.toString());
         }
-
         mutex.lock();
         if (!cancelled.contains(runningJob)) runningJob->postReply(errorCode, errorStr, readSize, image);
         mutex.unlock();
@@ -487,10 +504,8 @@ void QDeclarativePixmapReader::processJob(QDeclarativePixmapReply *runningJob, c
             QNetworkRequest req(url);
             req.setAttribute(QNetworkRequest::HttpPipeliningAllowedAttribute, true);
             QNetworkReply *reply = networkAccessManager()->get(req);
-
             QMetaObject::connect(reply, replyDownloadProgress, runningJob, downloadProgress);
             QMetaObject::connect(reply, replyFinished, threadObject, threadNetworkRequestDone);
-
             replies.insert(reply, runningJob);
         }
     }
@@ -498,22 +513,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();
@@ -692,7 +712,7 @@ void QDeclarativePixmapStore::flushCache()
 }
 
 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()");
@@ -750,8 +770,14 @@ void QDeclarativePixmapData::release()
 
     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) {
@@ -1013,13 +1039,12 @@ 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();