From: Christian Kamm Date: Thu, 15 Nov 2018 08:45:14 +0000 (+0100) Subject: vfs: Be more careful about Vfs instance ownership X-Git-Tag: archive/raspbian/3.16.7-1_deb13u1+rpi1~1^2~12^2~21^2~468^2~384 X-Git-Url: https://dgit.raspbian.org/?a=commitdiff_plain;h=fa2450cf11bbcffc288fee207088c75b44e1138e;p=nextcloud-desktop.git vfs: Be more careful about Vfs instance ownership --- diff --git a/src/common/vfs.cpp b/src/common/vfs.cpp index 9809ff4a6..961c0c280 100644 --- 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 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(factory->create(parent)); + auto vfs = std::unique_ptr(qobject_cast(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 7d68854d8..28fab2085 100644 --- a/src/common/vfs.h +++ b/src/common/vfs.h @@ -17,6 +17,8 @@ #include #include +#include + #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 createVfsFromPlugin(Vfs::Mode mode); } // namespace OCC diff --git a/src/gui/folder.cpp b/src/gui/folder.cpp index da85ea4b5..adfc75bc1 100644 --- 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, 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 ec62080ce..507fd09f0 100644 --- a/src/gui/folder.h +++ b/src/gui/folder.h @@ -28,6 +28,7 @@ #include #include #include +#include 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, 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; }; } diff --git a/src/gui/folderman.cpp b/src/gui/folderman.cpp index 501d5b7ec..f17f9b402 100644 --- 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()); 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) { 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 bc49167b0..75cbd2307 100644 --- 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); /* unloads a folder object, does not delete it */ void unloadFolder(Folder *); diff --git a/test/testsyncvirtualfiles.cpp b/test/testsyncvirtualfiles.cpp index a90f006c9..d647d58a8 100644 --- 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; }