Don't crash on transaction save

Fixes #39265
qgis · Oct 12, 2020 · c2831cc · c2831cc
1 parent 1d2bb41
commit c2831cc
Show file tree

Hide file tree

Showing 3 changed files with 66 additions and 9 deletions.
diff --git a/src/core/qgstransactiongroup.cpp b/src/core/qgstransactiongroup.cpp
@@ -19,6 +19,7 @@
 #include "qgsvectorlayer.h"
 #include "qgsdatasourceuri.h"
 #include "qgsvectordataprovider.h"
+#include "qgslogger.h"
 
 #include <QTimer>
 
@@ -102,25 +103,37 @@ void QgsTransactionGroup::onBeforeCommitChanges( bool stopEditing )
 
   mEditingStopping = true;
 
-  QgsVectorLayer *triggeringLayer = qobject_cast<QgsVectorLayer *>( sender() );
+  const QgsVectorLayer *triggeringLayer = qobject_cast<QgsVectorLayer *>( sender() );
 
   QString errMsg;
   if ( mTransaction->commit( errMsg ) )
   {
     const auto constMLayers = mLayers;
     for ( QgsVectorLayer *layer : constMLayers )
     {
-      if ( layer != sender() )
+      if ( layer != triggeringLayer )
+      {
         layer->commitChanges( stopEditing );
+      }
+    }
+
+    if ( stopEditing )
+    {
+      disableTransaction();
+    }
+    else
+    {
+      if ( ! mTransaction->begin( errMsg ) )
+      {
+        QgsDebugMsg( QStringLiteral( "Could not restart a transaction for %1: %2" ).arg( triggeringLayer->name() ).arg( errMsg ) );
+      }
     }
 
-    disableTransaction();
   }
   else
   {
     emit commitError( errMsg );
-    // Restart editing the calling layer in the next event loop cycle
-    QTimer::singleShot( 0, triggeringLayer, &QgsVectorLayer::startEditing );
+    restartTransaction( triggeringLayer );
   }
   mEditingStopping = false;
 }
@@ -147,8 +160,7 @@ void QgsTransactionGroup::onRollback()
   }
   else
   {
-    // Restart editing the calling layer in the next event loop cycle
-    QTimer::singleShot( 0, triggeringLayer, &QgsVectorLayer::startEditing );
+    restartTransaction( triggeringLayer );
   }
   mEditingStopping = false;
 }
@@ -165,6 +177,12 @@ void QgsTransactionGroup::disableTransaction()
   }
 }
 
+void QgsTransactionGroup::restartTransaction( const QgsVectorLayer *layer )
+{
+  // Restart editing the calling layer in the next event loop cycle
+  QTimer::singleShot( 0, layer, &QgsVectorLayer::startEditing );
+}
+
 QString QgsTransactionGroup::providerKey() const
 {
   return mProviderKey;

diff --git a/src/core/qgstransactiongroup.h b/src/core/qgstransactiongroup.h
@@ -91,6 +91,8 @@ class CORE_EXPORT QgsTransactionGroup : public QObject
 
     void disableTransaction();
 
+    void restartTransaction( const QgsVectorLayer *layer );
+
     QSet<QgsVectorLayer *> mLayers;
     //! Only set while a transaction is active
     std::unique_ptr<QgsTransaction> mTransaction;

diff --git a/tests/src/python/test_provider_ogr_gpkg.py b/tests/src/python/test_provider_ogr_gpkg.py
@@ -1728,7 +1728,7 @@ def testExporterWithFIDColumn(self):
         self.assertEqual(f.id(), 123)
 
     def testTransactionGroup(self):
-        """Issue https://github.com/qgis/QGIS/issues/36525"""
+        """Test issue GH #36525"""
 
         project = QgsProject()
         project.setAutoTransaction(True)
@@ -1768,7 +1768,7 @@ def testTransactionGroup(self):
         self.assertFalse(vl1_1.isEditable())
         self.assertFalse(vl1_2.isEditable())
 
-    @unittest.skipIf(int(gdal.VersionInfo('VERSION_NUM')) < GDAL_COMPUTE_VERSION(2, 3, 0), "GDAL 2.3 required")
+   @unittest.skipIf(int(gdal.VersionInfo('VERSION_NUM')) < GDAL_COMPUTE_VERSION(2, 3, 0), "GDAL 2.3 required")
     def testTransactionGroupIterator(self):
         """Test issue GH #39178: the bug is that this test hangs
         forever in an endless loop"""
@@ -1800,6 +1800,43 @@ def testTransactionGroupIterator(self):
         # Test that QGIS sees the new changes
         self.assertEqual(next(vl.getFeatures()).attribute(1), 'new value')
 
+    def testTransactionGroupCrash(self):
+        """Test issue GH #39265 segfault"""
+
+        project = QgsProject()
+        project.setAutoTransaction(True)
+        tmpfile = os.path.join(
+            self.basetestpath, 'tempGeoPackageTransactionCrash.gpkg')
+        ds = ogr.GetDriverByName('GPKG').CreateDataSource(tmpfile)
+        lyr = ds.CreateLayer('test', geom_type=ogr.wkbPoint)
+        lyr.CreateField(ogr.FieldDefn('str_field', ogr.OFTString))
+
+        f = ogr.Feature(lyr.GetLayerDefn())
+        f.SetGeometry(ogr.CreateGeometryFromWkt('POINT (1 1)'))
+        f.SetField('str_field', 'one')
+        lyr.CreateFeature(f)
+
+        del lyr
+        del ds
+
+        vl = QgsVectorLayer(tmpfile + '|layername=test', 'test', 'ogr')
+
+        project.addMapLayers([vl])
+
+        feature = next(vl.getFeatures())
+        feature.setAttributes([None, 'two'])
+
+        self.assertTrue(vl.startEditing())
+        self.assertTrue(vl.addFeature(feature))
+
+        # Save without leaving editing
+        self.assertTrue(vl.commitChanges(False))
+
+        # Now add another one
+        feature.setAttributes([None, 'three'])
+        self.assertTrue(vl.addFeature(feature))
+
 
 if __name__ == '__main__':
     unittest.main()
+