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

nyalldawson · nyalldawson · commit fabf32efc589 · 2017-07-11T14:12:02.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/qgssymbol.cpp b/src/core/symbology-ng/qgssymbol.cpp
@@ -383,6 +383,9 @@ bool QgsSymbol::changeSymbolLayer( int index, QgsSymbolLayer *layer )
 
 void QgsSymbol::startRender( QgsRenderContext &context, const QgsFields &fields )
 {
+  Q_ASSERT_X( !mStarted, "startRender", "Rendering has already been started for this symbol instance!" );
+  mStarted = true;
+
   mSymbolRenderContext.reset( new QgsSymbolRenderContext( context, outputUnit(), mOpacity, false, mRenderHints, nullptr, fields, mapUnitScale() ) );
 
   QgsSymbolRenderContext symbolContext( context, outputUnit(), mOpacity, false, mRenderHints, nullptr, fields, mapUnitScale() );
@@ -402,6 +405,9 @@ void QgsSymbol::startRender( QgsRenderContext &context, const QgsFields &fields
 
 void QgsSymbol::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/qgssymbol.h b/src/core/symbology-ng/qgssymbol.h
@@ -388,6 +388,10 @@ class CORE_EXPORT QgsSymbol
     QgsSymbol( const QgsSymbol & );
 #endif
 
+    //! 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 = false;
+
     //! Initialized in startRender, destroyed in stopRender
     std::unique_ptr< QgsSymbolRenderContext > mSymbolRenderContext;
 

Original file line number	Diff line number	Diff line change
`@@ -383,6 +383,9 @@ bool QgsSymbol::changeSymbolLayer( int index, QgsSymbolLayer *layer )`
`383`	`383`
`384`	`384`	`void QgsSymbol::startRender( QgsRenderContext &context, const QgsFields &fields )`
`385`	`385`	`{`
	`386`	`+ Q_ASSERT_X( !mStarted, "startRender", "Rendering has already been started for this symbol instance!" );`
	`387`	`+ mStarted = true;`
	`388`	`+`
`386`	`389`	`mSymbolRenderContext.reset( new QgsSymbolRenderContext( context, outputUnit(), mOpacity, false, mRenderHints, nullptr, fields, mapUnitScale() ) );`
`387`	`390`
`388`	`391`	`QgsSymbolRenderContext symbolContext( context, outputUnit(), mOpacity, false, mRenderHints, nullptr, fields, mapUnitScale() );`
`@@ -402,6 +405,9 @@ void QgsSymbol::startRender( QgsRenderContext &context, const QgsFields &fields`
`402`	`405`
`403`	`406`	`void QgsSymbol::stopRender( QgsRenderContext &context )`
`404`	`407`	`{`
	`408`	`+ Q_ASSERT_X( mStarted, "startRender", "startRender was not called for this symbol instance!" );`
	`409`	`+ mStarted = false;`
	`410`	`+`
`405`	`411`	`Q_UNUSED( context )`
`406`	`412`	`if ( mSymbolRenderContext )`
`407`	`413`	`{`