Correct destruction of mappingManagerEngine

Use of weakPointer to prevent accessing deleted memory. Plus thread exiting to
cause the fetcher and thread to destroy themselves.
Additional use of weak pointers to deal with the complexity of the mapping manager
destructing before the declarative objects.

Task-number: QTBUG-25797
Change-Id: I6a27568580c1a00f7588565ff7e35d63eb5dd785
Reviewed-by: Aaron McCarthy <aaron.mccarthy@nokia.com>

11 files changed, 52 insertions, 31 deletions

diff --git a/src/imports/location/qdeclarativegeomap_p.h b/src/imports/location/qdeclarativegeomap_p.h
index 2fdc25f9..47ddbdd4 100644
--- a/src/imports/location/qdeclarativegeomap_p.h
+++ b/src/imports/location/qdeclarativegeomap_p.h
@@ -242,7 +242,6 @@ private:
     QQuickCanvas *canvas_;
     int touchTimer_;
 
-    QGeoTileCache *tileCache_;
     QGeoMap *map_;
     QWeakPointer<QDeclarativeGeoMapCopyrightNotice> copyrightsWPtr_;
 
diff --git a/src/location/maps/qgeomappingmanager.cpp b/src/location/maps/qgeomappingmanager.cpp
index 3b74b68d..2856344e 100644
--- a/src/location/maps/qgeomappingmanager.cpp
+++ b/src/location/maps/qgeomappingmanager.cpp
@@ -187,6 +187,8 @@ QGeoMappingManagerPrivate::QGeoMappingManagerPrivate()
 
 QGeoMappingManagerPrivate::~QGeoMappingManagerPrivate()
 {
+    delete engine;
+    engine = 0;
 }
 
 #include "moc_qgeomappingmanager.cpp"
diff --git a/src/location/maps/qgeotilecache.cpp b/src/location/maps/qgeotilecache.cpp
index 671675ba..608110be 100644
--- a/src/location/maps/qgeotilecache.cpp
+++ b/src/location/maps/qgeotilecache.cpp
@@ -55,6 +55,8 @@ Q_DECLARE_METATYPE(QList<QGeoTileSpec>)
 Q_DECLARE_METATYPE(QSet<QGeoTileSpec>)
 
 QT_BEGIN_NAMESPACE
+QMutex QGeoTileCache::cleanupMutex_;
+QList<QGLTexture2D*> QGeoTileCache::cleanupList_;
 
 class QGeoCachedTileMemory
 {
@@ -73,7 +75,6 @@ public:
 
 QGeoTileTexture::QGeoTileTexture()
     : texture(0),
-      cache(0),
       textureBound(false) {}
 
 void QCache3QTileEvictionPolicy::aboutToBeRemoved(const QGeoTileSpec &key, QSharedPointer<QGeoCachedTileDisk> obj)
@@ -98,8 +99,7 @@ QGeoCachedTileDisk::~QGeoCachedTileDisk()
 
 QGeoTileTexture::~QGeoTileTexture()
 {
-    if (cache)
-        cache->evictFromTextureCache(this);
+    QGeoTileCache::evictFromTextureCache(this);
 }
 
 QGeoTileCache::QGeoTileCache(const QString &directory, QObject *parent)
@@ -130,7 +130,12 @@ QGeoTileCache::QGeoTileCache(const QString &directory, QObject *parent)
     loadTiles();
 }
 
