Fix crashes related to tile loading in background

- feature source must not be shared by multiple loaders (not thread safe) - root rule must not be shared (not thread safe) - delete map scene before map settings
qgis · Jan 8, 2020 · e52bdf3 · e52bdf3
1 parent 108e8c8
commit e52bdf3
Show file tree

Hide file tree

Showing 5 changed files with 62 additions and 20 deletions.
diff --git a/src/3d/qgsrulebasedchunkloader_p.cpp b/src/3d/qgsrulebasedchunkloader_p.cpp
@@ -17,6 +17,7 @@ QgsRuleBasedChunkLoader::QgsRuleBasedChunkLoader( const QgsRuleBasedChunkLoaderF
   : QgsChunkLoader( node )
   , mFactory( factory )
   , mContext( factory->mMap )
+  , mSource( new QgsVectorLayerFeatureSource( factory->mLayer ) )
 {
   if ( node->level() < mFactory->mLeafLevel )
   {
@@ -31,10 +32,16 @@ QgsRuleBasedChunkLoader::QgsRuleBasedChunkLoader( const QgsRuleBasedChunkLoaderF
   exprContext.setFields( layer->fields() );
   mContext.setExpressionContext( exprContext );
 
-  mFactory->mRootRule->createHandlers( layer, mHandlers );
+  // factory is shared among multiple loaders which may be run at the same time
+  // so we need a local copy of our rule tree that does not intefere with others
+  // (e.g. it happened that filter expressions with invalid syntax would cause
+  // nasty crashes when trying to simultaneously record evaluation error)
+  mRootRule.reset( mFactory->mRootRule->clone() );
+
+  mRootRule->createHandlers( layer, mHandlers );
 
   QSet<QString> attributeNames;
-  mFactory->mRootRule->prepare( mContext, attributeNames, mHandlers );
+  mRootRule->prepare( mContext, attributeNames, mHandlers );
 
   // build the feature request
   QgsFeatureRequest req;
@@ -54,20 +61,32 @@ QgsRuleBasedChunkLoader::QgsRuleBasedChunkLoader( const QgsRuleBasedChunkLoaderF
     QgsEventTracing::ScopedEvent e( "3D", "RB chunk load" );
 
     QgsFeature f;
-    QgsFeatureIterator fi = mFactory->mSource->getFeatures( req );
+    QgsFeatureIterator fi = mSource->getFeatures( req );
     while ( fi.nextFeature( f ) )
     {
       if ( mCanceled )
         break;
       mContext.expressionContext().setFeature( f );
-      mFactory->mRootRule->registerFeature( f, mContext, mHandlers );
+      mRootRule->registerFeature( f, mContext, mHandlers );
     }
   } );
 
   // emit finished() as soon as the handler is populated with features
-  QFutureWatcher<void> *fw = new QFutureWatcher<void>( nullptr );
-  fw->setFuture( future );
-  connect( fw, &QFutureWatcher<void>::finished, this, &QgsChunkQueueJob::finished );
+  mFutureWatcher = new QFutureWatcher<void>( this );
+  mFutureWatcher->setFuture( future );
+  connect( mFutureWatcher, &QFutureWatcher<void>::finished, this, &QgsChunkQueueJob::finished );
+}
+
+QgsRuleBasedChunkLoader::~QgsRuleBasedChunkLoader()
+{
+  if ( mFutureWatcher && !mFutureWatcher->isFinished() )
+  {
+    disconnect( mFutureWatcher, &QFutureWatcher<void>::finished, this, &QgsChunkQueueJob::finished );
+    mFutureWatcher->waitForFinished();
+  }
+
+  qDeleteAll( mHandlers );
+  mHandlers.clear();
 }
 
 void QgsRuleBasedChunkLoader::cancel()
@@ -86,9 +105,6 @@ Qt3DCore::QEntity *QgsRuleBasedChunkLoader::createEntity( Qt3DCore::QEntity *par
   for ( QgsFeature3DHandler *handler : mHandlers.values() )
     handler->finalize( entity, mContext );
 
-  qDeleteAll( mHandlers );
-  mHandlers.clear();
-
   return entity;
 }
 
@@ -101,7 +117,10 @@ QgsRuleBasedChunkLoaderFactory::QgsRuleBasedChunkLoaderFactory( const Qgs3DMapSe
   , mLayer( vl )
   , mRootRule( rootRule->clone() )
   , mLeafLevel( leafLevel )
-  , mSource( new QgsVectorLayerFeatureSource( vl ) )
+{
+}
+
+QgsRuleBasedChunkLoaderFactory::~QgsRuleBasedChunkLoaderFactory()
 {
 }
 

diff --git a/src/3d/qgsrulebasedchunkloader_p.h b/src/3d/qgsrulebasedchunkloader_p.h
@@ -17,6 +17,7 @@ class QgsRuleBasedChunkLoaderFactory : public QgsChunkLoaderFactory
 {
   public:
     QgsRuleBasedChunkLoaderFactory( const Qgs3DMapSettings &map, QgsVectorLayer *vl, QgsRuleBased3DRenderer::Rule *rootRule, int leafLevel );
+    ~QgsRuleBasedChunkLoaderFactory();
 
     //! Creates loader for the given chunk node. Ownership of the returned is passed to the caller.
     virtual QgsChunkLoader *createChunkLoader( QgsChunkNode *node ) const;
@@ -25,7 +26,6 @@ class QgsRuleBasedChunkLoaderFactory : public QgsChunkLoaderFactory
     QgsVectorLayer *mLayer;
     std::unique_ptr<QgsRuleBased3DRenderer::Rule> mRootRule;
     int mLeafLevel;
-    std::unique_ptr<QgsVectorLayerFeatureSource> mSource;
 };
 
 
@@ -35,6 +35,7 @@ class QgsRuleBasedChunkLoader : public QgsChunkLoader
 {
   public:
     QgsRuleBasedChunkLoader( const QgsRuleBasedChunkLoaderFactory *factory, QgsChunkNode *node );
+    ~QgsRuleBasedChunkLoader();
 
     virtual void cancel();
     virtual Qt3DCore::QEntity *createEntity( Qt3DCore::QEntity *parent );
@@ -43,7 +44,10 @@ class QgsRuleBasedChunkLoader : public QgsChunkLoader
     const QgsRuleBasedChunkLoaderFactory *mFactory;
     QgsRuleBased3DRenderer::RuleToHandlerMap mHandlers;
     Qgs3DRenderContext mContext;
+    std::unique_ptr<QgsVectorLayerFeatureSource> mSource;
     bool mCanceled = false;
+    QFutureWatcher<void> *mFutureWatcher = nullptr;
+    std::unique_ptr<QgsRuleBased3DRenderer::Rule> mRootRule;
 };
 
 

diff --git a/src/3d/qgsvectorlayerchunkloader_p.cpp b/src/3d/qgsvectorlayerchunkloader_p.cpp
@@ -38,6 +38,7 @@ QgsVectorLayerChunkLoader::QgsVectorLayerChunkLoader( const QgsVectorLayerChunkL
   : QgsChunkLoader( node )
   , mFactory( factory )
   , mContext( factory->mMap )
+  , mSource( new QgsVectorLayerFeatureSource( factory->mLayer ) )
 {
   if ( node->level() < mFactory->mLeafLevel )
   {
@@ -83,7 +84,7 @@ QgsVectorLayerChunkLoader::QgsVectorLayerChunkLoader( const QgsVectorLayerChunkL
     QgsEventTracing::ScopedEvent e( "3D", "VL chunk load" );
 
     QgsFeature f;
-    QgsFeatureIterator fi = mFactory->mSource->getFeatures( req );
+    QgsFeatureIterator fi = mSource->getFeatures( req );
     while ( fi.nextFeature( f ) )
     {
       if ( mCanceled )
@@ -94,9 +95,21 @@ QgsVectorLayerChunkLoader::QgsVectorLayerChunkLoader( const QgsVectorLayerChunkL
   } );
 
   // emit finished() as soon as the handler is populated with features
-  QFutureWatcher<void> *fw = new QFutureWatcher<void>( nullptr );
-  fw->setFuture( future );
-  connect( fw, &QFutureWatcher<void>::finished, this, &QgsChunkQueueJob::finished );
+  mFutureWatcher = new QFutureWatcher<void>( this );
+  mFutureWatcher->setFuture( future );
+  connect( mFutureWatcher, &QFutureWatcher<void>::finished, this, &QgsChunkQueueJob::finished );
+}
+
+QgsVectorLayerChunkLoader::~QgsVectorLayerChunkLoader()
+{
+  if ( mFutureWatcher && !mFutureWatcher->isFinished() )
+  {
+    disconnect( mFutureWatcher, &QFutureWatcher<void>::finished, this, &QgsChunkQueueJob::finished );
+    mFutureWatcher->waitForFinished();
+  }
+
+  delete mHandler;
+  mHandler = nullptr;
 }
 
 void QgsVectorLayerChunkLoader::cancel()
@@ -113,8 +126,6 @@ Qt3DCore::QEntity *QgsVectorLayerChunkLoader::createEntity( Qt3DCore::QEntity *p
 
   Qt3DCore::QEntity *entity = new Qt3DCore::QEntity( parent );
   mHandler->finalize( entity, mContext );
-  delete mHandler;
-  mHandler = nullptr;
   return entity;
 }
 
@@ -127,7 +138,6 @@ QgsVectorLayerChunkLoaderFactory::QgsVectorLayerChunkLoaderFactory( const Qgs3DM
   , mLayer( vl )
   , mSymbol( symbol->clone() )
   , mLeafLevel( leafLevel )
-  , mSource( new QgsVectorLayerFeatureSource( vl ) )
 {
 }
 

diff --git a/src/3d/qgsvectorlayerchunkloader_p.h b/src/3d/qgsvectorlayerchunkloader_p.h
@@ -25,6 +25,8 @@ class QgsVectorLayerFeatureSource;
 class QgsAbstract3DSymbol;
 class QgsFeature3DHandler;
 
+#include <QFutureWatcher>
+
 
 class QgsVectorLayerChunkLoaderFactory : public QgsChunkLoaderFactory
 {
@@ -38,14 +40,14 @@ class QgsVectorLayerChunkLoaderFactory : public QgsChunkLoaderFactory
     QgsVectorLayer *mLayer;
     std::unique_ptr<QgsAbstract3DSymbol> mSymbol;
     int mLeafLevel;
-    std::unique_ptr<QgsVectorLayerFeatureSource> mSource;
 };
 
 
 class QgsVectorLayerChunkLoader : public QgsChunkLoader
 {
   public:
     QgsVectorLayerChunkLoader( const QgsVectorLayerChunkLoaderFactory *factory, QgsChunkNode *node );
+    ~QgsVectorLayerChunkLoader();
 
     virtual void cancel();
     virtual Qt3DCore::QEntity *createEntity( Qt3DCore::QEntity *parent );
@@ -54,7 +56,9 @@ class QgsVectorLayerChunkLoader : public QgsChunkLoader
     const QgsVectorLayerChunkLoaderFactory *mFactory;
     QgsFeature3DHandler *mHandler = nullptr;
     Qgs3DRenderContext mContext;
+    std::unique_ptr<QgsVectorLayerFeatureSource> mSource;
     bool mCanceled = false;
+    QFutureWatcher<void> *mFutureWatcher = nullptr;
 };
 
 #include "qgschunkedentity_p.h"

diff --git a/src/app/3d/qgs3dmapcanvas.cpp b/src/app/3d/qgs3dmapcanvas.cpp
@@ -57,7 +57,12 @@ Qgs3DMapCanvas::Qgs3DMapCanvas( QWidget *parent )
 
 Qgs3DMapCanvas::~Qgs3DMapCanvas()
 {
+  // make sure the scene is deleted while map settings object is still alive
+  delete mScene;
+  mScene = nullptr;
+
   delete mMap;
+  mMap = nullptr;
 }
 
 void Qgs3DMapCanvas::resizeEvent( QResizeEvent *ev )