Fix LinkManager bugs found by unit test

Largest change is that once a link is added to LinkManager it maintains
ownership and is responsible for delete. If you need to delete a link
use LinkManager::delete. Also added guard code to assert if a Link is
deleted outside of LinkManager.

src/comm/LinkManager.cc

@@ -63,8 +63,8 @@ void LinkManager::deleteInstance(void)
  *
  * This class implements the singleton design pattern and has therefore only a private constructor.
  **/
-LinkManager::LinkManager(QObject* parent) :
-    QGCSingleton(parent),
+LinkManager::LinkManager(QObject* parent, bool registerSingleton) :
+    QGCSingleton(parent, registerSingleton),
     _connectionsSuspended(false)
 {
     _links = QList<LinkInterface*>();
@@ -75,24 +75,23 @@ LinkManager::~LinkManager()
 {
     disconnectAll();
     
-    _dataMutex.lock();
     foreach (LinkInterface* link, _links) {
         Q_ASSERT(link);
-        link->deleteLater();
+        deleteLink(link);
     }
     _links.clear();
-    _dataMutex.unlock();
 }
 
 void LinkManager::add(LinkInterface* link)
 {
     Q_ASSERT(link);
     
+    // Take ownership for delete
+    link->_ownedByLinkManager = true;
+    
     _dataMutex.lock();
     
-    if (!_links.contains(link))
-    {
-        connect(link, SIGNAL(destroyed(QObject*)), this, SLOT(_removeLink(QObject*)));
+    if (!_links.contains(link)) {
         _links.append(link);
         _dataMutex.unlock();
         emit newLink(link);
@@ -174,7 +173,7 @@ bool LinkManager::disconnectAll()
     foreach (LinkInterface* link, _links)
     {
         Q_ASSERT(link);
-        if (!disconnectLink(link)) {
+        if (!link->_disconnect()) {
             allDisconnected = false;
         }
     }
@@ -200,56 +199,30 @@ bool LinkManager::disconnectLink(LinkInterface* link)
     return link->_disconnect();
 }
 
-void LinkManager::_removeLink(QObject* obj)
+void LinkManager::deleteLink(LinkInterface* link)
 {
-    // Be careful of the fact that by the time this signal makes it through the queue
-    // the link object has already been destructed.
+    Q_ASSERT(link);
     
-    Q_ASSERT(obj);
+    _dataMutex.lock();
     
-    LinkInterface* link = static_cast<LinkInterface*>(obj);
+    Q_ASSERT(_links.contains(link));
+    _links.removeOne(link);
+    Q_ASSERT(!_links.contains(link));
 
-    _dataMutex.lock();
-    for (int i=0; i < _links.size(); i++)
-    {
-        if(link==_links.at(i))
-        {
-            _links.removeAt(i); //remove from link list
-        }
-    }
     // Remove link from protocol map
     QList<ProtocolInterface* > protocols = _protocolLinks.keys(link);
-    foreach (ProtocolInterface* proto, protocols)
-    {
+    foreach (ProtocolInterface* proto, protocols) {
         _protocolLinks.remove(proto, link);
     }
+             
     _dataMutex.unlock();
              
     // Emit removal of link
-    emit linkRemoved(link);
-}
+    emit linkDeleted(link);
     
-/**
- * The access time is linear in the number of links.
- *
- * @param id link identifier to search for
- * @return A pointer to the link or NULL if not found
- */
-LinkInterface* LinkManager::getLinkForId(int id)
-{
-    _dataMutex.lock();
-    LinkInterface* linkret = NULL;
-    foreach (LinkInterface* link, _links)
-    {
-        Q_ASSERT(link);
-        
-        if (link->getId() == id)
-        {
-            linkret = link;
-        }
-    }
-    _dataMutex.unlock();
-    return linkret;
+    Q_ASSERT(link->_ownedByLinkManager);
+    link->_deletedByLinkManager = true;   // Signal that this is a valid delete
+    delete link;
 }
 
 /**
@@ -301,4 +274,3 @@ void LinkManager::setConnectionsSuspended(QString reason)
     _connectionsSuspendedReason = reason;
     Q_ASSERT(!reason.isEmpty());
 }
-

src/comm/LinkManager.h

@@ -42,6 +42,8 @@ This file is part of the PIXHAWK project
 #include "ProtocolInterface.h"
 #include "QGCSingleton.h"
 
+class LinkManagerTest;
+
 /**
  * The Link Manager organizes the physical Links. It can manage arbitrary
  * links and takes care of connecting them as well assigning the correct
@@ -60,15 +62,10 @@ public:
 
     ~LinkManager();
 
-    void run();
-
     QList<LinkInterface*> getLinksForProtocol(ProtocolInterface* protocol);
 
     ProtocolInterface* getProtocolForLink(LinkInterface* link);
 
-    /** @brief Get the link for this id */
-    LinkInterface* getLinkForId(int id);
-
     /** @brief Get a list of all links */
     const QList<LinkInterface*> getLinks();
 
@@ -87,10 +84,14 @@ public:
     /// @brief Sets the flag to allow new connections to be made
     void setConnectionsAllowed(void) { _connectionsSuspended = false; }
     
+    /// @brief Deletes the specified link. Will disconnect if connected.
+    void deleteLink(LinkInterface* link);
+
 public slots:
-    /// @brief Adds the link to the LinkManager. No need to remove a link you added. Just delete it and LinkManager will pick up on
-    ///         that and remove the link from the list.
+    /// @brief Adds the link to the LinkManager. LinkManager takes ownership of this object. To delete
+    //          it, call LinkManager::deleteLink.
     void add(LinkInterface* link);
+    
     void addProtocol(LinkInterface* link, ProtocolInterface* protocol);
 
     bool connectAll();
@@ -101,15 +102,16 @@ public slots:
 
 signals:
     void newLink(LinkInterface* link);
-    void linkRemoved(LinkInterface* link);
+    void linkDeleted(LinkInterface* link);
     
 private:
     /// @brief All access to LinkManager is through LinkManager::instance
-    LinkManager(QObject* parent = NULL);
+    LinkManager(QObject* parent = NULL, bool registerSingleton = true);
     
-    static LinkManager* _instance;
+    // LinkManager unit test is allowed to new LinkManager objects
+    friend class LinkManagerTest;
     
-    void _removeLink(QObject* obj);
+    static LinkManager* _instance;
     
     QList<LinkInterface*> _links;
     QMultiMap<ProtocolInterface*,LinkInterface*> _protocolLinks;