Propagator: Fix finished signal of directory being emited twice and c…

…ausing crash When there is a FatalError, we ended up emiting the finished signal for the directory job several times, which would lead to crashes Issue #5578
owncloud · Mar 6, 2017 · bb02dd3 · bb02dd3
1 parent e7296d0
commit bb02dd3
Show file tree

Hide file tree

Showing 3 changed files with 75 additions and 12 deletions.
diff --git a/src/libsync/owncloudpropagator.cpp b/src/libsync/owncloudpropagator.cpp
@@ -669,9 +669,11 @@ void PropagateDirectory::slotSubJobFinished(SyncFileItem::Status status)
 
     if (status == SyncFileItem::FatalError ||
             (wasFirstJob && status != SyncFileItem::Success && status != SyncFileItem::Restoration)) {
-        abort();
-        _state = Finished;
-        emit finished(status);
+        if (_state != Finished) {
+            abort();
+            _state = Finished;
+            emit finished(status);
+        }
         return;
     } else if (status == SyncFileItem::NormalError || status == SyncFileItem::SoftError) {
         _hasError = status;
@@ -688,6 +690,9 @@ void PropagateDirectory::slotSubJobFinished(SyncFileItem::Status status)
 
 void PropagateDirectory::finalize()
 {
+    if (_state == Finished)
+        return;
+
     bool ok = true;
     if (!_item->isEmpty() && _hasError == SyncFileItem::NoStatus) {
         if( !_item->_renameTarget.isEmpty() ) {

diff --git a/test/syncenginetestutils.h b/test/syncenginetestutils.h
@@ -536,6 +536,7 @@ class FakeGetReply : public QNetworkReply
     const FileInfo *fileInfo;
     char payload;
     int size;
+    bool aborted = false;
 
     FakeGetReply(FileInfo &remoteRootFileInfo, QNetworkAccessManager::Operation op, const QNetworkRequest &request, QObject *parent)
     : QNetworkReply{parent} {
@@ -551,6 +552,12 @@ class FakeGetReply : public QNetworkReply
     }
 
     Q_INVOKABLE void respond() {
+        if (aborted) {
+            setError(OperationCanceledError, "Operation Canceled");
+            emit metaDataChanged();
+            emit finished();
+            return;
+        }
         payload = fileInfo->contentChar;
         size = fileInfo->size;
         setHeader(QNetworkRequest::ContentLengthHeader, size);
@@ -564,8 +571,14 @@ class FakeGetReply : public QNetworkReply
         emit finished();
     }
 
-    void abort() override { }
-    qint64 bytesAvailable() const override { return size + QIODevice::bytesAvailable(); }
+    void abort() override {
+        aborted = true;
+    }
+    qint64 bytesAvailable() const override {
+        if (aborted)
+            return 0;
+        return size + QIODevice::bytesAvailable();
+    }
 
     qint64 readData(char *data, qint64 maxlen) override {
         qint64 len = std::min(qint64{size}, maxlen);
@@ -666,8 +679,9 @@ class FakeErrorReply : public QNetworkReply
 {
     Q_OBJECT
 public:
-    FakeErrorReply(QNetworkAccessManager::Operation op, const QNetworkRequest &request, QObject *parent)
-    : QNetworkReply{parent} {
+    FakeErrorReply(QNetworkAccessManager::Operation op, const QNetworkRequest &request,
+                   QObject *parent, int httpErrorCode)
+    : QNetworkReply{parent}, _httpErrorCode(httpErrorCode) {
         setRequest(request);
         setUrl(request.url());
         setOperation(op);
@@ -676,34 +690,38 @@ class FakeErrorReply : public QNetworkReply
     }
 
     Q_INVOKABLE void respond() {
-        setAttribute(QNetworkRequest::HttpStatusCodeAttribute, 500);
+        setAttribute(QNetworkRequest::HttpStatusCodeAttribute, _httpErrorCode);
         setError(InternalServerError, "Internal Server Fake Error");
         emit metaDataChanged();
         emit finished();
     }
 
     void abort() override { }
     qint64 readData(char *, qint64) override { return 0; }
+
+    int _httpErrorCode;
 };
 
 class FakeQNAM : public QNetworkAccessManager
 {
     FileInfo _remoteRootFileInfo;
     FileInfo _uploadFileInfo;
-    QStringList _errorPaths;
+    // maps a path to an HTTP error
+    QHash<QString, int> _errorPaths;
 public:
     FakeQNAM(FileInfo initialRoot) : _remoteRootFileInfo{std::move(initialRoot)} { }
     FileInfo &currentRemoteState() { return _remoteRootFileInfo; }
     FileInfo &uploadState() { return _uploadFileInfo; }
-    QStringList &errorPaths() { return _errorPaths; }
+
+    QHash<QString, int> &errorPaths() { return _errorPaths; }
 
 protected:
     QNetworkReply *createRequest(Operation op, const QNetworkRequest &request,
                                          QIODevice *outgoingData = 0) {
         const QString fileName = getFilePathFromUrl(request.url());
         Q_ASSERT(!fileName.isNull());
         if (_errorPaths.contains(fileName))
-            return new FakeErrorReply{op, request, this};
+            return new FakeErrorReply{op, request, this, _errorPaths[fileName]};
 
         bool isUpload = request.url().path().startsWith(sUploadUrl.path());
         FileInfo &info = isUpload ? _uploadFileInfo : _remoteRootFileInfo;
@@ -798,7 +816,13 @@ class FakeFolder
     FileInfo currentRemoteState() { return _fakeQnam->currentRemoteState(); }
     FileInfo &uploadState() { return _fakeQnam->uploadState(); }
 
-    QStringList &serverErrorPaths() { return _fakeQnam->errorPaths(); }
+    struct ErrorList {
+        FakeQNAM *_qnam;
+        void append(const QString &path, int error = 500)
+        { _qnam->errorPaths().insert(path, error); }
+        void clear() { _qnam->errorPaths().clear(); }
+    };
+    ErrorList serverErrorPaths() { return {_fakeQnam}; }
 
     QString localPath() const {
         // SyncEngine wants a trailing slash

diff --git a/test/testsyncengine.cpp b/test/testsyncengine.cpp
@@ -225,6 +225,40 @@ private slots:
         QCOMPARE(finishedSpy.size(), 1);
         QCOMPARE(finishedSpy.first().first().toBool(), false);
     }
+
+    void testDirDownloadWithError() {
+        FakeFolder fakeFolder{FileInfo::A12_B12_C12_S12()};
+        QSignalSpy completeSpy(&fakeFolder.syncEngine(), SIGNAL(itemCompleted(const SyncFileItemPtr &)));
+        fakeFolder.remoteModifier().mkdir("Y");
+        fakeFolder.remoteModifier().mkdir("Y/Z");
+        fakeFolder.remoteModifier().insert("Y/Z/d0");
+        fakeFolder.remoteModifier().insert("Y/Z/d1");
+        fakeFolder.remoteModifier().insert("Y/Z/d2");
+        fakeFolder.remoteModifier().insert("Y/Z/d3");
+        fakeFolder.remoteModifier().insert("Y/Z/d4");
+        fakeFolder.remoteModifier().insert("Y/Z/d5");
+        fakeFolder.remoteModifier().insert("Y/Z/d6");
+        fakeFolder.remoteModifier().insert("Y/Z/d7");
+        fakeFolder.remoteModifier().insert("Y/Z/d8");
+        fakeFolder.remoteModifier().insert("Y/Z/d9");
+        fakeFolder.serverErrorPaths().append("Y/Z/d2", 503); // 503 is a fatal error
+        fakeFolder.serverErrorPaths().append("Y/Z/d3", 503); // 503 is a fatal error
+        QVERIFY(!fakeFolder.syncOnce());
+        QCoreApplication::processEvents(); // should not crash
+
+        QSet<QString> seen;
+        for(const QList<QVariant> &args : completeSpy) {
+            auto item = args[0].value<SyncFileItemPtr>();
+            qDebug() << item->_file << item->_isDirectory << item->_status;
+            QVERIFY(!seen.contains(item->_file)); // signal only sent once per item
+            seen.insert(item->_file);
+            if (item->_file == "Y/Z/d2" || item->_file == "Y/Z/d3") {
+                QVERIFY(item->_status == SyncFileItem::FatalError);
+            }
+            QVERIFY(item->_file != "Y/Z/d9"); // we should have aborted the sync before d9 starts
+        }
+    }
+
 };
 
 QTEST_GUILESS_MAIN(TestSyncEngine)