Skip to content

Commit

Permalink
Add an assert to protect multiple calls to QgsSymbolV2::startRender()
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
nyalldawson committed Jul 11, 2017
1 parent 67c6e8f commit fabf32e
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 0 deletions.
6 changes: 6 additions & 0 deletions src/core/symbology-ng/qgssymbol.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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() );
Expand All @@ -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 )
{
Expand Down
4 changes: 4 additions & 0 deletions src/core/symbology-ng/qgssymbol.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down

4 comments on commit fabf32e

@nirvn
Copy link
Contributor

@nirvn nirvn commented on fabf32e Jul 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nyalldawson , QGIS now crashes when opening a project here:

#0  0x00007ffff2fe877f in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:58
#1  0x00007ffff2fea37a in __GI_abort () at abort.c:89
#2  0x000055555555d7b1 in qgisCrash(int) (signal=-1) at /home/webmaster/dev/cpp/QGIS/src/app/main.cpp:329
#3  0x000055555555da5a in myMessageOutput(QtMsgType, char const*) (type=QtFatalMsg, msg=0x555559255af8 "ASSERT failure in startRender: \"Rendering has already been started for this symbol instance!\", file /home/webmaster/dev/cpp/QGIS/src/core/symbology-ng/qgssymbol.cpp, line 386") at /home/webmaster/dev/cpp/QGIS/src/app/main.cpp:382
#4  0x00007ffff39aa99d in  () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#5  0x00007ffff39ac439 in QMessageLogger::fatal(char const*, ...) const () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#6  0x00007ffff39a7981 in  () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#7  0x00007ffff536fc74 in QgsSymbol::startRender(QgsRenderContext&, QgsFields const&) (this=0x55555919d2b0, context=..., fields=...)
    at /home/webmaster/dev/cpp/QGIS/src/core/symbology-ng/qgssymbol.cpp:386
#8  0x00007ffff529444b in QgsLinePatternFillSymbolLayer::startRender(QgsSymbolRenderContext&) (this=0x55555919dd10, context=...)
    at /home/webmaster/dev/cpp/QGIS/src/core/symbology-ng/qgsfillsymbollayer.cpp:2799
#9  0x00007ffff533f7b4 in QgsFillSymbolLayer::drawPreviewIcon(QgsSymbolRenderContext&, QSize) (this=0x55555919dd10, context=..., size=...)
    at /home/webmaster/dev/cpp/QGIS/src/core/symbology-ng/qgssymbollayer.cpp:639
#10 0x00007ffff5349f86 in QgsSymbolLayerUtils::symbolLayerPreviewIcon(QgsSymbolLayer*, QgsUnitTypes::RenderUnit, QSize, QgsMapUnitScale const&) (layer=0x55555919dd10, u=QgsUnitTypes::RenderMillimeters, size=..., scale=...) at /home/webmaster/dev/cpp/QGIS/src/core/symbology-ng/qgssymbollayerutils.cpp:667
#11 0x00007ffff658d497 in SymbolLayerItem::updatePreview() (this=0x555559253cf0) at /home/webmaster/dev/cpp/QGIS/src/gui/symbology-ng/qgssymbolselectordialog.cpp:150
#12 0x00007ffff658d3ce in SymbolLayerItem::setLayer(QgsSymbolLayer*) (this=0x555559253cf0, layer=0x55555919dd10) at /home/webmaster/dev/cpp/QGIS/src/gui/symbology-ng/qgssymbolselectordialog.cpp:135
#13 0x00007ffff658d2f4 in SymbolLayerItem::SymbolLayerItem(QgsSymbolLayer*) (this=0x555559253cf0, layer=0x55555919dd10)
    at /home/webmaster/dev/cpp/QGIS/src/gui/symbology-ng/qgssymbolselectordialog.cpp:122
#14 0x00007ffff6588b28 in QgsSymbolSelectorWidget::loadSymbol(QgsSymbol*, SymbolLayerItem*) (this=0x55555923d360, symbol=0x5555591a0440, parent=0x55555925a720)
    at /home/webmaster/dev/cpp/QGIS/src/gui/symbology-ng/qgssymbolselectordialog.cpp:321
#15 0x00007ffff65881b3 in QgsSymbolSelectorWidget::QgsSymbolSelectorWidget(QgsSymbol*, QgsStyle*, QgsVectorLayer const*, QWidget*) (this=0x55555923d360, symbol=0x5555591a0440, style=
    0x55555645f710, vl=0x5555591986a0, parent=0x0) at /home/webmaster/dev/cpp/QGIS/src/gui/symbology-ng/qgssymbolselectordialog.cpp:257
#16 0x00007ffff64f07b1 in QgsSingleSymbolRendererWidget::QgsSingleSymbolRendererWidget(QgsVectorLayer*, QgsStyle*, QgsFeatureRenderer*) (this=
    0x555558532290, layer=0x5555591986a0, style=0x55555645f710, renderer=0x555556cf8340) at /home/webmaster/dev/cpp/QGIS/src/gui/symbology-ng/qgssinglesymbolrendererwidget.cpp:57
