From bb02dd3b6e29c5d0db659d5c8376a10a5908563f Mon Sep 17 00:00:00 2001 From: Olivier Goffart Date: Mon, 6 Mar 2017 13:07:08 +0100 Subject: [PATCH] Propagator: Fix finished signal of directory being emited twice and causing 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 --- src/libsync/owncloudpropagator.cpp | 11 +++++--- test/syncenginetestutils.h | 42 +++++++++++++++++++++++------- test/testsyncengine.cpp | 34 ++++++++++++++++++++++++ 3 files changed, 75 insertions(+), 12 deletions(-) diff --git a/src/libsync/owncloudpropagator.cpp b/src/libsync/owncloudpropagator.cpp index 6e73c0d5dbc..e2c910213ff 100644 --- 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 index 232cd419dca..937bf35fda4 100644 --- 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,7 +690,7 @@ 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(); @@ -684,18 +698,22 @@ class FakeErrorReply : public QNetworkReply 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 _errorPaths; public: FakeQNAM(FileInfo initialRoot) : _remoteRootFileInfo{std::move(initialRoot)} { } FileInfo ¤tRemoteState() { return _remoteRootFileInfo; } FileInfo &uploadState() { return _uploadFileInfo; } - QStringList &errorPaths() { return _errorPaths; } + + QHash &errorPaths() { return _errorPaths; } protected: QNetworkReply *createRequest(Operation op, const QNetworkRequest &request, @@ -703,7 +721,7 @@ class FakeQNAM : public QNetworkAccessManager 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 index ae5296ea1e4..2498860c269 100644 --- 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 seen; + for(const QList &args : completeSpy) { + auto item = args[0].value(); + 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)