PropagateUpload: Avoid crash due to cascading aborts

https://sentry.io/owncloud/desktop-win-and-mac/issues/698694072/activity/

diff --git a/src/libsync/propagateupload.cpp b/src/libsync/propagateupload.cpp
index 9f1c1cf6b29d012beac8b0be98c2fdf5acf24399..dd67be3ae359881e867c22ddc5a51ae7c84ab3be 100644 (file)
--- a/src/libsync/propagateupload.cpp
+++ b/src/libsync/propagateupload.cpp
@@ -169,6 +169,7 @@ PropagateUploadFileCommon::PropagateUploadFileCommon(OwncloudPropagator *propaga
     : PropagateItemJob(propagator, item)
     , _finished(false)
     , _deleteExisting(false)
+    , _aborting(false)
     , _parallelism(FullParallelism)
     , _uploadEncryptedHelper(nullptr)
     , _uploadingEncrypted(false)
@@ -696,6 +697,8 @@ void PropagateUploadFileCommon::slotJobDestroyed(QObject *job)
 // This function is used whenever there is an error occuring and jobs might be in progress
 void PropagateUploadFileCommon::abortWithError(SyncFileItem::Status status, const QString &error)
 {
+    if (_aborting)
+        return;
     abort(AbortType::Synchronous);
     done(status, error);
 }
@@ -777,6 +780,10 @@ void PropagateUploadFileCommon::abortNetworkJobs(
     PropagatorJob::AbortType abortType,
     const std::function<bool(AbstractNetworkJob *)> &mayAbortJob)
 {
+    if (_aborting)
+        return;
+    _aborting = true;
+
     // Count the number of jobs that need aborting, and emit the overall
     // abort signal when they're all done.
     QSharedPointer<int> runningCount(new int(0));

diff --git a/src/libsync/propagateupload.h b/src/libsync/propagateupload.h
index 591477f1eafafe14a4f791d56c93d8c7be8dc126..f5fed24ed6fffe7e702a34e3185f14c72527d670 100644 (file)
--- a/src/libsync/propagateupload.h
+++ b/src/libsync/propagateupload.h
@@ -212,6 +212,13 @@ protected:
     bool _finished BITFIELD(1); /// Tells that all the jobs have been finished
     bool _deleteExisting BITFIELD(1);
 
+    /** Whether an abort is currently ongoing.
+     *
+     * Important to avoid duplicate aborts since each finishing PUTFileJob might
+     * trigger an abort on error.
+     */
+    bool _aborting BITFIELD(1);
+
     /* This is a minified version of the SyncFileItem,
      * that holds only the specifics about the file that's
      * being uploaded.

diff --git a/test/syncenginetestutils.h b/test/syncenginetestutils.h
index 2d960ca25aad81d04e3adbce206d68ebe6844bcb..efa581d17b097e0c8687e0ccccce0ad8b23a0465 100644 (file)
--- a/test/syncenginetestutils.h
+++ b/test/syncenginetestutils.h
@@ -732,7 +732,7 @@ public:
         QMetaObject::invokeMethod(this, "respond", Qt::QueuedConnection);
     }
 
-    Q_INVOKABLE void respond() {
+    Q_INVOKABLE virtual void respond() {
         emit metaDataChanged();
         emit readyRead();
         // finishing can come strictly after readyRead was called
@@ -916,6 +916,7 @@ public:
         _account->setUrl(QUrl(QStringLiteral("http://admin:admin@localhost/owncloud")));
         _account->setCredentials(new FakeCredentials{_fakeQnam});
         _account->setDavDisplayName("fakename");
+        _account->setServerVersion("10.0.0");
 
         _journalDb = std::make_unique<OCC::SyncJournalDb>(localPath() + "._sync_test.db");
         _syncEngine = std::make_unique<OCC::SyncEngine>(_account, localPath(), "", _journalDb.get());

diff --git a/test/testsyncengine.cpp b/test/testsyncengine.cpp
index 3f6f5ba8c4a290888567c526a528efde8b092e0d..193b8189d8fa6178779a031dc5411afc75eb03a4 100644 (file)
--- a/test/testsyncengine.cpp
+++ b/test/testsyncengine.cpp
@@ -649,6 +649,33 @@ private slots:
         QTextCodec::setCodecForLocale(utf8Locale);
 #endif
     }
+
+    // Aborting has had bugs when there are parallel upload jobs
+    void testUploadV1Multiabort()
+    {
+        FakeFolder fakeFolder{ FileInfo{} };
+        SyncOptions options;
+        options._initialChunkSize = 10;
+        options._maxChunkSize = 10;
+        options._minChunkSize = 10;
+        fakeFolder.syncEngine().setSyncOptions(options);
+
+        QObject parent;
+        int nPUT = 0;
+        fakeFolder.setServerOverride([&](QNetworkAccessManager::Operation op, const QNetworkRequest &request, QIODevice *) -> QNetworkReply * {
+            if (op == QNetworkAccessManager::PutOperation) {
+                ++nPUT;
+                return new FakeHangingReply(op, request, &parent);
+            }
+            return nullptr;
+        });
+
+        fakeFolder.localModifier().insert("file", 100, 'W');
+        QTimer::singleShot(100, &fakeFolder.syncEngine(), [&]() { fakeFolder.syncEngine().abort(); });
+        QVERIFY(!fakeFolder.syncOnce());
+
+        QCOMPARE(nPUT, 3);
+    }
 };
 
 QTEST_GUILESS_MAIN(TestSyncEngine)