Skip to content
Permalink
Browse files

Safety checks for unregistering map layers from registry

If a map layer which is registered is deleted outside of the layer
registry but not unregistered, the layer registry would still happily
return a pointer to this layer if queried with its id.

Up to now, this caused crashes. Now, the layer will be unregistered and
a warning is printed.

This patch also contains slight improvements to other parts of the map
layer registry.
  • Loading branch information
m-kuhn committed Jul 3, 2016
1 parent 6103669 commit f3bfef4066b8b7af6a7f252fbc702c239aeb09f5
Showing with 26 additions and 14 deletions.
  1. +23 −13 src/core/qgsmaplayerregistry.cpp
  2. +3 −1 src/core/qgsmaplayerregistry.h
@@ -73,12 +73,11 @@ QList<QgsMapLayer *> QgsMapLayerRegistry::addMapLayers(
bool takeOwnership )
{
QList<QgsMapLayer *> myResultList;
for ( int i = 0; i < theMapLayers.size(); ++i )
Q_FOREACH ( QgsMapLayer* myLayer, theMapLayers )
{
QgsMapLayer * myLayer = theMapLayers.at( i );
if ( !myLayer || !myLayer->isValid() )
{
QgsDebugMsg( "cannot add invalid layers" );
QgsDebugMsg( "Cannot add invalid layers" );
continue;
}
//check the layer is not already registered!
@@ -87,7 +86,10 @@ QList<QgsMapLayer *> QgsMapLayerRegistry::addMapLayers(
mMapLayers[myLayer->id()] = myLayer;
myResultList << mMapLayers[myLayer->id()];
if ( takeOwnership )
mOwnedLayers << myLayer;
{
myLayer->setParent( this );
}
connect( myLayer, SIGNAL( destroyed( QObject* ) ), this, SLOT( onMapLayerDeleted( QObject* ) ) );
emit layerWasAdded( myLayer );
}
}
@@ -149,10 +151,9 @@ void QgsMapLayerRegistry::removeMapLayers( const QList<QgsMapLayer*>& layers )
QString myId( lyr->id() );
emit layerWillBeRemoved( myId );
emit layerWillBeRemoved( lyr );
if ( mOwnedLayers.contains( lyr ) )
if ( lyr->parent() == this )
{
delete lyr;
mOwnedLayers.remove( lyr );
}
mMapLayers.remove( myId );
emit layerRemoved( myId );
@@ -183,14 +184,23 @@ void QgsMapLayerRegistry::removeAllMapLayers()

void QgsMapLayerRegistry::reloadAllLayers()
{
QMap<QString, QgsMapLayer *>::iterator it;
for ( it = mMapLayers.begin(); it != mMapLayers.end() ; ++it )
Q_FOREACH ( QgsMapLayer* layer, mMapLayers )
{
QgsMapLayer* layer = it.value();
if ( layer )
{
layer->reload();
}
layer->reload();
}
}

void QgsMapLayerRegistry::onMapLayerDeleted( QObject* obj )
{
QgsMapLayer* ml = qobject_cast<QgsMapLayer*>( obj );
Q_ASSERT( ml );

QString id = mMapLayers.key( ml );

if ( !id.isNull() )
{
QgsDebugMsg( QString( "Map layer deleted without unregistering! %1" ).arg( id ) );
mMapLayers.remove( id );
}
}

@@ -309,12 +309,14 @@ class CORE_EXPORT QgsMapLayerRegistry : public QObject
void connectNotify( const char * signal ) override;
#endif

private slots:
void onMapLayerDeleted( QObject* obj );

private:
//! private singleton constructor
QgsMapLayerRegistry( QObject * parent = nullptr );

QMap<QString, QgsMapLayer*> mMapLayers;
QSet<QgsMapLayer*> mOwnedLayers;
}; // class QgsMapLayerRegistry

#endif //QgsMapLayerRegistry_H

7 comments on commit f3bfef4

@nyalldawson

This comment has been minimized.

Copy link
Collaborator

@nyalldawson nyalldawson replied Jul 3, 2016

@m-kuhn nice fixes! it was actually on my agenda for this morning to start building up the map registry unit tests, so very timely :)

@m-kuhn

This comment has been minimized.

Copy link
Member Author

@m-kuhn m-kuhn replied Jul 3, 2016

Hehe :)

Just wondering, I was working on a QReadWriteLock for QgsMapLayerRegistry. In the end it turned out the real culprit was layers being deleted outside the registry (so what this commit relates to).

In the end, the map layer registry is still accessed from different threads, so it may also be good to harden it for multithreading. I think QMap itself is not threadsafe. So far I couldn't definitely relate any crash to this but OTOH if a crash relates to this it's probably a hard-to-reproduce one.

@nyalldawson

This comment has been minimized.

Copy link
Collaborator

@nyalldawson nyalldawson replied Jul 3, 2016

In the end, the map layer registry is still accessed from different threads, so it may also be good to harden it for multithreading. I think QMap itself is not threadsafe. So far I couldn't definitely relate any crash to this but OTOH if a crash relates to this it's probably a hard-to-reproduce one.

I've been wondering the exact same thing for QgsCRSCache and QgsCoordinateTransformCache

@nyalldawson

This comment has been minimized.

Copy link
Collaborator

@nyalldawson nyalldawson replied Jul 3, 2016

@m-kuhn btw, i see a bunch of crashes on my qt5 build following this commit

@nyalldawson

This comment has been minimized.

Copy link
Collaborator

@nyalldawson nyalldawson replied Jul 3, 2016

actually- ignore that. i missed the follow up.

@m-kuhn

This comment has been minimized.

Copy link
Member Author

@m-kuhn m-kuhn replied Jul 3, 2016

Followup commit as well?

@m-kuhn

This comment has been minimized.

Copy link
Member Author

@m-kuhn m-kuhn replied Jul 3, 2016

Ah :)

Please sign in to comment.
You can’t perform that action at this time.