[DC-193] Make CfAPi locking more granular and always groups siblings together (#12386)

* Make CfApi locks more granular

Also lock when disconnecting/unregistering.

* Always group in the sidebar when that is available

And remove all traces of `groupInSidebar`.

* Only unregister syncroot when it was actually registered before.

---------

Co-authored-by: modSpike <lisa.reese@kiteworks.com>

Author: erikjv

src/common/vfs.cpp

@@ -276,10 +276,9 @@ const VfsPluginManager &VfsPluginManager::instance()
     return *_instance;
 }
 
-VfsSetupParams::VfsSetupParams(Account *account, const QUrl &baseUrl, bool groupInSidebar, SyncEngine *syncEngine)
+VfsSetupParams::VfsSetupParams(Account *account, const QUrl &baseUrl, SyncEngine *syncEngine)
     : account(account)
     , _baseUrl(baseUrl)
-    , _groupInSidebar(groupInSidebar)
     , _syncEngine(syncEngine)
 {
 }

src/common/vfs.h

@@ -44,7 +44,7 @@ class SyncEngine;
 /** Collection of parameters for initializing a Vfs instance. */
 struct OCSYNC_EXPORT VfsSetupParams
 {
-    explicit VfsSetupParams(Account *account, const QUrl &baseUrl, bool groupInSidebar, SyncEngine *syncEngine);
+    explicit VfsSetupParams(Account *account, const QUrl &baseUrl, SyncEngine *syncEngine);
     /** The full path to the folder on the local filesystem
      *
      * Always ends with /.
@@ -81,16 +81,10 @@ struct OCSYNC_EXPORT VfsSetupParams
         return _baseUrl;
     }
 
-    bool groupInSidebar() const
-    {
-        return _groupInSidebar;
-    }
-
     SyncEngine *syncEngine() const;
 
 private:
     QUrl _baseUrl;
-    bool _groupInSidebar = false;
     SyncEngine *_syncEngine;
 };
 

src/gui/folder.cpp

@@ -520,7 +520,7 @@ void Folder::startVfs()
         return;
     }
 
-    VfsSetupParams vfsParams(_accountState->account(), webDavUrl(), groupInSidebar(), _engine.get());
+    VfsSetupParams vfsParams(_accountState->account(), webDavUrl(), _engine.get());
     vfsParams.filesystemPath = path();
     vfsParams.remotePath = remotePathTrailingSlash();
     vfsParams.journal = &_journal;
@@ -1246,15 +1246,4 @@ QString FolderDefinition::displayName() const
     return _displayName;
 }
 
-bool Folder::groupInSidebar() const
-{
-    if (_accountState && _accountState->account() && _accountState->account()->hasDefaultSyncRoot()) {
-        // QFileInfo is horrible and "/foo/" is treated different to "/foo"
-        const QString parentDir = QFileInfo(Utility::stripTrailingSlash(path())).dir().path();
-        // If parentDir == home, we would add the home dir to the sidebar.
-        return QFileInfo(parentDir) != QFileInfo(QDir::homePath()) && FileSystem::isChildPathOf(parentDir, _accountState->account()->defaultSyncRoot());
-    }
-    return false;
-}
-
 } // namespace OCC

src/gui/folder.h

@@ -344,14 +344,6 @@ class OWNCLOUDGUI_EXPORT Folder : public QObject
     /** Whether this folder should show selective sync ui */
     bool supportsSelectiveSync() const;
 
-    /**
-     * Whether to register the parent folder of our sync root in the explorer
-     * The default behaviour is to register alls spaces in a common dir in the home folder
-     * in that case we only display that common dir in the Windows sidebar.
-     * With the legacy behaviour we only have one dir which we will register with Windows
-     */
-    bool groupInSidebar() const;
-
     /**
      * The folder is deployed by an admin
      * We will hide the remove option and the disable/enable vfs option.

src/gui/folderman.cpp

@@ -1150,8 +1150,7 @@ Folder *FolderMan::addFolderFromScratch(AccountState *accountState, FolderDefini
     auto newFolder = addFolder(accountState, folderDefinition);
 
     if (newFolder) {
-        // With spaces we only handle the main folder
-        if (!newFolder->groupInSidebar()) {
+        if (newFolder->vfs().mode() != Vfs::WindowsCfApi) {
             Utility::setupFavLink(folderDefinition.localPath());
         }
         qCDebug(lcFolderMan) << "Local sync folder" << folderDefinition.localPath() << "successfully created!";

src/plugins/vfs/win/vfs_win.cpp

@@ -517,6 +517,8 @@ QString VfsWinPrivate::syncRootPath() const
     return QDir::toNativeSeparators(Utility::stripTrailingSlash(q->params().filesystemPath));
 }
 
+QMutex VfsWinPrivate::registrationMutex;
+
 // Register as a sync provider with cfapi as well as with the explorer integration
 void VfsWinPrivate::registerFolder(const VfsSetupParams &params)
 {
@@ -607,7 +609,7 @@ void VfsWinPrivate::registerFolder(const VfsSetupParams &params)
     // A Uri to a cloud storage recycle bin.
     //providerInfo.RecycleBinUri(L"recycle bin");
 
-    providerInfo.ShowSiblingsAsGroup(params.groupInSidebar());
+    providerInfo.ShowSiblingsAsGroup(true);
 
     // Prepage the key from the shell property store we'll use to determine
     // availability.
@@ -642,14 +644,6 @@ void VfsWinPrivate::registerFolder(const VfsSetupParams &params)
             return;
         }
 
-        // Code below this point cannot run in parallel: the calls below will first search for existing sync root
-        // information, and as a second step it will register the new provider for the folder. If multiple
-        // sync connections are being created in short order (e.g. when setting up a new account), it is possible
-        // that a second call to `registerFolder` will start running this registration. This can result in
-        // multiple side-bar entries in the windows explorer.
-        static QMutex registrationMutex;
-        QMutexLocker registrationLock(&registrationMutex);
-
         try {
             auto previousInfo = StorageProviderSyncRootManager::GetSyncRootInformationForFolder(f);
             const QString oldId = hstringToQString(previousInfo.Id());
@@ -699,6 +693,7 @@ void VfsWinPrivate::registerFolder(const VfsSetupParams &params)
         }
 
         try {
+            QMutexLocker registrationLock(&registrationMutex);
             StorageProviderSyncRootManager::Register(providerInfo);
         } catch (winrt::hresult_error const& ex) {
             qCWarning(lcVfs) << "Error registering StorageProvider for" << params.filesystemPath << QString::number(ex.code()) << hstringToQString(ex.message());
@@ -727,13 +722,15 @@ void VfsWinPrivate::registerFolder(const VfsSetupParams &params)
         };
         // clang-format on
 
-        HRESULT ok = CfConnectSyncRoot(
-            syncRootW.c_str(),
-            callbacks,
-            this, // use VfsWinPrivate as callback context
-            CF_CONNECT_FLAG_BLOCK_SELF_IMPLICIT_HYDRATION // don't let antivirus implicitly hydrate (or something, a bit unclear)
-                | CF_CONNECT_FLAG_REQUIRE_FULL_FILE_PATH, // request that callbacks get the full path
-            &_connectionKey);
+        HRESULT ok;
+        {
+            QMutexLocker registrationLock(&registrationMutex);
+            ok = CfConnectSyncRoot(syncRootW.c_str(), callbacks,
+                this, // use VfsWinPrivate as callback context
+                CF_CONNECT_FLAG_BLOCK_SELF_IMPLICIT_HYDRATION // don't let antivirus implicitly hydrate (or something, a bit unclear)
+                    | CF_CONNECT_FLAG_REQUIRE_FULL_FILE_PATH, // request that callbacks get the full path
+                &_connectionKey);
+        }
         if (FAILED(ok)) {
             Q_EMIT q->error(tr("Unable to connect sync root: %1 error: %2").arg(syncRoot, Utility::formatWinError(ok)));
             return;
@@ -809,20 +806,18 @@ void VfsWin::stop()
         wait.processEvents(QEventLoop::AllEvents, 100);
     }
 
+    QMutexLocker registrationLock(&d->registrationMutex);
     CfDisconnectSyncRoot(d->_connectionKey);
 }
 
 void VfsWin::unregisterFolder()
 {
     Q_D(VfsWin);
-    using SyncRootManager = winrt::Windows::Storage::Provider::StorageProviderSyncRootManager;
-    try {
-        SyncRootManager::Unregister(d->_registrationId);
-    } catch (winrt::hresult_error const &ex) {
-        qCWarning(lcVfs) << "Could not Unregister() sync root:" << hstringToQString(ex.message());
-    }
 
-    // Remove the folder from the search index
+    if (d->_registrationState == VfsWinPrivate::Unregistered)
+        return;
+
+    // First step: remove the folder from the search index
     const std::wstring url(convertFullNativePathToFullyDecodedUrlW(d->syncRootPath()));
     winrt::com_ptr<ISearchCrawlScopeManager> searchCrawlScopeManager = getSearchCrawlScopeManager();
 
@@ -835,6 +830,15 @@ void VfsWin::unregisterFolder()
     } else {
         qCWarning(lcVfs) << "Unable to get the SearchCrawlScopeManager to remove the path" << url;
     }
+
+    // Final step: unregister the path. We're now completely done with it.
+    using SyncRootManager = winrt::Windows::Storage::Provider::StorageProviderSyncRootManager;
+    try {
+        QMutexLocker registrationLock(&d->registrationMutex);
+        SyncRootManager::Unregister(d->_registrationId);
+    } catch (winrt::hresult_error const &ex) {
+        qCWarning(lcVfs) << "Could not Unregister() sync root:" << hstringToQString(ex.message());
+    }
 }
 
 OCC::Result<OCC::Vfs::ConvertToPlaceholderResult, QString> VfsWin::updateMetadata(const SyncFileItem &item, const QString &filePath, const QString &replacesFile)

src/plugins/vfs/win/vfs_win_p.h

@@ -89,6 +89,7 @@ class VfsWinPrivate : public QObject
     VfsWin *q;
     QAtomicInt _registrationState;
     CF_CONNECTION_KEY _connectionKey;
+    static QMutex registrationMutex;
 
     // System.StorageProviderState key
     std::unique_ptr<PROPERTYKEY> storageProviderStateKey;

test/testutils/syncenginetestutils.cpp

@@ -914,32 +914,28 @@ FakeFolder::FakeFolder(const FileInfo &fileTemplate, OCC::Vfs::Mode vfsMode, boo
     OC_ENFORCE(syncOnce())
 }
 
-FakeFolder::~FakeFolder()
-{
-    auto opts = _syncEngine->syncOptions();
-    opts._vfs->stop();
-    opts._vfs->unregisterFolder(); // Important! This removes the side-bar entry in Windows Explorer!
-}
+FakeFolder::~FakeFolder() { }
 
 void FakeFolder::switchToVfs(QSharedPointer<OCC::Vfs> vfs)
 {
     auto opts = _syncEngine->syncOptions();
 
     opts._vfs->stop();
+    opts._vfs->unregisterFolder();
     QObject::disconnect(_syncEngine.get(), nullptr, opts._vfs.data(), nullptr);
 
     opts._vfs = vfs;
     _syncEngine->setSyncOptions(opts);
 
-    OCC::VfsSetupParams vfsParams(account(), account()->davUrl(), false, &syncEngine());
+    OCC::VfsSetupParams vfsParams(account(), account()->davUrl(), &syncEngine());
     vfsParams.filesystemPath = localPath();
     vfsParams.remotePath = QLatin1Char('/');
     vfsParams.journal = _journalDb.get();
     vfsParams.providerName = QStringLiteral("OC-TEST");
     vfsParams.providerDisplayName = QStringLiteral("OC-TEST");
     vfsParams.providerVersion = QVersionNumber(0, 1, 0);
     vfsParams.multipleAccountsRegistered = false;
-    QObject::connect(_syncEngine.get(), &QObject::destroyed, vfs.data(), [vfs]() {
+    QObject::connect(_syncEngine.get(), &QObject::destroyed, this, [vfs]() {
         vfs->stop();
         vfs->unregisterFolder();
     });