Don't crash on transaction save

elpaso · elpaso · commit c2831cc61841 · 2020-10-12T09:05:49.000+02:00
Fixes #39265
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()
+

Original file line number	Diff line number	Diff line change
`@@ -19,6 +19,7 @@`
`19`	`19`	`#include "qgsvectorlayer.h"`
`20`	`20`	`#include "qgsdatasourceuri.h"`
`21`	`21`	`#include "qgsvectordataprovider.h"`
	`22`	`+#include "qgslogger.h"`
`22`	`23`
`23`	`24`	`#include <QTimer>`
`24`	`25`
`@@ -102,25 +103,37 @@ void QgsTransactionGroup::onBeforeCommitChanges( bool stopEditing )`
`102`	`103`
`103`	`104`	`mEditingStopping = true;`
`104`	`105`
`105`		`- QgsVectorLayer triggeringLayer = qobject_cast<QgsVectorLayer >( sender() );`
	`106`	`+ const QgsVectorLayer triggeringLayer = qobject_cast<QgsVectorLayer >( sender() );`
`106`	`107`
`107`	`108`	`QString errMsg;`
`108`	`109`	`if ( mTransaction->commit( errMsg ) )`
`109`	`110`	`{`
`110`	`111`	`const auto constMLayers = mLayers;`
`111`	`112`	`for ( QgsVectorLayer *layer : constMLayers )`
`112`	`113`	`{`
`113`		`- if ( layer != sender() )`
	`114`	`+ if ( layer != triggeringLayer )`
	`115`	`+ {`
`114`	`116`	`layer->commitChanges( stopEditing );`
	`117`	`+ }`
	`118`	`+ }`
	`119`	`+`
	`120`	`+ if ( stopEditing )`
	`121`	`+ {`
	`122`	`+ disableTransaction();`
	`123`	`+ }`
	`124`	`+ else`
	`125`	`+ {`
	`126`	`+ if ( ! mTransaction->begin( errMsg ) )`
	`127`	`+ {`
	`128`	`+ QgsDebugMsg( QStringLiteral( "Could not restart a transaction for %1: %2" ).arg( triggeringLayer->name() ).arg( errMsg ) );`
	`129`	`+ }`
`115`	`130`	`}`
`116`	`131`
`117`		`- disableTransaction();`
`118`	`132`	`}`
`119`	`133`	`else`
`120`	`134`	`{`
`121`	`135`	`emit commitError( errMsg );`
`122`		`- // Restart editing the calling layer in the next event loop cycle`
`123`		`- QTimer::singleShot( 0, triggeringLayer, &QgsVectorLayer::startEditing );`
	`136`	`+ restartTransaction( triggeringLayer );`
`124`	`137`	`}`
`125`	`138`	`mEditingStopping = false;`
`126`	`139`	`}`
`@@ -147,8 +160,7 @@ void QgsTransactionGroup::onRollback()`
`147`	`160`	`}`
`148`	`161`	`else`
`149`	`162`	`{`
`150`		`- // Restart editing the calling layer in the next event loop cycle`
`151`		`- QTimer::singleShot( 0, triggeringLayer, &QgsVectorLayer::startEditing );`
	`163`	`+ restartTransaction( triggeringLayer );`
`152`	`164`	`}`
`153`	`165`	`mEditingStopping = false;`
`154`	`166`	`}`
`@@ -165,6 +177,12 @@ void QgsTransactionGroup::disableTransaction()`
`165`	`177`	`}`
`166`	`178`	`}`
`167`	`179`
	`180`	`+void QgsTransactionGroup::restartTransaction( const QgsVectorLayer *layer )`
	`181`	`+{`
	`182`	`+ // Restart editing the calling layer in the next event loop cycle`
	`183`	`+ QTimer::singleShot( 0, layer, &QgsVectorLayer::startEditing );`
	`184`	`+}`
	`185`	`+`
`168`	`186`	`QString QgsTransactionGroup::providerKey() const`
`169`	`187`	`{`
`170`	`188`	`return mProviderKey;`