Merge branch 'master' of scm.dev.nokia.troll.no:qt/qt-earth-staging into master-integration

* 'master' of scm.dev.nokia.troll.no:qt/qt-earth-staging:
  Properly protect access to pixmap reader thread with mutex

3 files changed, 126 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();
diff --git a/tests/auto/declarative/qdeclarativeimage/data/qtbug_22125.qml b/tests/auto/declarative/qdeclarativeimage/data/qtbug_22125.qml
new file mode 100644
index 0000000..8588028
--- /dev/null
+++ b/tests/auto/declarative/qdeclarativeimage/data/qtbug_22125.qml
@@ -0,0 +1,44 @@
+import QtQuick 1.1
+
+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/qdeclarativeimage/tst_qdeclarativeimage.cpp b/tests/auto/declarative/qdeclarativeimage/tst_qdeclarativeimage.cpp
index a35d69a..f67c5b5 100644
--- a/tests/auto/declarative/qdeclarativeimage/tst_qdeclarativeimage.cpp
+++ b/tests/auto/declarative/qdeclarativeimage/tst_qdeclarativeimage.cpp
@@ -94,6 +94,7 @@ private slots:
     void resetSourceSize();
     void testQtQuick11Attributes();
     void testQtQuick11Attributes_data();
+    void readerCrash_QTBUG_22125();
 
 private:
     template<typename T>
@@ -762,6 +763,46 @@ void tst_qdeclarativeimage::testQtQuick11Attributes_data()
         << ":1 \"Image.cache\" is not available in QtQuick 1.0.\n";
 }
 
+void tst_qdeclarativeimage::readerCrash_QTBUG_22125()
+{
+    {
+        TestHTTPServer server(SERVER_PORT);
+        QVERIFY(server.isValid());
+        server.serveDirectory(SRCDIR "/data/", TestHTTPServer::Delay);
+
+        {
+            QDeclarativeView view(QUrl::fromLocalFile(SRCDIR "/data/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(1000);
+        qApp->processEvents(QEventLoop::DeferredDeletion);
+    }
+
+    {
+        TestHTTPServer server(SERVER_PORT);
+        QVERIFY(server.isValid());
+        server.serveDirectory(SRCDIR "/data/");
+
+        {
+            QDeclarativeView view(QUrl::fromLocalFile(SRCDIR "/data/qtbug_22125.qml"));
+            view.show();
+            qApp->processEvents();
+            QTest::qWait(1000);
+            qApp->processEvents();
+            // shouldn't crash when the view drops out of scope due to
+            // the reader thread accessing self-deleted QDeclarativePixmapReplys.
+        }
+        qApp->processEvents();
+    }
+}
+
 /*
    Find an item with the specified objectName.  If index is supplied then the
    item must also evaluate the {index} expression equal to index