From 8b14e56f88a378ae9e2a16b00a3c4147f92797dc Mon Sep 17 00:00:00 2001
From: James Turner <zakalawe@mac.com>
Date: Sat, 22 Aug 2020 21:27:38 +0100
Subject: [PATCH] HTTPRepo: Fix ownership of HTTPDirectory

---
 simgear/io/HTTPRepository.cxx | 37 ++++++++++++-----------------------
 1 file changed, 13 insertions(+), 24 deletions(-)

diff --git a/simgear/io/HTTPRepository.cxx b/simgear/io/HTTPRepository.cxx
index b3f6e71e..a396a2bb 100644
--- a/simgear/io/HTTPRepository.cxx
+++ b/simgear/io/HTTPRepository.cxx
@@ -49,6 +49,7 @@ namespace simgear
 {
 
     class HTTPDirectory;
+    using HTTPDirectory_ptr = std::unique_ptr<HTTPDirectory>;
 
     class HTTPRepoGetRequest : public HTTP::Request
     {
@@ -137,7 +138,7 @@ public:
     SGPath basePath;
     bool isUpdating;
     HTTPRepository::ResultCode status;
-    HTTPDirectory* rootDir;
+    HTTPDirectory_ptr rootDir;
     size_t totalDownloaded;
 
     HTTP::Request_ptr updateFile(HTTPDirectory* dir, const std::string& name,
@@ -165,7 +166,7 @@ public:
     HTTPDirectory* getOrCreateDirectory(const std::string& path);
     bool deleteDirectory(const std::string& relPath, const SGPath& absPath);
 
-    typedef std::vector<HTTPDirectory*> DirectoryVector;
+    typedef std::vector<HTTPDirectory_ptr> DirectoryVector;
     DirectoryVector directories;
 
     SGPath installedCopyPath;
@@ -597,7 +598,7 @@ private:
     void removeChild(SGPath path)
     {
         bool ok;
-        SG_LOG(SG_TERRASYNC, SG_WARN, "Removing:" << path);
+        SG_LOG(SG_TERRASYNC, SG_INFO, "Removing:" << path);
 
         std::string fpath = _relativePath + "/" + path.file();
         if (path.isDir()) {
@@ -631,14 +632,7 @@ HTTPRepository::HTTPRepository(const SGPath& base, HTTP::Client *cl) :
 {
     _d->http = cl;
     _d->basePath = base;
-    // REVIEW: Memory Leak - 885,598 (3,072 direct, 882,526 indirect) bytes in 48 blocks are definitely lost
-    //      _d->rootDir should be changed to a shared_ptr
-    //      this object is passed to several chained methods, so destruction within ~HTTPRepoPrivate may be
-    //      an easier fix assuming sole ownership would apply
-    // REVIEW: _d is only partially initialized when it is being passed to the HTTPDirectory ctor.
-    //      The HTTPDirectory object is taking ownership of the shared HTTPRepoPrivate object.
-    //      Mismatch between _d being a unique_ptr, yet also cloned into HTTPDirectory.
-    _d->rootDir = new HTTPDirectory(_d.get(), "");
+    _d->rootDir.reset(new HTTPDirectory(_d.get(), ""));
     _d->parseHashCache();
 }
 
@@ -675,7 +669,7 @@ void HTTPRepository::update()
     _d->status = REPO_NO_ERROR;
     _d->isUpdating = true;
     _d->failures.clear();
-    _d->updateDir(_d->rootDir, std::string(), 0);
+    _d->updateDir(_d->rootDir.get(), {}, 0);
 }
 
 bool HTTPRepository::isDoingSync() const
@@ -943,10 +937,7 @@ HTTPRepository::failure() const
             http->cancelRequest(*rq, "Repository object deleted");
         }
 
-        DirectoryVector::iterator it;
-        for (it=directories.begin(); it != directories.end(); ++it) {
-            delete *it;
-        }
+        directories.clear(); // wil delete them all
     }
 
     HTTP::Request_ptr HTTPRepoPrivate::updateFile(HTTPDirectory* dir, const std::string& name, size_t sz)
@@ -1109,7 +1100,7 @@ HTTPRepository::failure() const
     {
     public:
         DirectoryWithPath(const std::string& p) : path(p) {}
-        bool operator()(const HTTPDirectory* entry) const
+        bool operator()(const HTTPDirectory_ptr& entry) const
         { return entry->relativePath() == path; }
     private:
         std::string path;
@@ -1118,14 +1109,13 @@ HTTPRepository::failure() const
     HTTPDirectory* HTTPRepoPrivate::getOrCreateDirectory(const std::string& path)
     {
         DirectoryWithPath p(path);
-        DirectoryVector::iterator it = std::find_if(directories.begin(), directories.end(), p);
+        auto it = std::find_if(directories.begin(), directories.end(), p);
         if (it != directories.end()) {
-            return *it;
+            return it->get();
         }
 
-        HTTPDirectory* d = new HTTPDirectory(this, path);
-        directories.push_back(d);
-        return d;
+        directories.emplace_back(new HTTPDirectory(this, path));
+        return directories.back().get();
     }
 
     bool HTTPRepoPrivate::deleteDirectory(const std::string& relPath, const SGPath& absPath)
@@ -1133,10 +1123,9 @@ HTTPRepository::failure() const
         DirectoryWithPath p(relPath);
         auto it = std::find_if(directories.begin(), directories.end(), p);
         if (it != directories.end()) {
-            HTTPDirectory* d = *it;
+            HTTPDirectory* d = it->get();
 			assert(d->absolutePath() == absPath);
             directories.erase(it);
-            delete d;
 		} else {
 			// we encounter this code path when deleting an orphaned directory
 		}