Add an assert to protect multiple calls to QgsSymbolV2::startRender()

nyalldawson · nyalldawson · commit 348088e773a5 · 2017-07-11T09:58:00.000+10:00
while rendering has already been started for a particular symbol instance

Relates to a random but frequent crash which occurs when using the
categorised symbol renderer - tracked down to a race condition
in which multiple concurrent calls to startRender() are performed
on a single symbol instance.
diff --git a/src/core/symbology-ng/qgssymbolv2.cpp b/src/core/symbology-ng/qgssymbolv2.cpp
@@ -89,9 +89,9 @@ QgsSymbolV2::QgsSymbolV2( SymbolType type, const QgsSymbolLayerV2List& layers )
     , mRenderHints( 0 )
     , mClipFeaturesToExtent( true )
     , mLayer( nullptr )
+    , mStarted( false )
     , mSymbolRenderContext( nullptr )
 {
-
   // check they're all correct symbol layers
   for ( int i = 0; i < mLayers.count(); i++ )
   {
@@ -444,6 +444,9 @@ bool QgsSymbolV2::changeSymbolLayer( int index, QgsSymbolLayerV2* layer )
 
 void QgsSymbolV2::startRender( QgsRenderContext& context, const QgsFields* fields )
 {
+  Q_ASSERT_X( !mStarted, "startRender", "Rendering has already been started for this symbol instance!" );
+
+  mStarted = true;
   delete mSymbolRenderContext;
   mSymbolRenderContext = new QgsSymbolV2RenderContext( context, outputUnit(), mAlpha, false, mRenderHints, nullptr, fields, mapUnitScale() );
 
@@ -459,6 +462,9 @@ void QgsSymbolV2::startRender( QgsRenderContext& context, const QgsFields* field
 
 void QgsSymbolV2::stopRender( QgsRenderContext& context )
 {
+  Q_ASSERT_X( mStarted, "startRender", "startRender was not called for this symbol instance!" );
+  mStarted = false;
+
   Q_UNUSED( context )
   if ( mSymbolRenderContext )
   {
diff --git a/src/core/symbology-ng/qgssymbolv2.h b/src/core/symbology-ng/qgssymbolv2.h
@@ -335,6 +335,9 @@ class CORE_EXPORT QgsSymbolV2
     const QgsVectorLayer* mLayer; //current vectorlayer
 
   private:
+    //! True if render has already been started - guards against multiple calls to
+    //! startRender() (usually a result of not cloning a shared symbol instance before rendering).
+    bool mStarted;
     //! Initialized in startRender, destroyed in stopRender
     QgsSymbolV2RenderContext* mSymbolRenderContext;
 

Original file line number	Diff line number	Diff line change
`@@ -89,9 +89,9 @@ QgsSymbolV2::QgsSymbolV2( SymbolType type, const QgsSymbolLayerV2List& layers )`
`89`	`89`	`, mRenderHints( 0 )`
`90`	`90`	`, mClipFeaturesToExtent( true )`
`91`	`91`	`, mLayer( nullptr )`
	`92`	`+ , mStarted( false )`
`92`	`93`	`, mSymbolRenderContext( nullptr )`
`93`	`94`	`{`
`94`		`-`
`95`	`95`	`// check they're all correct symbol layers`
`96`	`96`	`for ( int i = 0; i < mLayers.count(); i++ )`
`97`	`97`	`{`
`@@ -444,6 +444,9 @@ bool QgsSymbolV2::changeSymbolLayer( int index, QgsSymbolLayerV2* layer )`
`444`	`444`
`445`	`445`	`void QgsSymbolV2::startRender( QgsRenderContext& context, const QgsFields* fields )`
`446`	`446`	`{`
	`447`	`+ Q_ASSERT_X( !mStarted, "startRender", "Rendering has already been started for this symbol instance!" );`
	`448`	`+`
	`449`	`+ mStarted = true;`
`447`	`450`	`delete mSymbolRenderContext;`
`448`	`451`	`mSymbolRenderContext = new QgsSymbolV2RenderContext( context, outputUnit(), mAlpha, false, mRenderHints, nullptr, fields, mapUnitScale() );`
`449`	`452`
`@@ -459,6 +462,9 @@ void QgsSymbolV2::startRender( QgsRenderContext& context, const QgsFields* field`
`459`	`462`
`460`	`463`	`void QgsSymbolV2::stopRender( QgsRenderContext& context )`
`461`	`464`	`{`
	`465`	`+ Q_ASSERT_X( mStarted, "startRender", "startRender was not called for this symbol instance!" );`
	`466`	`+ mStarted = false;`
	`467`	`+`
`462`	`468`	`Q_UNUSED( context )`
`463`	`469`	`if ( mSymbolRenderContext )`
`464`	`470`	`{`