-QGeoTileCache::~QGeoTileCache() {}
+QGeoTileCache::~QGeoTileCache()
+{
+    textureCache_.clear();
+    memoryCache_.clear();
+    diskCache_.clear();
+}
 
 void QGeoTileCache::printStats()
 {
@@ -308,7 +313,6 @@ QSharedPointer<QGeoCachedTileDisk> QGeoTileCache::addToDiskCache(const QGeoTileS
     QFileInfo fi(filename);
     int diskCost = fi.size();
     diskCache_.insert(spec, td, diskCost);
-
     return td;
 }
 
@@ -334,7 +338,6 @@ QSharedPointer<QGeoTileTexture> QGeoTileCache::addToTextureCache(const QGeoTileS
     tt->texture->setPixmap(pixmap);
     tt->texture->setHorizontalWrap(QGL::ClampToEdge);
     tt->texture->setVerticalWrap(QGL::ClampToEdge);
-    tt->cache = this;
 
     /* Do not bind/cleanImage on the texture here -- it needs to be done
      * in the render thread (by qgeomapscene) */
diff --git a/src/location/maps/qgeotilecache_p.h b/src/location/maps/qgeotilecache_p.h
index d7a900de..774f0a0b 100644
--- a/src/location/maps/qgeotilecache_p.h
+++ b/src/location/maps/qgeotilecache_p.h
@@ -99,7 +99,6 @@ public:
 
     QGeoTileSpec spec;
     QGLTexture2D *texture;
-    QGeoTileCache *cache;
     bool textureBound;
 };
 
@@ -137,9 +136,10 @@ public:
 
     QSharedPointer<QGeoTileTexture> get(const QGeoTileSpec &spec);
 
-    void evictFromDiskCache(QGeoCachedTileDisk *td);
-    void evictFromMemoryCache(QGeoCachedTileMemory *tm);
-    void evictFromTextureCache(QGeoTileTexture *tt);
+    // can be called without a specific tileCache pointer
+    static void evictFromDiskCache(QGeoCachedTileDisk *td);
+    static void evictFromMemoryCache(QGeoCachedTileMemory *tm);
+    static void evictFromTextureCache(QGeoTileTexture *tt);
 
     void insert(const QGeoTileSpec &spec,
                 const QByteArray &bytes,
@@ -168,8 +168,8 @@ private:
     int minTextureUsage_;
     int extraTextureUsage_;
 
-    QMutex cleanupMutex_;
-    QList<QGLTexture2D *> cleanupList_;
+    static QMutex cleanupMutex_;
+    static QList<QGLTexture2D*> cleanupList_;
 };
 
 QT_END_NAMESPACE
diff --git a/src/location/maps/qgeotiledmapdata.cpp b/src/location/maps/qgeotiledmapdata.cpp
index c1a6bdff..4e2911d2 100644
--- a/src/location/maps/qgeotiledmapdata.cpp
+++ b/src/location/maps/qgeotiledmapdata.cpp
@@ -140,7 +140,8 @@ QGeoTiledMapData::QGeoTiledMapData(QGeoTiledMappingManagerEngine *engine, QObjec
 
 QGeoTiledMapData::~QGeoTiledMapData()
 {
-    d_ptr->engine()->deregisterMap(this);
+    if (d_ptr->engine()) // check if engine hasn't already been deleted
+        d_ptr->engine().data()->deregisterMap(this);
     delete d_ptr;
 }
 QGeoTileRequestManager *QGeoTiledMapData::getRequestManager()
@@ -264,7 +265,7 @@ QGeoTileCache *QGeoTiledMapDataPrivate::tileCache()
     return cache_;
 }
 
-QGeoTiledMappingManagerEngine *QGeoTiledMapDataPrivate::engine() const
+QWeakPointer<QGeoTiledMappingManagerEngine> QGeoTiledMapDataPrivate::engine() const
 {
     return engine_;
 }
@@ -353,8 +354,11 @@ void QGeoTiledMapDataPrivate::newTileFetched(const QGeoTileSpec &spec)
 {
     // Only promote the texture up to GPU if it is visible
     if (cameraTiles_->tiles().contains(spec)){
-        mapScene_->addTile(spec, engine_->getTileTexture(spec));
-        map_->update();
+        QSharedPointer<QGeoTileTexture> tex = engine_.data()->getTileTexture(spec);
+        if (tex) {
+            mapScene_->addTile(spec, tex);
+            map_->update();
+        }
     }
 }
 
diff --git a/src/location/maps/qgeotiledmapdata_p_p.h b/src/location/maps/qgeotiledmapdata_p_p.h
index 79af86fb..a235e2b0 100644
--- a/src/location/maps/qgeotiledmapdata_p_p.h
+++ b/src/location/maps/qgeotiledmapdata_p_p.h
@@ -104,13 +104,13 @@ public:
     void newTileFetched(const QGeoTileSpec &spec);
     QSet<QGeoTileSpec> visibleTiles();
 
-    QGeoTiledMappingManagerEngine *engine() const;
     void prefetchTiles();
+    QWeakPointer<QGeoTiledMappingManagerEngine> engine() const;
 
 private:
     QGeoTiledMapData *map_;
     QGeoTileCache *cache_;
-    QGeoTiledMappingManagerEngine *engine_;
+    QWeakPointer<QGeoTiledMappingManagerEngine> engine_;
 
     QGeoCameraTiles *cameraTiles_;
     QGeoMapScene *mapScene_;
diff --git a/src/location/maps/qgeotiledmappingmanagerengine.cpp b/src/location/maps/qgeotiledmappingmanagerengine.cpp
index 3dafc302..b8c12e1b 100644
--- a/src/location/maps/qgeotiledmappingmanagerengine.cpp
+++ b/src/location/maps/qgeotiledmappingmanagerengine.cpp
@@ -329,9 +329,12 @@ QGeoTiledMappingManagerEnginePrivate::QGeoTiledMappingManagerEnginePrivate()
 QGeoTiledMappingManagerEnginePrivate::~QGeoTiledMappingManagerEnginePrivate()
 {
     delete tileCache_;
-    delete fetcher_;
 
-    thread_->quit();
+    // will delete fetcher and thread later
+    thread_->exit();
+
+    // but we still want to stop the fetcher's timer immediately
+    fetcher_->stopTimer();
 }
 #include "moc_qgeotiledmappingmanagerengine.cpp"
 
diff --git a/src/location/maps/qgeotilefetcher.cpp b/src/location/maps/qgeotilefetcher.cpp
index 79b838bb..65853a65 100644
--- a/src/location/maps/qgeotilefetcher.cpp
+++ b/src/location/maps/qgeotilefetcher.cpp
@@ -91,12 +91,19 @@ bool QGeoTileFetcher::init()
     return false;
 }
 
