Fix killing timer in different thread.

It was possible for another thread to attempt to stop the timer used by
the tile fetcher.

Change-Id: Ic44cf416bc7e6c987c4b5c9914a1c7d911dbbea5
Reviewed-by: Alex Blasche <alexander.blasche@digia.com>

5 files changed, 47 insertions, 69 deletions

diff --git a/src/location/maps/qgeotiledmappingmanagerengine.cpp b/src/location/maps/qgeotiledmappingmanagerengine.cpp
index 6b60a1ce..c0382b1a 100644
--- a/src/location/maps/qgeotiledmappingmanagerengine.cpp
+++ b/src/location/maps/qgeotiledmappingmanagerengine.cpp
@@ -327,9 +327,6 @@ QGeoTiledMappingManagerEnginePrivate::~QGeoTiledMappingManagerEnginePrivate()
 {
     delete tileCache_;
 
-    // but we still want to stop the fetcher's timer immediately
-    fetcher_->stopTimer();
-
     // will delete fetcher and thread later
     thread_->exit();
 }
diff --git a/src/location/maps/qgeotilefetcher.cpp b/src/location/maps/qgeotilefetcher.cpp
index 450bf322..a86c0a43 100644
--- a/src/location/maps/qgeotilefetcher.cpp
+++ b/src/location/maps/qgeotilefetcher.cpp
@@ -39,6 +39,8 @@
 **
 ****************************************************************************/
 
+#include <QtCore/QTimerEvent>
+
 #include "qgeomappingmanagerengine_p.h"
 #include "qgeotilefetcher_p.h"
 #include "qgeotilefetcher_p_p.h"
@@ -66,21 +68,10 @@ void QGeoTileFetcher::threadStarted()
 {
     Q_D(QGeoTileFetcher);
 
-    if (d->stopped_)
-        return;
-
-    d->timer_ = new QTimer(this);
-
-    d->timer_->setInterval(0);
-
-    connect(d->timer_,
-            SIGNAL(timeout()),
-            this,
-            SLOT(requestNextTile()));
+    d->enabled_ = true;
 
-    d->started_ = true;
     if (!d->queue_.isEmpty())
-        d->timer_->start();
+        d->timer_.start(0, this);
 }
 
 bool QGeoTileFetcher::init()
@@ -88,19 +79,12 @@ bool QGeoTileFetcher::init()
     return false;
 }
 
-void QGeoTileFetcher::stopTimer()
+void QGeoTileFetcher::threadFinished()
 {
     Q_D(QGeoTileFetcher);
-    d->stopped_ = true;
-    if (d->timer_) {
-        disconnect(d->timer_);
-        d->timer_->stop();
-    }
-}
 
-void QGeoTileFetcher::threadFinished()
-{
-    stopTimer();
+    d->enabled_ = false;
+    d->timer_.stop();
     this->deleteLater();
 }
 
@@ -111,15 +95,12 @@ void QGeoTileFetcher::updateTileRequests(const QSet<QGeoTileSpec> &tilesAdded,
 
     QMutexLocker ml(&d->queueMutex_);
 
-    if (d->stopped_)
-        return;
-
     cancelTileRequests(tilesRemoved);
 
     d->queue_ += tilesAdded.toList();
 
-    if (!d->queue_.empty())
-        d->timer_->start();
+    if (d->enabled_ && !d->queue_.isEmpty() && !d->timer_.isActive())
+        d->timer_.start(0, this);
 }
 
 void QGeoTileFetcher::cancelTileRequests(const QSet<QGeoTileSpec> &tiles)
@@ -139,9 +120,6 @@ void QGeoTileFetcher::cancelTileRequests(const QSet<QGeoTileSpec> &tiles)
         }
         d->queue_.removeAll(*tile);
     }
-
-    if (d->queue_.isEmpty())
-        d->timer_->stop();
 }
 
 void QGeoTileFetcher::requestNextTile()
@@ -150,13 +128,11 @@ void QGeoTileFetcher::requestNextTile()
 
     QMutexLocker ml(&d->queueMutex_);
 
-    if (d->stopped_)
+    if (!d->enabled_)
         return;
 
-    if (d->queue_.isEmpty()) {
-        d->timer_->stop();
+    if (d->queue_.isEmpty())
         return;
-    }
 
     QGeoTileSpec ts = d->queue_.takeFirst();
 
@@ -175,7 +151,7 @@ void QGeoTileFetcher::requestNextTile()
     }
 
     if (d->queue_.isEmpty())
-        d->timer_->stop();
+        d->timer_.stop();
 }
 
 void QGeoTileFetcher::finished()
@@ -200,11 +176,27 @@ void QGeoTileFetcher::finished()
     handleReply(reply, spec);
 }
 
