vfs: Be more careful about Vfs instance ownership

diff --git a/src/common/vfs.cpp b/src/common/vfs.cpp
index 9809ff4a692d9c1380947df7b06d152d54b21a80..961c0c280984c5815659e0b96a83954ef2bf68be 100644 (file)
--- a/src/common/vfs.cpp
+++ b/src/common/vfs.cpp
@@ -87,7 +87,7 @@ Vfs::Mode OCC::bestAvailableVfsMode()
 
 Q_LOGGING_CATEGORY(lcPlugin, "plugins", QtInfoMsg)
 
-Vfs *OCC::createVfsFromPlugin(Vfs::Mode mode, QObject *parent)
+std::unique_ptr<Vfs> OCC::createVfsFromPlugin(Vfs::Mode mode)
 {
     auto name = modeToPluginName(mode);
     if (name.isEmpty())
@@ -107,7 +107,7 @@ Vfs *OCC::createVfsFromPlugin(Vfs::Mode mode, QObject *parent)
         return nullptr;
     }
 
-    auto vfs = qobject_cast<Vfs *>(factory->create(parent));
+    auto vfs = std::unique_ptr<Vfs>(qobject_cast<Vfs *>(factory->create(nullptr)));
     if (!vfs) {
         qCWarning(lcPlugin) << "Plugin" << pluginPath << "does not create a Vfs instance";
         return nullptr;

diff --git a/src/common/vfs.h b/src/common/vfs.h
index 7d68854d8fdbb61c70324081a11b22aacfa1b898..28fab2085a17d343ddb8c25e2b1dae68653573f3 100644 (file)
--- a/src/common/vfs.h
+++ b/src/common/vfs.h
@@ -17,6 +17,8 @@
 #include <QScopedPointer>
 #include <QSharedPointer>
 
+#include <memory>
+
 #include "ocsynclib.h"
 #include "result.h"
 
@@ -163,6 +165,6 @@ OCSYNC_EXPORT bool isVfsPluginAvailable(Vfs::Mode mode);
 OCSYNC_EXPORT Vfs::Mode bestAvailableVfsMode();
 
 /// Create a VFS instance for the mode, returns nullptr on failure.
-OCSYNC_EXPORT Vfs *createVfsFromPlugin(Vfs::Mode mode, QObject *parent);
+OCSYNC_EXPORT std::unique_ptr<Vfs> createVfsFromPlugin(Vfs::Mode mode);
 
 } // namespace OCC