#17 0x00007ffff64f05cb in QgsSingleSymbolRendererWidget::create(QgsVectorLayer*, QgsStyle*, QgsFeatureRenderer*) (layer=0x5555591986a0, style=0x55555645f710, renderer=0x555556cf8340)
    at /home/webmaster/dev/cpp/QGIS/src/gui/symbology-ng/qgssinglesymbolrendererwidget.cpp:32
#18 0x00007ffff530cb1e in QgsRendererMetadata::createRendererWidget(QgsVectorLayer*, QgsStyle*, QgsFeatureRenderer*) (this=0x555555c33d10, layer=0x5555591986a0, style=0x55555645f710, renderer=0x555556cf8340) at /home/webmaster/dev/cpp/QGIS/src/core/symbology-ng/qgsrendererregistry.h:148
#19 0x00007ffff64d1bcf in QgsRendererPropertiesDialog::rendererChanged() (this=0x5555591e6130) at /home/webmaster/dev/cpp/QGIS/src/gui/symbology-ng/qgsrendererpropertiesdialog.cpp:243
#20 0x00007ffff64d76a9 in QtPrivate::FunctorCall<QtPrivate::IndexesList<>, QtPrivate::List<>, void, void (QgsRendererPropertiesDialog::*)()>::call(void (QgsRendererPropertiesDialog::*)(), QgsRendererPropertiesDialog*, void**) (f=(void (QgsRendererPropertiesDialog::*)(QgsRendererPropertiesDialog * const)) 0x7ffff64d194a <QgsRendererPropertiesDialog::rendererChanged()>, o=0x5555591e6130, arg=0x7fffffffa050) at /usr/include/x86_64-linux-gnu/qt5/QtCore/qobjectdefs_impl.h:141
#21 0x00007ffff64d75a2 in QtPrivate::FunctionPointer<void (QgsRendererPropertiesDialog::*)()>::call<QtPrivate::List<>, void>(void (QgsRendererPropertiesDialog::*)(), QgsRendererPropertiesDialog*, void**) (f=(void (QgsRendererPropertiesDialog::*)(QgsRendererPropertiesDialog * const)) 0x7ffff64d194a <QgsRendererPropertiesDialog::rendererChanged()>, o=0x5555591e6130, arg=0x7fffffffa050)
    at /usr/include/x86_64-linux-gnu/qt5/QtCore/qobjectdefs_impl.h:160
#22 0x00007ffff64d6fa1 in QtPrivate::QSlotObject<void (QgsRendererPropertiesDialog::*)(), QtPrivate::List<>, void>::impl(int, QtPrivate::QSlotObjectBase*, QObject*, void**, bool*) (which=1, this_=0x555559244ef0, r=0x5555591e6130, a=0x7fffffffa050, ret=0x0) at /usr/include/x86_64-linux-gnu/qt5/QtCore/qobject_impl.h:120
#23 0x00007ffff3bc181e in QMetaObject::activate(QObject*, int, int, void**) () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#24 0x00007ffff496b471 in QComboBox::currentIndexChanged(int) () at /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#25 0x00007ffff496d8c1 in  () at /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#26 0x00007ffff497002d in  () at /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#27 0x00007ffff497024f in QComboBox::setCurrentIndex(int) () at /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#28 0x00007ffff64d2adc in QgsRendererPropertiesDialog::syncToLayer() (this=0x5555591e6130) at /home/webmaster/dev/cpp/QGIS/src/gui/symbology-ng/qgsrendererpropertiesdialog.cpp:378
#29 0x00007ffff64d0edb in QgsRendererPropertiesDialog::QgsRendererPropertiesDialog(QgsVectorLayer*, QgsStyle*, bool, QWidget*) (this=0x5555591e6130, layer=0x5555591986a0, style=0x55555645f710, embedded=true, parent=0x555556a82350) at /home/webmaster/dev/cpp/QGIS/src/gui/symbology-ng/qgsrendererpropertiesdialog.cpp:123
#30 0x00007ffff73e325c in QgsLayerStylingWidget::updateCurrentWidgetLayer() (this=0x555556a81d20) at /home/webmaster/dev/cpp/QGIS/src/app/qgslayerstylingwidget.cpp:354
#31 0x00007ffff73e8c5f in QtPrivate::FunctorCall<QtPrivate::IndexesList<>, QtPrivate::List<>, void, void (QgsLayerStylingWidget::*)()>::call(void (QgsLayerStylingWidget::*)(), QgsLayerStylingWidget*, void**) (f=(void (QgsLayerStylingWidget::*)(QgsLayerStylingWidget * const)) 0x7ffff73e2e98 <QgsLayerStylingWidget::updateCurrentWidgetLayer()>, o=0x555556a81d20, arg=0x7fffffffa770)

@nyalldawson
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nirvn looks like you've found another place where this crash could occur! Basically what my commit did is force these to be brought to our attention, rather then letting them hide away until random occurrences.

@nirvn
Copy link
Contributor

@nirvn nirvn commented on fabf32e Jul 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nyalldawson , I've been able to come up with a small test project that crashes QGIS:
qgis-crash.zip

@nirvn
Copy link
Contributor

@nirvn nirvn commented on fabf32e Jul 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nyalldawson , actually, no test project needed. Simply add a polygon dataset, and change its symbology to a line pattern symbol. boom.

Please sign in to comment.