+void QGeoTileFetcher::timerEvent(QTimerEvent *event)
+{
+    Q_D(QGeoTileFetcher);
+    if (event->timerId() != d->timer_.timerId()) {
+        QObject::timerEvent(event);
+        return;
+    }
+
+    if (d->queue_.isEmpty()) {
+        d->timer_.stop();
+        return;
+    }
+
+    requestNextTile();
+}
+
 void QGeoTileFetcher::handleReply(QGeoTiledMapReply *reply, const QGeoTileSpec &spec)
 {
     Q_D(QGeoTileFetcher);
 
-    if (d->stopped_) {
+    if (!d->enabled_) {
         reply->deleteLater();
         return;
     }
@@ -222,21 +214,12 @@ void QGeoTileFetcher::handleReply(QGeoTiledMapReply *reply, const QGeoTileSpec &
 *******************************************************************************/
 
 QGeoTileFetcherPrivate::QGeoTileFetcherPrivate(QGeoTiledMappingManagerEngine *engine)
-    : engine_(engine),
-      started_(false),
-      stopped_(false),
-      timer_(0) {}
+:   engine_(engine), enabled_(false)
+{
+}
 
 QGeoTileFetcherPrivate::~QGeoTileFetcherPrivate()
 {
 }
 
 QT_END_NAMESPACE
-
-
-
-
-
-
-
-
diff --git a/src/location/maps/qgeotilefetcher_p.h b/src/location/maps/qgeotilefetcher_p.h
index 45e38e05..d08092b9 100644
--- a/src/location/maps/qgeotilefetcher_p.h
+++ b/src/location/maps/qgeotilefetcher_p.h
@@ -74,7 +74,6 @@ class Q_LOCATION_EXPORT QGeoTileFetcher : public QObject
 public:
     QGeoTileFetcher(QGeoTiledMappingManagerEngine *engine, QObject *parent = 0);
     virtual ~QGeoTileFetcher();
-    void stopTimer();
 
 public Q_SLOTS:
     void threadStarted();
@@ -91,6 +90,7 @@ Q_SIGNALS:
     void tileError(const QGeoTileSpec &spec, const QString &errorString);
 
 protected:
+    void timerEvent(QTimerEvent *event);
     virtual bool init();
     QGeoTiledMappingManagerEngine::CacheAreas cacheHint() const;
 
diff --git a/src/location/maps/qgeotilefetcher_p_p.h b/src/location/maps/qgeotilefetcher_p_p.h
index ba71a750..c47f8f90 100644
--- a/src/location/maps/qgeotilefetcher_p_p.h
+++ b/src/location/maps/qgeotilefetcher_p_p.h
@@ -78,9 +78,8 @@ public:
 
     QGeoTiledMappingManagerEngine *engine_;
 
-    bool started_;
-    bool stopped_;
-    QTimer *timer_;
+    bool enabled_;
+    QBasicTimer timer_;
     QMutex queueMutex_;
     QList<QGeoTileSpec> queue_;
     QHash<QGeoTileSpec, QGeoTiledMapReply *> invmap_;
diff --git a/tests/auto/geotestplugin/qgeotilefetcher_test.h b/tests/auto/geotestplugin/qgeotilefetcher_test.h
index 2f2754cf..9c939b9d 100644
--- a/tests/auto/geotestplugin/qgeotilefetcher_test.h
+++ b/tests/auto/geotestplugin/qgeotilefetcher_test.h
@@ -82,7 +82,6 @@ public:
         : QGeoTileFetcher(engine, parent),
           finishRequestImmediately_(false),
           mappingReply_(0),
-          timerId_(0),
           errorCode_(QGeoTiledMapReply::NoError) {}
 
     bool init()
@@ -125,7 +124,8 @@ public:
 
         mappingReply_->callSetMapImageData(bytes);
         mappingReply_->callSetMapImageFormat("png");
-        mappingReply_->callSetFinished(true);
+
+        timer_.start(500, this);
 
         return mappingReply_;
     }
@@ -143,35 +143,34 @@ public:
 public Q_SLOTS:
     void requestAborted()
     {
-        if (timerId_) {
-            killTimer(timerId_);
-            timerId_ = 0;
-        }
-        errorString_ = "";
+        timer_.stop();
+        errorString_.clear();
         errorCode_ = QGeoTiledMapReply::NoError;
     }
 
 protected:
      void timerEvent(QTimerEvent *event)
      {
-         Q_ASSERT(timerId_ == event->timerId());
+         if (event->timerId() != timer_.timerId()) {
+             QGeoTileFetcher::timerEvent(event);
+             return;
+         }
+
          Q_ASSERT(mappingReply_);
-         killTimer(timerId_);
-         timerId_ = 0;
+         timer_.stop();
          if (errorCode_) {
              mappingReply_->callSetError(errorCode_, errorString_);
              emit tileError(mappingReply_->tileSpec(), errorString_);
-        } else {
+         } else {
              mappingReply_->callSetError(QGeoTiledMapReply::NoError, "no error");
              mappingReply_->callSetFinished(true);
          }
-         // emit finished(mappingReply_); todo tileFinished
      }
 
 private:
     bool finishRequestImmediately_;
     TiledMapReplyTest* mappingReply_;
-    int timerId_;
+    QBasicTimer timer_;
     QGeoTiledMapReply::Error errorCode_;
     QString errorString_;
     QVariantMap parameters_;