FdoSecrets: simplify collection internal states

This gets rid of the m_registered state, so whenever there is a valid m_backend, it is guaranteed to be registered already.

While at it, this commit also improves DBusObject::registerWithPath a little bit by allowing properly registering multiple paths using the same adaptor, mostly for supporting Collection aliases.

Now when DBus registration fails, the code does not go into an inconsistent state or crash.

src/fdosecrets/FdoSecretsPlugin.cpp

@@ -104,6 +104,7 @@ void FdoSecretsPlugin::emitRequestShowNotification(const QString& msg, const QSt
 void FdoSecretsPlugin::emitError(const QString& msg)
 {
     emit error(tr("<b>Fdo Secret Service:</b> %1").arg(msg));
+    qDebug() << msg;
 }
 
 QString FdoSecretsPlugin::reportExistingService() const

src/fdosecrets/objects/Collection.cpp

@@ -48,14 +48,13 @@ namespace FdoSecrets
     }
 
     Collection::Collection(Service* parent, DatabaseWidget* backend)
-        : DBusObject(parent)
+        : DBusObjectHelper(parent)
         , m_backend(backend)
         , m_exposedGroup(nullptr)
-        , m_registered(false)
     {
         // whenever the file path or the database object itself change, we do a full reload.
-        connect(backend, &DatabaseWidget::databaseFilePathChanged, this, &Collection::reloadBackend);
-        connect(backend, &DatabaseWidget::databaseReplaced, this, &Collection::reloadBackend);
+        connect(backend, &DatabaseWidget::databaseFilePathChanged, this, &Collection::reloadBackendOrDelete);
+        connect(backend, &DatabaseWidget::databaseReplaced, this, &Collection::reloadBackendOrDelete);
 
         // also remember to clear/populate the database when lock state changes.
         connect(backend, &DatabaseWidget::databaseUnlocked, this, &Collection::onDatabaseLockChanged);
@@ -72,33 +71,24 @@ namespace FdoSecrets
 
     bool Collection::reloadBackend()
     {
-        if (m_registered) {
-            // delete all items
-            // this has to be done because the backend is actually still there, just we don't expose them
-            // NOTE: Do NOT use a for loop, because Item::doDelete will remove itself from m_items.
-            while (!m_items.isEmpty()) {
-                m_items.first()->doDelete();
-            }
-            cleanupConnections();
+        Q_ASSERT(m_backend);
 
-            unregisterCurrentPath();
-            m_registered = false;
+        // delete all items
+        // this has to be done because the backend is actually still there, just we don't expose them
+        // NOTE: Do NOT use a for loop, because Item::doDelete will remove itself from m_items.
+        while (!m_items.isEmpty()) {
+            m_items.first()->doDelete();
         }
-
-        Q_ASSERT(m_backend);
+        cleanupConnections();
+        unregisterPrimaryPath();
 
         // make sure we have updated copy of the filepath, which is used to identify the database.
         m_backendPath = m_backend->database()->filePath();
 
-        // the database may not have a name (derived from filePath) yet, which may happen if it's newly created.
-        // defer the registration to next time a file path change happens.
-        if (!name().isEmpty()) {
-            auto path = QStringLiteral(DBUS_PATH_TEMPLATE_COLLECTION).arg(p()->objectPath().path(), encodePath(name()));
-            if (!registerWithPath(path, new CollectionAdaptor(this))) {
-                service()->plugin()->emitError(tr("Failed to register database on DBus under the name '%1'").arg(name()));
-                return false;
-            }
-            m_registered = true;
+        auto path = QStringLiteral(DBUS_PATH_TEMPLATE_COLLECTION).arg(p()->objectPath().path(), encodePath(name()));
+        if (!registerWithPath(path)) {
+            service()->plugin()->emitError(tr("Failed to register database on DBus under the name '%1'").arg(name()));
+            return false;
         }
 
         // populate contents after expose on dbus, because items rely on parent's dbus object path
@@ -111,6 +101,13 @@ namespace FdoSecrets
         return true;
     }
 
+    void Collection::reloadBackendOrDelete()
+    {
+        if (!reloadBackend()) {
+            doDelete();
+        }
+    }
+
     DBusReturn<void> Collection::ensureBackend() const
     {
         if (!m_backend) {
@@ -418,8 +415,7 @@ namespace FdoSecrets
 
         emit aliasAboutToAdd(alias);
 
-        bool ok = QDBusConnection::sessionBus().registerObject(
-            QStringLiteral(DBUS_PATH_TEMPLATE_ALIAS).arg(p()->objectPath().path(), alias), this);
+        bool ok = registerWithPath(QStringLiteral(DBUS_PATH_TEMPLATE_ALIAS).arg(p()->objectPath().path(), alias), false);
         if (ok) {
             m_aliases.insert(alias);
             emit aliasAdded(alias);
@@ -454,6 +450,7 @@ namespace FdoSecrets
 
     QString Collection::name() const
     {
+        // todo: make sure the name is never empty
         if (m_backendPath.isEmpty()) {
             // This is a newly created db without saving to file.
             // This name is also used to register dbus path.
@@ -486,7 +483,7 @@ namespace FdoSecrets
 
     void Collection::populateContents()
     {
-        if (!m_registered) {
+        if (!m_backend) {
             return;
         }
 
@@ -645,7 +642,7 @@ namespace FdoSecrets
 
         emit collectionAboutToDelete();
 
-        unregisterCurrentPath();
+        unregisterPrimaryPath();
 
         // remove alias manually to trigger signal
         for (const auto& a : aliases()) {
@@ -663,6 +660,8 @@ namespace FdoSecrets
         // reset backend and delete self
         m_backend = nullptr;
         deleteLater();
+
+        // items will be removed automatically as they are children objects
     }
 
     void Collection::cleanupConnections()

src/fdosecrets/objects/Collection.h

@@ -36,7 +36,7 @@ namespace FdoSecrets
     class Item;
     class PromptBase;
     class Service;
-    class Collection : public DBusObject
+    class Collection : public DBusObjectHelper<Collection, CollectionAdaptor>
     {
         Q_OBJECT
 
@@ -124,9 +124,13 @@ namespace FdoSecrets
     private slots:
         void onDatabaseLockChanged();
         void onDatabaseExposedGroupChanged();
+
         // force reload info from backend, potentially delete self
         bool reloadBackend();
 
+        // calls reloadBackend, delete self when error
+        void reloadBackendOrDelete();
+
     private:
         friend class DeleteCollectionPrompt;
         friend class CreateCollectionPrompt;
@@ -165,8 +169,6 @@ namespace FdoSecrets
         QSet<QString> m_aliases;
         QList<Item*> m_items;
         QMap<const Entry*, Item*> m_entryToItem;
-
-        bool m_registered;
     };
 
 } // namespace FdoSecrets

src/fdosecrets/objects/DBusObject.cpp

@@ -17,15 +17,12 @@
 
 #include "DBusObject.h"
 
-#include <QDBusAbstractAdaptor>
 #include <QFile>
 #include <QRegularExpression>
 #include <QTextStream>
 #include <QUrl>
 #include <QUuid>
 
-#include <utility>
-
 namespace FdoSecrets
 {
 
@@ -35,14 +32,13 @@ namespace FdoSecrets
     {
     }
 
-    bool DBusObject::registerWithPath(const QString& path, QDBusAbstractAdaptor* adaptor)
+    bool DBusObject::registerWithPath(const QString& path, bool primary)
     {
-        Q_ASSERT(!m_dbusAdaptor);
+        if (primary) {
+            m_objectPath.setPath(path);
+        }
 
-        m_objectPath.setPath(path);
-        m_dbusAdaptor = adaptor;
-        adaptor->setParent(this);
-        return QDBusConnection::sessionBus().registerObject(m_objectPath.path(), this);
+        return QDBusConnection::sessionBus().registerObject(path, this);
     }
 
     QString DBusObject::callingPeerName() const
@@ -58,7 +54,10 @@ namespace FdoSecrets
 
     QString encodePath(const QString& value)
     {
-        // force "-.~_" to be encoded
+        // toPercentEncoding encodes everything except those in the unreserved group:
+        // ALPHA / DIGIT / "-" / "." / "_" / "~"
+        // we want only ALPHA / DIGIT / "_", with "_" as the escape character
+        // so force "-.~_" to be encoded
         return QUrl::toPercentEncoding(value, "", "-.~_").replace('%', '_');
     }
 

src/fdosecrets/objects/DBusObject.h

@@ -21,6 +21,7 @@
 #include "fdosecrets/objects/DBusReturn.h"
 #include "fdosecrets/objects/DBusTypes.h"
 
+#include <QDBusAbstractAdaptor>
 #include <QDBusConnection>
 #include <QDBusConnectionInterface>
 #include <QDBusContext>
@@ -31,21 +32,19 @@
 #include <QObject>
 #include <QScopedPointer>
 
-class QDBusAbstractAdaptor;
-
 namespace FdoSecrets
 {
     class Service;
 
     /**
-     * @brief A common base class for all dbus-exposed objects
+     * @brief A common base class for all dbus-exposed objects.
+     * However, derived class should inherit from `DBusObjectHelper`, which is
+     * the only way to set DBus adaptor and enforces correct adaptor creation.
      */
     class DBusObject : public QObject, public QDBusContext
     {
         Q_OBJECT
     public:
-        explicit DBusObject(DBusObject* parent = nullptr);
-
         const QDBusObjectPath& objectPath() const
         {
             return m_objectPath;
@@ -57,12 +56,21 @@ namespace FdoSecrets
         }
 
     protected:
-        bool registerWithPath(const QString& path, QDBusAbstractAdaptor* adaptor);
+        /**
+         * @brief Register this object at given DBus path
+         * @param path DBus path to register at
+         * @param primary whether this path to be considered primary. The primary path is the one to be returned by
+         * `DBusObject::objectPath`.
+         * @return true on success
+         */
+        bool registerWithPath(const QString& path, bool primary = true);
 
-        void unregisterCurrentPath()
+        void unregisterPrimaryPath()
         {
+            if (m_objectPath.path() == QStringLiteral("/")) {
+                return;
+            }
             QDBusConnection::sessionBus().unregisterObject(m_objectPath.path());
-            m_dbusAdaptor = nullptr;
             m_objectPath.setPath(QStringLiteral("/"));
         }
 
@@ -85,19 +93,35 @@ namespace FdoSecrets
         }
 
     private:
+        explicit DBusObject(DBusObject* parent);
+
         /**
          * Derived class should not directly use sendErrorReply.
          * Instead, use raiseError
          */
         using QDBusContext::sendErrorReply;
 
         template <typename U> friend class DBusReturn;
+        template <typename Object, typename Adaptor> friend class DBusObjectHelper;
 
-    private:
         QDBusAbstractAdaptor* m_dbusAdaptor;
         QDBusObjectPath m_objectPath;
     };
 
+    template <typename Object, typename Adaptor> class DBusObjectHelper : public DBusObject
+    {
+    protected:
+        explicit DBusObjectHelper(DBusObject* parent)
+            : DBusObject(parent)
+        {
+            // creating new Adaptor has to be delayed into constructor's body,
+            // and can't be simply moved to initializer list, because at that
+            // point the base QObject class hasn't been initialized and will sigfault.
+            m_dbusAdaptor = new Adaptor(static_cast<Object*>(this));
+            m_dbusAdaptor->setParent(this);
+        }
+    };
+
     /**
      * Return the object path of the pointed DBusObject, or "/" if the pointer is null
      * @tparam T

src/fdosecrets/objects/Item.cpp

@@ -60,7 +60,7 @@ namespace FdoSecrets
     }
 
     Item::Item(Collection* parent, Entry* backend)
-        : DBusObject(parent)
+        : DBusObjectHelper(parent)
         , m_backend(backend)
     {
         Q_ASSERT(!p()->objectPath().path().isEmpty());
@@ -71,7 +71,7 @@ namespace FdoSecrets
     bool Item::registerSelf()
     {
         auto path = QStringLiteral(DBUS_PATH_TEMPLATE_ITEM).arg(p()->objectPath().path(), m_backend->uuidToHex());
-        bool ok = registerWithPath(path, new ItemAdaptor(this));
+        bool ok = registerWithPath(path);
         if (!ok) {
             service()->plugin()->emitError(tr("Failed to register item on DBus at path '%1'").arg(path));
         }
@@ -340,7 +340,7 @@ namespace FdoSecrets
         // Unregister current path early, do not rely on deleteLater's call to destructor
         // as in case of Entry moving between groups, new Item will be created at the same DBus path
         // before the current Item is deleted in the event loop.
-        unregisterCurrentPath();
+        unregisterPrimaryPath();
 
         m_backend = nullptr;
         deleteLater();

src/fdosecrets/objects/Item.h

@@ -38,7 +38,7 @@ namespace FdoSecrets
     class Collection;
     class PromptBase;
 
-    class Item : public DBusObject
+    class Item : public DBusObjectHelper<Item, ItemAdaptor>
     {
         Q_OBJECT
 
@@ -100,7 +100,6 @@ namespace FdoSecrets
     public slots:
         void doDelete();
 
-    private:
         /**
          * @brief Register self on DBus
          * @return

src/fdosecrets/objects/Prompt.cpp

@@ -34,15 +34,15 @@ namespace FdoSecrets
 {
 
     PromptBase::PromptBase(Service* parent)
-        : DBusObject(parent)
+        : DBusObjectHelper(parent)
     {
         connect(this, &PromptBase::completed, this, &PromptBase::deleteLater);
     }
 
     bool PromptBase::registerSelf()
     {
         auto path = QStringLiteral(DBUS_PATH_TEMPLATE_PROMPT).arg(p()->objectPath().path(), Tools::uuidToHex(QUuid::createUuid()));
-        bool ok = registerWithPath(path, new PromptAdaptor(this));
+        bool ok = registerWithPath(path);
         if (!ok) {
             service()->plugin()->emitError(tr("Failed to register item on DBus at path '%1'").arg(path));
         }

src/fdosecrets/objects/Prompt.h

@@ -32,7 +32,7 @@ namespace FdoSecrets
 
     class Service;
 
-    class PromptBase : public DBusObject
+    class PromptBase : public DBusObjectHelper<PromptBase, PromptAdaptor>
     {
         Q_OBJECT
     public: