Play it safe and work with clones of symbol renderers

This fixes some definite crashes, e.g. saving a vector layer with symbology, which is caused by accessing the same layer renderer across different threads. But I've also swapped a lot of non-threaded code to use clones of the renderers for extra safety.
qgis · Nov 19, 2017 · fe535c5 · fe535c5
1 parent d13eaa6
commit fe535c5
Show file tree

Hide file tree

Showing 10 changed files with 44 additions and 43 deletions.
diff --git a/src/app/qgsmaptoolpointsymbol.cpp b/src/app/qgsmaptoolpointsymbol.cpp
@@ -70,9 +70,10 @@ void QgsMapToolPointSymbol::canvasPressEvent( QgsMapMouseEvent *e )
   }
 
   //check whether selected feature has a modifiable symbol
-  QgsFeatureRenderer *renderer = mActiveLayer->renderer();
-  if ( !renderer )
+  if ( !mActiveLayer->renderer() )
     return;
+
+  std::unique_ptr< QgsFeatureRenderer > renderer( mActiveLayer->renderer()->clone() );
   QgsRenderContext context = QgsRenderContext::fromMapSettings( mCanvas->mapSettings() );
   context.expressionContext() << QgsExpressionContextUtils::layerScope( mActiveLayer );
   context.expressionContext().setFeature( feature );

diff --git a/src/app/qgsmaptoolselectutils.cpp b/src/app/qgsmaptoolselectutils.cpp
@@ -229,9 +229,12 @@ QgsFeatureIds QgsMapToolSelectUtils::getMatchingFeatures( QgsMapCanvas *canvas,
 
   QgsRenderContext context = QgsRenderContext::fromMapSettings( canvas->mapSettings() );
   context.expressionContext() << QgsExpressionContextUtils::layerScope( vlayer );
-  QgsFeatureRenderer *r = vlayer->renderer();
-  if ( r )
+  std::unique_ptr< QgsFeatureRenderer > r;
+  if ( vlayer->renderer() )
+  {
+    r.reset( vlayer->renderer()->clone() );
     r->startRender( context, vlayer->fields() );
+  }
 
   QgsFeatureRequest request;
   request.setFilterRect( selectGeomTrans.boundingBox() );

diff --git a/src/core/dxf/qgsdxfexport.cpp b/src/core/dxf/qgsdxfexport.cpp
@@ -982,13 +982,14 @@ void QgsDxfExport::writeEntities()
     }
 
     QgsSymbolRenderContext sctx( ctx, QgsUnitTypes::RenderMillimeters, 1.0, false, 0, nullptr );
-    QgsFeatureRenderer *renderer = vl->renderer();
-    if ( !renderer )
+    if ( !vl->renderer() )
     {
       if ( hasStyleOverride )
         vl->styleManager()->restoreOverrideStyle();
       continue;
     }
+
+    std::unique_ptr< QgsFeatureRenderer > renderer( vl->renderer()->clone() );
     renderer->startRender( ctx, vl->fields() );
 
     QSet<QString> attributes = renderer->usedAttributes( ctx );
@@ -1118,12 +1119,12 @@ void QgsDxfExport::writeEntitiesSymbolLevels( QgsVectorLayer *layer )
     return;
   }
 
-  QgsFeatureRenderer *renderer = layer->renderer();
-  if ( !renderer )
+  if ( !layer->renderer() )
   {
     // TODO return error
     return;
   }
+  std::unique_ptr< QgsFeatureRenderer > renderer( layer->renderer()->clone() );
   QHash< QgsSymbol *, QList<QgsFeature> > features;
 
   QgsRenderContext ctx = renderContext();

diff --git a/src/core/qgsmaphittest.cpp b/src/core/qgsmaphittest.cpp
@@ -106,7 +106,7 @@ void QgsMapHitTest::runHitTestLayer( QgsVectorLayer *vl, SymbolSet &usedSymbols,
   if ( hasStyleOverride )
     vl->styleManager()->setOverrideStyle( mSettings.layerStyleOverrides().value( vl->id() ) );
 
-  QgsFeatureRenderer *r = vl->renderer();
+  std::unique_ptr< QgsFeatureRenderer > r( vl->renderer()->clone() );
   bool moreSymbolsPerFeature = r->capabilities() & QgsFeatureRenderer::MoreSymbolsPerFeature;
   r->startRender( context, vl->fields() );
 

diff --git a/src/core/qgsvectorfilewriter.cpp b/src/core/qgsvectorfilewriter.cpp
@@ -2655,7 +2655,7 @@ QgsVectorFileWriter::writeAsVectorFormat( QgsVectorLayer *layer,
     n++;
   }
 
-  writer->stopRender( layer );
+  writer->stopRender();
   delete writer;
 
   if ( errors > 0 && errorMessage && n > 0 )
@@ -3135,7 +3135,7 @@ QgsVectorFileWriter::WriterError QgsVectorFileWriter::exportFeaturesSymbolLevels
     }
   }
 