-void QGeoTileFetcher::threadFinished()
+void QGeoTileFetcher::stopTimer()
 {
     Q_D(QGeoTileFetcher);
     d->stopped_ = true;
-    disconnect(d->timer_);
-    d->timer_->stop();
+    if (d->timer_) {
+        disconnect(d->timer_);
+        d->timer_->stop();
+    }
+}
+
+void QGeoTileFetcher::threadFinished()
+{
+    stopTimer();
     this->deleteLater();
 }
 
diff --git a/src/location/maps/qgeotilefetcher.h b/src/location/maps/qgeotilefetcher.h
index e40f21ef..d87bd4e3 100644
--- a/src/location/maps/qgeotilefetcher.h
+++ b/src/location/maps/qgeotilefetcher.h
@@ -65,6 +65,7 @@ class Q_LOCATION_EXPORT QGeoTileFetcher : public QObject
 public:
     QGeoTileFetcher(QGeoTiledMappingManagerEngine *engine, QObject *parent = 0);
     virtual ~QGeoTileFetcher();
+    void stopTimer();
 
 public Q_SLOTS:
     void threadStarted();
diff --git a/src/plugins/geoservices/nokia/qgeotilefetcher_nokia.cpp b/src/plugins/geoservices/nokia/qgeotilefetcher_nokia.cpp
index 3f407e64..f90b1c4a 100644
--- a/src/plugins/geoservices/nokia/qgeotilefetcher_nokia.cpp
+++ b/src/plugins/geoservices/nokia/qgeotilefetcher_nokia.cpp
@@ -196,7 +196,7 @@ QString QGeoTileFetcherNokia::getRequestString(const QGeoTileSpec &spec)
 
 QString QGeoTileFetcherNokia::getLanguageString() const
 {
-    const QLocale::Language lang = m_engineNokia->locale().language();
+    const QLocale::Language lang = m_engineNokia.data()->locale().language();
     // English is the default, where no ln is specified. We hardcode the languages
     // here even though the entire list is updated automagically from the server.
     // The current languages are Arabic, Chinese, Simplified Chinese, English
@@ -207,7 +207,7 @@ QString QGeoTileFetcherNokia::getLanguageString() const
     case QLocale::Arabic:
         return "ARA";
     case QLocale::Chinese:
-        if (QLocale::TraditionalChineseScript == m_engineNokia->locale().script())
+        if (QLocale::TraditionalChineseScript == m_engineNokia.data()->locale().script())
             return "CHI";
         else
             return "CHT";
@@ -241,10 +241,12 @@ QString QGeoTileFetcherNokia::applicationId() const
 
 void QGeoTileFetcherNokia::copyrightsFetched()
 {
-    QMetaObject::invokeMethod(m_engineNokia,
-                              "loadCopyrightsDescriptorsFromJson",
-                              Qt::QueuedConnection,
-                              Q_ARG(QByteArray, m_copyrightsReply->readAll()));
+    if (m_engineNokia) {
+        QMetaObject::invokeMethod(m_engineNokia.data(),
+                                  "loadCopyrightsDescriptorsFromJson",
+                                  Qt::QueuedConnection,
+                                  Q_ARG(QByteArray, m_copyrightsReply->readAll()));
+    }
 }
 
 void QGeoTileFetcherNokia::fetchCopyrightsData()
diff --git a/src/plugins/geoservices/nokia/qgeotilefetcher_nokia.h b/src/plugins/geoservices/nokia/qgeotilefetcher_nokia.h
index 610dc147..eb5f02b7 100644
--- a/src/plugins/geoservices/nokia/qgeotilefetcher_nokia.h
+++ b/src/plugins/geoservices/nokia/qgeotilefetcher_nokia.h
@@ -93,7 +93,7 @@ private:
 
     QString getLanguageString() const;
 
-    QGeoTiledMappingManagerEngineNokia *m_engineNokia;
+    QWeakPointer<QGeoTiledMappingManagerEngineNokia> m_engineNokia;
     QGeoNetworkAccessManager *m_networkManager;
     QMap<QString, QVariant> m_parameters;
     QSize m_tileSize;