diff --git a/src/gui/folder.cpp b/src/gui/folder.cpp
index da85ea4b5c3be80815f869c7c102253cea15aeea..adfc75bc15fa119f105227f13dec3310af68c706 100644 (file)
--- a/src/gui/folder.cpp
+++ b/src/gui/folder.cpp
@@ -51,7 +51,7 @@ namespace OCC {
 Q_LOGGING_CATEGORY(lcFolder, "nextcloud.gui.folder", QtInfoMsg)
 
 Folder::Folder(const FolderDefinition &definition,
-    AccountState *accountState, Vfs *vfs,
+    AccountState *accountState, std::unique_ptr<Vfs> vfs,
     QObject *parent)
     : QObject(parent)
     , _accountState(accountState)
@@ -61,7 +61,7 @@ Folder::Folder(const FolderDefinition &definition,
     , _consecutiveFollowUpSyncs(0)
     , _journal(_definition.absoluteJournalPath())
     , _fileLog(new SyncRunFileLog)
-    , _vfs(vfs)
+    , _vfs(vfs.release())
 {
     _timeSinceLastSyncStart.start();
     _timeSinceLastSyncDone.start();
@@ -120,13 +120,12 @@ Folder::Folder(const FolderDefinition &definition,
 
     // Potentially upgrade suffix vfs to windows vfs
     if (_definition.virtualFilesMode == Vfs::WithSuffix && _definition.upgradeVfsMode) {
-        if (auto winvfs = createVfsFromPlugin(Vfs::WindowsCfApi, this)) {
+        if (auto winvfs = createVfsFromPlugin(Vfs::WindowsCfApi)) {
             // Wipe the existing suffix files from fs and journal
-            SyncEngine::wipeVirtualFiles(path(), _journal, _vfs);
+            SyncEngine::wipeVirtualFiles(path(), _journal, _vfs.data());
 
             // Then switch to winvfs mode
-            delete _vfs;
-            _vfs = winvfs;
+            _vfs.reset(winvfs.release());
             _definition.virtualFilesMode = Vfs::WindowsCfApi;
             saveToSettings();
         }
@@ -473,8 +472,6 @@ void Folder::startVfs()
     ENFORCE(_vfs);
     ENFORCE(_vfs->mode() == _definition.virtualFilesMode);
 
-    _vfs->setParent(this);
-
     VfsSetupParams vfsParams;
     vfsParams.filesystemPath = path();
     vfsParams.remotePath = remotePath();
@@ -483,8 +480,8 @@ void Folder::startVfs()
     vfsParams.providerName = Theme::instance()->appNameGUI();
     vfsParams.providerVersion = Theme::instance()->version();
 
-    connect(_vfs, &OCC::Vfs::beginHydrating, this, &Folder::slotHydrationStarts);
-    connect(_vfs, &OCC::Vfs::doneHydrating, this, &Folder::slotHydrationDone);
+    connect(_vfs.data(), &OCC::Vfs::beginHydrating, this, &Folder::slotHydrationStarts);
+    connect(_vfs.data(), &OCC::Vfs::doneHydrating, this, &Folder::slotHydrationDone);
 
     _vfs->registerFolder(vfsParams); // Do this always?
     _vfs->start(vfsParams);
@@ -600,7 +597,7 @@ void Folder::setUseVirtualFiles(bool enabled)
     if (enabled && _definition.virtualFilesMode == Vfs::Off) {
         _definition.virtualFilesMode = bestAvailableVfsMode();
 
-        _vfs = createVfsFromPlugin(_definition.virtualFilesMode, this);
+        _vfs.reset(createVfsFromPlugin(_definition.virtualFilesMode).release());
         startVfs();
 
         _saveInFoldersWithPlaceholders = true;
@@ -609,11 +606,11 @@ void Folder::setUseVirtualFiles(bool enabled)
         ENFORCE(_vfs);
 
         // TODO: Must wait for current sync to finish!
-        SyncEngine::wipeVirtualFiles(path(), _journal, _vfs);
+        SyncEngine::wipeVirtualFiles(path(), _journal, _vfs.data());
 
         _vfs->stop();
         _vfs->unregisterFolder();
-        delete _vfs;
+        _vfs.reset(nullptr);
 
         _definition.virtualFilesMode = Vfs::Off;
     }
@@ -802,7 +799,7 @@ void Folder::setSyncOptions()
     opt._newBigFolderSizeLimit = newFolderLimit.first ? newFolderLimit.second * 1000LL * 1000LL : -1; // convert from MB to B
     opt._confirmExternalStorage = cfgFile.confirmExternalStorage();
     opt._moveFilesToTrash = cfgFile.moveToTrash();
-    opt._vfs = _vfs;
+    opt._vfs = _vfs.data();
 
     QByteArray chunkSizeEnv = qgetenv("OWNCLOUD_CHUNK_SIZE");
     if (!chunkSizeEnv.isEmpty()) {

diff --git a/src/gui/folder.h b/src/gui/folder.h
index ec62080ce2ea5b36eb950d3b00aa6bdb32c45f00..507fd09f04d2f671e368e40307eca266a8598b92 100644 (file)
--- a/src/gui/folder.h
+++ b/src/gui/folder.h
@@ -28,6 +28,7 @@
 #include <QUuid>
 #include <set>
 #include <chrono>
+#include <memory>
 
 class QThread;
 class QSettings;
@@ -102,10 +103,8 @@ class Folder : public QObject
 
 public:
     /** Create a new Folder
-     *
-     * The vfs instance will be parented to this.
      */
-    Folder(const FolderDefinition &definition, AccountState *accountState, Vfs *vfs, QObject *parent = nullptr);
+    Folder(const FolderDefinition &definition, AccountState *accountState, std::unique_ptr<Vfs> vfs, QObject *parent = nullptr);
 
     ~Folder();
 
@@ -444,7 +443,7 @@ private:
     /**
      * The vfs mode instance (created by plugin) to use. Null means no vfs.
      */
-    Vfs *_vfs = nullptr;
+    QScopedPointer<Vfs> _vfs;
 };
 }
 

diff --git a/src/gui/folderman.cpp b/src/gui/folderman.cpp
index 501d5b7ec1ca3c099867814d883a443056acb640..f17f9b40245ad42852dc04c40f2d9a0626310547 100644 (file)
--- a/src/gui/folderman.cpp
+++ b/src/gui/folderman.cpp
@@ -259,12 +259,12 @@ void FolderMan::setupFoldersHelper(QSettings &settings, AccountStatePtr account,
                     qCInfo(lcFolderMan) << "Successfully migrated syncjournal database.";
                 }
 
-                Vfs *vfs = createVfsFromPlugin(folderDefinition.virtualFilesMode, nullptr);
+                auto vfs = createVfsFromPlugin(folderDefinition.virtualFilesMode);
                 if (!vfs && folderDefinition.virtualFilesMode != Vfs::Off) {
                     qCWarning(lcFolderMan) << "Could not load plugin for mode" << folderDefinition.virtualFilesMode;
                 }
 
-                Folder *f = addFolderInternal(folderDefinition, account.data(), vfs);
+                Folder *f = addFolderInternal(folderDefinition, account.data(), std::move(vfs));
                 f->saveToSettings();
 
                 continue;
@@ -284,13 +284,13 @@ void FolderMan::setupFoldersHelper(QSettings &settings, AccountStatePtr account,
                 SyncJournalDb::maybeMigrateDb(folderDefinition.localPath, folderDefinition.absoluteJournalPath());
             }
 
-            Vfs *vfs = createVfsFromPlugin(folderDefinition.virtualFilesMode, nullptr);
+            auto vfs = createVfsFromPlugin(folderDefinition.virtualFilesMode);
             if (!vfs && folderDefinition.virtualFilesMode != Vfs::Off) {
                 // TODO: Must do better error handling
                 qFatal("Could not load plugin");
             }
 
-            Folder *f = addFolderInternal(std::move(folderDefinition), account.data(), vfs);
+            Folder *f = addFolderInternal(std::move(folderDefinition), account.data(), std::move(vfs));
             if (f) {
                 // Migration: Mark folders that shall be saved in a backwards-compatible way
                 if (backwardsCompatible)
@@ -505,8 +505,7 @@ Folder *FolderMan::setupFolderFromOldConfigFile(const QString &file, AccountStat
     folderDefinition.paused = paused;
     folderDefinition.ignoreHiddenFiles = ignoreHiddenFiles();
 
-    Vfs *vfs = nullptr;
-    folder = addFolderInternal(folderDefinition, accountState, vfs);
+    folder = addFolderInternal(folderDefinition, accountState, std::unique_ptr<Vfs>());
     if (folder) {
         QStringList blackList = settings.value(QLatin1String("blackList")).toStringList();
         if (!blackList.empty()) {
@@ -994,13 +993,13 @@ Folder *FolderMan::addFolder(AccountState *accountState, const FolderDefinition
         return nullptr;
     }
 
-    Vfs *vfs = createVfsFromPlugin(folderDefinition.virtualFilesMode, nullptr);
+    auto vfs = createVfsFromPlugin(folderDefinition.virtualFilesMode);
     if (!vfs && folderDefinition.virtualFilesMode != Vfs::Off) {
         qCWarning(lcFolderMan) << "Could not load plugin for mode" << folderDefinition.virtualFilesMode;
         return 0;
     }
 
-    auto folder = addFolderInternal(definition, accountState, vfs);
+    auto folder = addFolderInternal(definition, accountState, std::move(vfs));
 
     // Migration: The first account that's configured for a local folder shall
     // be saved in a backwards-compatible way.
@@ -1025,7 +1024,7 @@ Folder *FolderMan::addFolder(AccountState *accountState, const FolderDefinition
 Folder *FolderMan::addFolderInternal(
     FolderDefinition folderDefinition,
     AccountState *accountState,
-    Vfs *vfs)
+    std::unique_ptr<Vfs> vfs)
 {
     auto alias = folderDefinition.alias;
     int count = 0;
@@ -1036,7 +1035,7 @@ Folder *FolderMan::addFolderInternal(
         folderDefinition.alias = alias + QString::number(++count);
     }
 
-    auto folder = new Folder(folderDefinition, accountState, vfs, this);
+    auto folder = new Folder(folderDefinition, accountState, std::move(vfs), this);
 
     if (_navigationPaneHelper.showInExplorerNavigationPane() && folderDefinition.navigationPaneClsid.isNull()) {
         folder->setNavigationPaneClsid(QUuid::createUuid());

diff --git a/src/gui/folderman.h b/src/gui/folderman.h
index bc49167b00375f5bae48bdf9622fac544b567aed..75cbd2307cf58f82cb79a119b5b809831a0b8979 100644 (file)
--- a/src/gui/folderman.h
+++ b/src/gui/folderman.h
@@ -293,7 +293,7 @@ private:
      *  does not set an account on the new folder.
       */
     Folder *addFolderInternal(FolderDefinition folderDefinition,
-        AccountState *accountState, Vfs *vfs);
+        AccountState *accountState, std::unique_ptr<Vfs> vfs);
 
     /* unloads a folder object, does not delete it */
     void unloadFolder(Folder *);

diff --git a/test/testsyncvirtualfiles.cpp b/test/testsyncvirtualfiles.cpp
index a90f006c96fe15040a6e921a37f4fa0b55492785..d647d58a8b71df81eb18548485c86e188cc801bd 100644 (file)
--- a/test/testsyncvirtualfiles.cpp
+++ b/test/testsyncvirtualfiles.cpp
@@ -62,7 +62,8 @@ void markForDehydration(FakeFolder &folder, const QByteArray &path)
 SyncOptions vfsSyncOptions()
 {
     SyncOptions options;
-    options._vfs = createVfsFromPlugin(Vfs::WithSuffix, nullptr);
+    // leak here
+    options._vfs = createVfsFromPlugin(Vfs::WithSuffix).release();
     return options;
 }