Fix map layer memory leak with bad layer handling

elpaso · elpaso · commit bd9b3736d6f8 · 2019-03-26T17:05:53.000+01:00
Long story: if the store contains a layer with the same ID
it silently fails to add it to the store.

This is the cause of leak in case client code calls
store-&gt;addLayers() within the (iterative) process of fixing
bad layers by patching the XML and re-reading it.

The proposed solution is to check in QgsProject::addLayer()
- which creates the layer object - if the newly created layer
was already in the store and delete it at the end of the
layer reading process if it was.

Deleting the layer from within the store-&gt;addLayers is
extremely dangerous, because the caller could keep a reference
to the layer and use it.
diff --git a/src/core/qgsproject.cpp b/src/core/qgsproject.cpp
@@ -944,30 +944,30 @@ bool QgsProject::addLayer( const QDomElement &layerElem, QList<QDomNode> &broken
 {
   QString type = layerElem.attribute( QStringLiteral( "type" ) );
   QgsDebugMsgLevel( "Layer type is " + type, 4 );
-  QgsMapLayer *mapLayer = nullptr;
+  std::unique_ptr<QgsMapLayer> mapLayer;
 
   if ( type == QLatin1String( "vector" ) )
   {
-    mapLayer = new QgsVectorLayer;
+    mapLayer = qgis::make_unique<QgsVectorLayer>();
 
     // apply specific settings to vector layer
-    if ( QgsVectorLayer *vl = qobject_cast<QgsVectorLayer *>( mapLayer ) )
+    if ( QgsVectorLayer *vl = qobject_cast<QgsVectorLayer *>( mapLayer.get() ) )
     {
       vl->setReadExtentFromXml( mTrustLayerMetadata );
     }
   }
   else if ( type == QLatin1String( "raster" ) )
   {
-    mapLayer = new QgsRasterLayer;
+    mapLayer =  qgis::make_unique<QgsRasterLayer>();
   }
   else if ( type == QLatin1String( "mesh" ) )
   {
-    mapLayer = new QgsMeshLayer;
+    mapLayer = qgis::make_unique<QgsMeshLayer>();
   }
   else if ( type == QLatin1String( "plugin" ) )
   {
     QString typeName = layerElem.attribute( QStringLiteral( "name" ) );
-    mapLayer = QgsApplication::pluginLayerRegistry()->createLayer( typeName );
+    mapLayer.reset( QgsApplication::pluginLayerRegistry()->createLayer( typeName ) );
   }
 
   if ( !mapLayer )
@@ -978,13 +978,19 @@ bool QgsProject::addLayer( const QDomElement &layerElem, QList<QDomNode> &broken
 
   Q_CHECK_PTR( mapLayer ); // NOLINT
 
+  // This is tricky: to avoid a leak we need to check if the layer was already in the store
+  // because if it was, the newly created layer will not be added to the store and it would leak.
+  const QString layerId { layerElem.namedItem( QStringLiteral( "id" ) ).toElement().text() };
+  Q_ASSERT( ! layerId.isEmpty() );
+  const bool layerWasStored { layerStore()->mapLayer( layerId ) };
+
   // have the layer restore state that is stored in Dom node
   bool layerIsValid = mapLayer->readLayerXml( layerElem, context ) && mapLayer->isValid();
   QList<QgsMapLayer *> newLayers;
-  newLayers << mapLayer;
+  newLayers << mapLayer.get();
   if ( layerIsValid )
   {
-    emit readMapLayer( mapLayer, layerElem );
+    emit readMapLayer( mapLayer.get(), layerElem );
     addMapLayers( newLayers );
   }
   else
@@ -995,6 +1001,14 @@ bool QgsProject::addLayer( const QDomElement &layerElem, QList<QDomNode> &broken
     QgsDebugMsg( "Unable to load " + type + " layer" );
     brokenNodes.push_back( layerElem );
   }
+
+  // It should be safe to delete the layer now if layer was stored, because all the store
+  // had to to was to reset the data source in case the validity changed.
+  if ( ! layerWasStored )
+  {
+    mapLayer.release();
+  }
+
   return layerIsValid;
 }
 

Original file line number	Diff line number	Diff line change
`@@ -944,30 +944,30 @@ bool QgsProject::addLayer( const QDomElement &layerElem, QList<QDomNode> &broken`
`944`	`944`	`{`
`945`	`945`	`QString type = layerElem.attribute( QStringLiteral( "type" ) );`
`946`	`946`	`QgsDebugMsgLevel( "Layer type is " + type, 4 );`
`947`		`- QgsMapLayer *mapLayer = nullptr;`
	`947`	`+ std::unique_ptr<QgsMapLayer> mapLayer;`
`948`	`948`
`949`	`949`	`if ( type == QLatin1String( "vector" ) )`
`950`	`950`	`{`
`951`		`- mapLayer = new QgsVectorLayer;`
	`951`	`+ mapLayer = qgis::make_unique<QgsVectorLayer>();`
`952`	`952`
`953`	`953`	`// apply specific settings to vector layer`
`954`		`- if ( QgsVectorLayer vl = qobject_cast<QgsVectorLayer >( mapLayer ) )`
	`954`	`+ if ( QgsVectorLayer vl = qobject_cast<QgsVectorLayer >( mapLayer.get() ) )`
`955`	`955`	`{`
`956`	`956`	`vl->setReadExtentFromXml( mTrustLayerMetadata );`
`957`	`957`	`}`
`958`	`958`	`}`
`959`	`959`	`else if ( type == QLatin1String( "raster" ) )`
`960`	`960`	`{`
`961`		`- mapLayer = new QgsRasterLayer;`
	`961`	`+ mapLayer = qgis::make_unique<QgsRasterLayer>();`
`962`	`962`	`}`
`963`	`963`	`else if ( type == QLatin1String( "mesh" ) )`
`964`	`964`	`{`
`965`		`- mapLayer = new QgsMeshLayer;`
	`965`	`+ mapLayer = qgis::make_unique<QgsMeshLayer>();`
`966`	`966`	`}`
`967`	`967`	`else if ( type == QLatin1String( "plugin" ) )`
`968`	`968`	`{`
`969`	`969`	`QString typeName = layerElem.attribute( QStringLiteral( "name" ) );`
`970`		`- mapLayer = QgsApplication::pluginLayerRegistry()->createLayer( typeName );`
	`970`	`+ mapLayer.reset( QgsApplication::pluginLayerRegistry()->createLayer( typeName ) );`
`971`	`971`	`}`
`972`	`972`
`973`	`973`	`if ( !mapLayer )`
`@@ -978,13 +978,19 @@ bool QgsProject::addLayer( const QDomElement &layerElem, QList<QDomNode> &broken`
`978`	`978`
`979`	`979`	`Q_CHECK_PTR( mapLayer ); // NOLINT`
`980`	`980`
	`981`	`+ // This is tricky: to avoid a leak we need to check if the layer was already in the store`
	`982`	`+ // because if it was, the newly created layer will not be added to the store and it would leak.`
	`983`	`+ const QString layerId { layerElem.namedItem( QStringLiteral( "id" ) ).toElement().text() };`
	`984`	`+ Q_ASSERT( ! layerId.isEmpty() );`
	`985`	`+ const bool layerWasStored { layerStore()->mapLayer( layerId ) };`
	`986`	`+`
`981`	`987`	`// have the layer restore state that is stored in Dom node`
`982`	`988`	`bool layerIsValid = mapLayer->readLayerXml( layerElem, context ) && mapLayer->isValid();`
`983`	`989`	`QList<QgsMapLayer *> newLayers;`
`984`		`- newLayers << mapLayer;`
	`990`	`+ newLayers << mapLayer.get();`
`985`	`991`	`if ( layerIsValid )`
`986`	`992`	`{`
`987`		`- emit readMapLayer( mapLayer, layerElem );`
	`993`	`+ emit readMapLayer( mapLayer.get(), layerElem );`
`988`	`994`	`addMapLayers( newLayers );`
`989`	`995`	`}`
`990`	`996`	`else`
`@@ -995,6 +1001,14 @@ bool QgsProject::addLayer( const QDomElement &layerElem, QList<QDomNode> &broken`
`995`	`1001`	`QgsDebugMsg( "Unable to load " + type + " layer" );`
`996`	`1002`	`brokenNodes.push_back( layerElem );`
`997`	`1003`	`}`
	`1004`	`+`
	`1005`	`+ // It should be safe to delete the layer now if layer was stored, because all the store`
	`1006`	`+ // had to to was to reset the data source in case the validity changed.`
	`1007`	`+ if ( ! layerWasStored )`
	`1008`	`+ {`
	`1009`	`+ mapLayer.release();`
	`1010`	`+ }`
	`1011`	`+`
`998`	`1012`	`return layerIsValid;`
`999`	`1013`	`}`
`1000`	`1014`