-  stopRender( layer );
+  stopRender();
 
   if ( nErrors > 0 && errorMessage )
   {
@@ -3181,46 +3181,44 @@ double QgsVectorFileWriter::mapUnitScaleFactor( double scale, QgsUnitTypes::Rend
 
 void QgsVectorFileWriter::startRender( QgsVectorLayer *vl )
 {
-  QgsFeatureRenderer *renderer = symbologyRenderer( vl );
-  if ( !renderer )
+  mRenderer = createSymbologyRenderer( vl );
+  if ( !mRenderer )
   {
     return;
   }
 
-  renderer->startRender( mRenderContext, vl->fields() );
+  mRenderer->startRender( mRenderContext, vl->fields() );
 }
 
-void QgsVectorFileWriter::stopRender( QgsVectorLayer *vl )
+void QgsVectorFileWriter::stopRender()
 {
-  QgsFeatureRenderer *renderer = symbologyRenderer( vl );
-  if ( !renderer )
+  if ( !mRenderer )
   {
     return;
   }
 
-  renderer->stopRender( mRenderContext );
+  mRenderer->stopRender( mRenderContext );
 }
 
-QgsFeatureRenderer *QgsVectorFileWriter::symbologyRenderer( QgsVectorLayer *vl ) const
+std::unique_ptr<QgsFeatureRenderer> QgsVectorFileWriter::createSymbologyRenderer( QgsVectorLayer *vl ) const
 {
   if ( mSymbologyExport == NoSymbology )
   {
     return nullptr;
   }
-  if ( !vl )
+  if ( !vl || !vl->renderer() )
   {
     return nullptr;
   }
 
-  return vl->renderer();
+  return std::unique_ptr< QgsFeatureRenderer >( vl->renderer()->clone() );
 }
 
 void QgsVectorFileWriter::addRendererAttributes( QgsVectorLayer *vl, QgsAttributeList &attList )
 {
-  QgsFeatureRenderer *renderer = symbologyRenderer( vl );
-  if ( renderer )
+  if ( mRenderer )
   {
-    const QSet<QString> rendererAttributes = renderer->usedAttributes( mRenderContext );
+    const QSet<QString> rendererAttributes = mRenderer->usedAttributes( mRenderContext );
     for ( const QString &attr : rendererAttributes )
     {
       int index = vl->fields().lookupField( attr );

diff --git a/src/core/qgsvectorfilewriter.h b/src/core/qgsvectorfilewriter.h
@@ -729,6 +729,7 @@ class CORE_EXPORT QgsVectorFileWriter : public QgsFeatureSink
                QgsVectorFileWriter::ActionOnExistingFile action );
     void resetMap( const QgsAttributeList &attributes );
 
+    std::unique_ptr< QgsFeatureRenderer > mRenderer;
     QgsRenderContext mRenderContext;
 
     bool mUsingTransaction = false;
@@ -744,8 +745,8 @@ class CORE_EXPORT QgsVectorFileWriter : public QgsFeatureSink
     double mapUnitScaleFactor( double scale, QgsUnitTypes::RenderUnit symbolUnits, QgsUnitTypes::DistanceUnit mapUnits );
 
     void startRender( QgsVectorLayer *vl );
-    void stopRender( QgsVectorLayer *vl );
-    QgsFeatureRenderer *symbologyRenderer( QgsVectorLayer *vl ) const;
+    void stopRender();
+    std::unique_ptr< QgsFeatureRenderer > createSymbologyRenderer( QgsVectorLayer *vl ) const;
     //! Adds attributes needed for classification
     void addRendererAttributes( QgsVectorLayer *vl, QgsAttributeList &attList );
     static QMap<QString, MetaData> sDriverMetadata;

diff --git a/src/gui/qgshighlight.cpp b/src/gui/qgshighlight.cpp
@@ -103,13 +103,13 @@ void QgsHighlight::setFillColor( const QColor &fillColor )
   mBrush.setStyle( Qt::SolidPattern );
 }
 
-QgsFeatureRenderer *QgsHighlight::getRenderer( QgsRenderContext &context, const QColor &color, const QColor &fillColor )
+std::unique_ptr<QgsFeatureRenderer> QgsHighlight::createRenderer( QgsRenderContext &context, const QColor &color, const QColor &fillColor )
 {
-  QgsFeatureRenderer *renderer = nullptr;
+  std::unique_ptr<QgsFeatureRenderer> renderer;
   QgsVectorLayer *layer = qobject_cast<QgsVectorLayer *>( mLayer );
   if ( layer && layer->renderer() )
   {
-    renderer = layer->renderer()->clone();
+    renderer.reset( layer->renderer()->clone() );
   }
   if ( renderer )
   {
@@ -328,23 +328,23 @@ void QgsHighlight::paint( QPainter *p )
     QColor tmpColor( 255, 0, 0, 255 );
     QColor tmpFillColor( 0, 255, 0, 255 );
 
-    QgsFeatureRenderer *renderer = getRenderer( context, tmpColor, tmpFillColor );
+    std::unique_ptr< QgsFeatureRenderer > renderer = createRenderer( context, tmpColor, tmpFillColor );
     if ( layer && renderer )
     {
 
       QSize imageSize( mMapCanvas->mapSettings().outputSize() );
       QImage image = QImage( imageSize.width(), imageSize.height(), QImage::Format_ARGB32 );
       image.fill( 0 );
-      QPainter *imagePainter = new QPainter( &image );
-      imagePainter->setRenderHint( QPainter::Antialiasing, true );
+      QPainter imagePainter( &image );
+      imagePainter.setRenderHint( QPainter::Antialiasing, true );
 
-      context.setPainter( imagePainter );
+      context.setPainter( &imagePainter );
 
       renderer->startRender( context, layer->fields() );
       renderer->renderFeature( mFeature, context );
       renderer->stopRender( context );
 
-      imagePainter->end();
+      imagePainter.end();
 
       QColor color( mPen.color() );  // true output color
       // coefficient to subtract alpha using green (temporary fill)
@@ -366,9 +366,6 @@ void QgsHighlight::paint( QPainter *p )
       }
 
       p->drawImage( 0, 0, image );
-
-      delete imagePainter;
-      delete renderer;
     }
   }
 }

diff --git a/src/gui/qgshighlight.h b/src/gui/qgshighlight.h
@@ -120,7 +120,7 @@ class GUI_EXPORT QgsHighlight: public QgsMapCanvasItem
     void setSymbol( QgsSymbol *symbol, const QgsRenderContext &context, const QColor &color, const QColor &fillColor );
     double getSymbolWidth( const QgsRenderContext &context, double width, QgsUnitTypes::RenderUnit unit );
     //! Get renderer for current color mode and colors. The renderer should be freed by caller.
-    QgsFeatureRenderer *getRenderer( QgsRenderContext &context, const QColor &color, const QColor &fillColor );
+    std::unique_ptr< QgsFeatureRenderer > createRenderer( QgsRenderContext &context, const QColor &color, const QColor &fillColor );
     void paintPoint( QPainter *p, const QgsPointXY &point );
     void paintLine( QPainter *p, QgsPolylineXY line );
     void paintPolygon( QPainter *p, const QgsPolygonXY &polygon );

diff --git a/src/gui/qgsmaptoolidentify.cpp b/src/gui/qgsmaptoolidentify.cpp
@@ -253,8 +253,8 @@ bool QgsMapToolIdentify::identifyVectorLayer( QList<IdentifyResult> *results, Qg
 
   QgsRenderContext context( QgsRenderContext::fromMapSettings( mCanvas->mapSettings() ) );
   context.expressionContext() << QgsExpressionContextUtils::layerScope( layer );
-  QgsFeatureRenderer *renderer = layer->renderer();
-  if ( renderer && renderer->capabilities() & QgsFeatureRenderer::ScaleDependent )
+  std::unique_ptr< QgsFeatureRenderer > renderer( layer->renderer() ? layer->renderer()->clone() : nullptr );
+  if ( renderer )
   {
     // setup scale for scale dependent visibility (rule based)
     renderer->startRender( context, layer->fields() );
@@ -280,7 +280,7 @@ bool QgsMapToolIdentify::identifyVectorLayer( QList<IdentifyResult> *results, Qg
     results->append( IdentifyResult( qobject_cast<QgsMapLayer *>( layer ), *f_it, derivedAttributes ) );
   }
 
-  if ( renderer && renderer->capabilities() & QgsFeatureRenderer::ScaleDependent )
+  if ( renderer )
   {
     renderer->stopRender( context );
   }

diff --git a/src/server/services/wms/qgswmsrenderer.cpp b/src/server/services/wms/qgswmsrenderer.cpp
@@ -255,7 +255,7 @@ namespace QgsWms
 
   void QgsRenderer::runHitTestLayer( QgsVectorLayer *vl, SymbolSet &usedSymbols, QgsRenderContext &context ) const
   {
-    QgsFeatureRenderer *r = vl->renderer();
+    std::unique_ptr< QgsFeatureRenderer > r( vl->renderer()->clone() );
     bool moreSymbolsPerFeature = r->capabilities() & QgsFeatureRenderer::MoreSymbolsPerFeature;
     r->startRender( context, vl->pendingFields() );
     QgsFeature f;
@@ -1476,7 +1476,7 @@ namespace QgsWms
 #endif
 
     QgsFeatureIterator fit = layer->getFeatures( fReq );
-    QgsFeatureRenderer *r2 = layer->renderer();
+    std::unique_ptr< QgsFeatureRenderer > r2( layer->renderer() ? layer->renderer()->clone() : nullptr );
     if ( r2 )
     {
       r2->startRender( renderContext, layer->pendingFields() );