diff options
author | Alex Wilson <alex.wilson@nokia.com> | 2012-03-06 14:00:36 +1000 |
---|---|---|
committer | Qt by Nokia <qt-info@nokia.com> | 2012-03-09 02:16:47 +0100 |
commit | 4c329881707317674e5672862f16e8143676c265 (patch) | |
tree | 0dc774424a987b01baad2969e41dcd3385055eda /src/location/maps | |
parent | bb78475bef0ea9f5cb7e3cb70826e5361320f060 (diff) | |
download | qtlocation-4c329881707317674e5672862f16e8143676c265.tar.gz |
Implementing deregisterMap to avoid crashes on slow requests
Previously, high-latency requests that were still running could complete
after deregisterMap() was called, and a pointer to the old QGeoMap
would still be present in the array -- which triggered a segfault. Now
we make sure to remove all references to a disappearing map.
This patch also handles a common destructor race causing similar segfaults
at exit (and in autotests)
Change-Id: I936d6c0d556d79fc43aa238b0b1d3dc63e5db11e
Reviewed-by: Aaron McCarthy <aaron.mccarthy@nokia.com>
Diffstat (limited to 'src/location/maps')
-rw-r--r-- | src/location/maps/qgeomap.cpp | 15 | ||||
-rw-r--r-- | src/location/maps/qgeomap_p_p.h | 3 | ||||
-rw-r--r-- | src/location/maps/qgeomappingmanager.cpp | 112 | ||||
-rw-r--r-- | src/location/maps/qgeomappingmanager.h | 3 |
4 files changed, 92 insertions, 41 deletions
diff --git a/src/location/maps/qgeomap.cpp b/src/location/maps/qgeomap.cpp index 0b937f98..d4a8e2e2 100644 --- a/src/location/maps/qgeomap.cpp +++ b/src/location/maps/qgeomap.cpp @@ -214,7 +214,6 @@ QGeoMapPrivate::QGeoMapPrivate(QGeoMap *parent, QGeoTileCache *cache) aspectRatio_(0.0), map_(parent), cache_(cache), - manager_(0), controller_(0), cameraTiles_(new QGeoCameraTiles()), mapGeometry_(new QGeoMapGeometry()), @@ -229,7 +228,8 @@ QGeoMapPrivate::~QGeoMapPrivate() delete mapGeometry_; delete cameraTiles_; - manager_->deregisterMap(map_); + if (manager_) + manager_.data()->deregisterMap(map_); // TODO map items are not deallocated! // However: how to ensure this is done in rendering thread? } @@ -241,6 +241,9 @@ QGeoTileCache* QGeoMapPrivate::tileCache() void QGeoMapPrivate::setMappingManager(QGeoMappingManager *manager) { + if (manager_) + manager_.data()->deregisterMap(map_); + if (manager) { manager->registerMap(map_); @@ -254,10 +257,8 @@ void QGeoMapPrivate::setMappingManager(QGeoMappingManager *manager) mapImages_ = new QGeoMapImages(map_); mapImages_->setMappingManager(manager); - - } else { - manager->deregisterMap(map_); } + manager_ = manager; } @@ -286,7 +287,7 @@ void QGeoMapPrivate::setCameraData(const QGeoCameraData &cameraData) } if (manager_) { - QGeoCameraCapabilities capabilities = manager_->cameraCapabilities(); + QGeoCameraCapabilities capabilities = manager_.data()->cameraCapabilities(); if (cameraData_.zoomLevel() < capabilities.minimumZoomLevel()) cameraData_.setZoomLevel(capabilities.minimumZoomLevel()); @@ -347,7 +348,7 @@ QGeoCameraData QGeoMapPrivate::cameraData() const QGeoMappingManager *QGeoMapPrivate::manager() const { - return manager_; + return manager_.data(); } void QGeoMapPrivate::resize(int width, int height) diff --git a/src/location/maps/qgeomap_p_p.h b/src/location/maps/qgeomap_p_p.h index c878697b..a2ed3496 100644 --- a/src/location/maps/qgeomap_p_p.h +++ b/src/location/maps/qgeomap_p_p.h @@ -61,6 +61,7 @@ #include <QMatrix4x4> #include <QString> #include <QSharedPointer> +#include <QWeakPointer> #include "qgeocameradata_p.h" #include "qgeomaptype.h" @@ -125,7 +126,7 @@ private: QGeoMap *map_; QGeoTileCache* cache_; - QGeoMappingManager *manager_; + QWeakPointer<QGeoMappingManager> manager_; QString pluginString_; QGeoMapController *controller_; diff --git a/src/location/maps/qgeomappingmanager.cpp b/src/location/maps/qgeomappingmanager.cpp index 90aff85d..13eba293 100644 --- a/src/location/maps/qgeomappingmanager.cpp +++ b/src/location/maps/qgeomappingmanager.cpp @@ -57,6 +57,9 @@ QT_BEGIN_NAMESPACE +#define Q_SHARED_D(type) \ + QSharedPointer< type > d = d_ptr; + /*! \class QGeoMappingManager \inmodule QtLocation @@ -129,7 +132,6 @@ QGeoMappingManager::QGeoMappingManager(QGeoMappingManagerEngine *engine, QObject */ QGeoMappingManager::~QGeoMappingManager() { - delete d_ptr; } /*! @@ -148,7 +150,9 @@ QGeoMappingManager::~QGeoMappingManager() */ QString QGeoMappingManager::managerName() const { - return d_ptr->engine->managerName(); + Q_SHARED_D(QGeoMappingManagerPrivate); + if (!d) return QString(); + return d->engine->managerName(); } /*! @@ -160,41 +164,68 @@ QString QGeoMappingManager::managerName() const */ int QGeoMappingManager::managerVersion() const { + Q_SHARED_D(QGeoMappingManagerPrivate); + if (!d) return 0; return d_ptr->engine->managerVersion(); } void QGeoMappingManager::registerMap(QGeoMap *map) { + Q_SHARED_D(QGeoMappingManagerPrivate); + if (!d) return; + QGeoTileCache *cache = map->tileCache(); - QSet<QGeoMap*> maps = d_ptr->caches.value(cache); + QSet<QGeoMap*> maps = d->caches.value(cache); maps.insert(map); - d_ptr->caches.insert(cache, maps); + d->caches.insert(cache, maps); } void QGeoMappingManager::deregisterMap(QGeoMap *map) { - Q_UNUSED(map); -// TileCache *cache = map->tileCache(); -// QSet<Map*> maps = d_ptr->caches.value(cache); -// maps.remove(map); -// if (maps.isEmpty()) { -// d_ptr->caches.remove(cache); -// } else { -// d_ptr->caches.insert(cache, maps); -// } + Q_SHARED_D(QGeoMappingManagerPrivate); + if (!d) return; + + QGeoTileCache *cache = map->tileCache(); + QSet<QGeoMap*> maps = d->caches.value(cache); + maps.remove(map); + if (maps.isEmpty()) { + d->caches.remove(cache); + } else { + d->caches.insert(cache, maps); + } // clear any tileHash / mapHash entries + d->mapHash.remove(map); + + QHash<QGeoTileSpec, QSet<QGeoMap*> > newTileHash = d->tileHash; + typedef QHash<QGeoTileSpec, QSet<QGeoMap*> >::const_iterator h_iter; + h_iter hi = d->tileHash.constBegin(); + h_iter hend = d->tileHash.constEnd(); + for (; hi != hend; ++hi) { + QSet<QGeoMap*> maps = hi.value(); + if (maps.contains(map)) { + maps.remove(map); + if (maps.isEmpty()) + newTileHash.remove(hi.key()); + else + newTileHash.insert(hi.key(), maps); + } + } + d->tileHash = newTileHash; } void QGeoMappingManager::updateTileRequests(QGeoMap *map, const QSet<QGeoTileSpec> &tilesAdded, const QSet<QGeoTileSpec> &tilesRemoved) { + Q_SHARED_D(QGeoMappingManagerPrivate); + if (!d) return; + typedef QSet<QGeoTileSpec>::const_iterator tile_iter; // add and remove tiles from tileset for this map - QSet<QGeoTileSpec> oldTiles = d_ptr->mapHash.value(map); + QSet<QGeoTileSpec> oldTiles = d->mapHash.value(map); tile_iter rem = tilesRemoved.constBegin(); tile_iter remEnd = tilesRemoved.constEnd(); @@ -208,7 +239,7 @@ void QGeoMappingManager::updateTileRequests(QGeoMap *map, oldTiles.insert(*add); } - d_ptr->mapHash.insert(map, oldTiles); + d->mapHash.insert(map, oldTiles); // add and remove map from mapset for the tiles @@ -217,29 +248,29 @@ void QGeoMappingManager::updateTileRequests(QGeoMap *map, rem = tilesRemoved.constBegin(); for (; rem != remEnd; ++rem) { - QSet<QGeoMap*> mapSet = d_ptr->tileHash.value(*rem); + QSet<QGeoMap*> mapSet = d->tileHash.value(*rem); mapSet.remove(map); if (mapSet.isEmpty()) { cancelTiles.insert(*rem); - d_ptr->tileHash.remove(*rem); + d->tileHash.remove(*rem); } else { - d_ptr->tileHash.insert(*rem, mapSet); + d->tileHash.insert(*rem, mapSet); } } add = tilesAdded.constBegin(); for (; add != addEnd; ++add) { - QSet<QGeoMap*> mapSet = d_ptr->tileHash.value(*add); + QSet<QGeoMap*> mapSet = d->tileHash.value(*add); if (mapSet.isEmpty()) { reqTiles.insert(*add); } mapSet.insert(map); - d_ptr->tileHash.insert(*add, mapSet); + d->tileHash.insert(*add, mapSet); } cancelTiles -= reqTiles; - QMetaObject::invokeMethod(d_ptr->engine, "updateTileRequests", + QMetaObject::invokeMethod(d->engine, "updateTileRequests", Qt::QueuedConnection, Q_ARG(QSet<QGeoTileSpec>, reqTiles), Q_ARG(QSet<QGeoTileSpec>, cancelTiles)); @@ -247,9 +278,12 @@ void QGeoMappingManager::updateTileRequests(QGeoMap *map, void QGeoMappingManager::engineTileFinished(const QGeoTileSpec &spec, const QByteArray &bytes, const QString &format) { + Q_SHARED_D(QGeoMappingManagerPrivate); + if (!d) return; + QSet<QGeoTileCache*> caches; - QSet<QGeoMap*> maps = d_ptr->tileHash.value(spec); + QSet<QGeoMap*> maps = d->tileHash.value(spec); typedef QSet<QGeoMap*>::const_iterator map_iter; @@ -258,22 +292,22 @@ void QGeoMappingManager::engineTileFinished(const QGeoTileSpec &spec, const QByt for (; map != mapEnd; ++map) { caches.insert((*map)->tileCache()); - QSet<QGeoTileSpec> tileSet = d_ptr->mapHash.value(*map); + QSet<QGeoTileSpec> tileSet = d->mapHash.value(*map); tileSet.remove(spec); if (tileSet.isEmpty()) - d_ptr->mapHash.remove(*map); + d->mapHash.remove(*map); else - d_ptr->mapHash.insert(*map, tileSet); + d->mapHash.insert(*map, tileSet); } - d_ptr->tileHash.remove(spec); + d->tileHash.remove(spec); typedef QSet<QGeoTileCache*>::const_iterator cache_iter; cache_iter cache = caches.constBegin(); cache_iter cacheEnd = caches.constEnd(); for (; cache != cacheEnd; ++cache) { - (*cache)->insert(spec, bytes, format, d_ptr->engine->cacheHint()); + (*cache)->insert(spec, bytes, format, d->engine->cacheHint()); } map = maps.constBegin(); @@ -289,7 +323,9 @@ void QGeoMappingManager::engineTileError(const QGeoTileSpec &spec, const QString QList<QGeoMapType> QGeoMappingManager::supportedMapTypes() const { - return d_ptr->engine->supportedMapTypes(); + Q_SHARED_D(QGeoMappingManagerPrivate); + if (!d) return QList<QGeoMapType>(); + return d->engine->supportedMapTypes(); } /*! @@ -299,7 +335,9 @@ QList<QGeoMapType> QGeoMappingManager::supportedMapTypes() const */ int QGeoMappingManager::tileSize() const { - return d_ptr->engine->tileSize(); + Q_SHARED_D(QGeoMappingManagerPrivate); + if (!d) return -1; + return d->engine->tileSize(); } /*! @@ -309,7 +347,9 @@ int QGeoMappingManager::tileSize() const */ bool QGeoMappingManager::isInitialized() const { - return d_ptr->engine->isInitialized(); + Q_SHARED_D(QGeoMappingManagerPrivate); + if (!d) return false; + return d->engine->isInitialized(); } /*! @@ -317,7 +357,9 @@ bool QGeoMappingManager::isInitialized() const */ QGeoCameraCapabilities QGeoMappingManager::cameraCapabilities() const { - return d_ptr->engine->cameraCapabilities(); + Q_SHARED_D(QGeoMappingManagerPrivate); + if (!d) return QGeoCameraCapabilities(); + return d->engine->cameraCapabilities(); } /*! @@ -330,7 +372,9 @@ QGeoCameraCapabilities QGeoMappingManager::cameraCapabilities() const */ void QGeoMappingManager::setLocale(const QLocale &locale) { - d_ptr->engine->setLocale(locale); + Q_SHARED_D(QGeoMappingManagerPrivate); + if (!d) return; + d->engine->setLocale(locale); } /*! @@ -339,11 +383,15 @@ void QGeoMappingManager::setLocale(const QLocale &locale) */ QLocale QGeoMappingManager::locale() const { + Q_SHARED_D(QGeoMappingManagerPrivate); + if (!d) return QLocale(); return d_ptr->engine->locale(); } QGeoMappingManager::CacheAreas QGeoMappingManager::cacheHint() const { + Q_SHARED_D(QGeoMappingManagerPrivate); + if (!d) return QGeoMappingManager::CacheAreas(); return d_ptr->engine->cacheHint(); } diff --git a/src/location/maps/qgeomappingmanager.h b/src/location/maps/qgeomappingmanager.h index fe6f74b3..aa877985 100644 --- a/src/location/maps/qgeomappingmanager.h +++ b/src/location/maps/qgeomappingmanager.h @@ -45,6 +45,7 @@ #include <QObject> #include <QSize> #include <QPair> +#include <QSharedPointer> #include <QtLocation/qlocationglobal.h> #include "qgeomaptype.h" @@ -113,7 +114,7 @@ Q_SIGNALS: private: QGeoMappingManager(QGeoMappingManagerEngine *engine, QObject *parent = 0); - QGeoMappingManagerPrivate* d_ptr; + QSharedPointer<QGeoMappingManagerPrivate> d_ptr; Q_DISABLE_COPY(QGeoMappingManager) friend class QGeoServiceProvider; |