Cleanup Download code, fixing SDL_Thread leaking

The download thread was setting itself to nullptr (d->mThread = nullptr)
in a number of locations. This caused a later call to SDL_WaitThread to
be unable to perform cleanup.

This reverts most of 1eb02f83a5d3895e4e18db30ea10d88da94ba4c0 (including
making Download::cancel no longer blocking), but keeps the necessary
waiting for the thread to finish before freeing the memory buffer in
~UpdaterWindow(), which might have been the bug fixed by that change.

Fixed removal of downloaded .part file when its checksum failed. It
trying to remove the file without .part appended instead.

Fixed download progress indication in ServerDialog to not be reversed,
though this is rarely visible due to the server list being so small.

Fixed reporting of curl error.

4 files changed, 55 insertions, 86 deletions

diff --git a/src/gui/serverdialog.cpp b/src/gui/serverdialog.cpp
index 01b477da..90469662 100644
--- a/src/gui/serverdialog.cpp
+++ b/src/gui/serverdialog.cpp
@@ -50,8 +50,6 @@
 #include <cstdlib>
 #include <string>
 
-static const int MAX_SERVERLIST = 6;
-
 ServersListModel::ServersListModel(ServerInfos *servers, ServerDialog *parent):
         mServers(servers),
         mVersionStrings(servers->size(), VersionString(0, std::string())),
@@ -141,7 +139,6 @@ public:
             if (info.version.first > 0)
             {
                 graphics->setColor(unsupported);
-
                 graphics->drawText(info.version.second,
                                    getWidth() - info.version.first - 2, top);
             }
@@ -231,9 +228,11 @@ ServerDialog::~ServerDialog()
     if (mDownload)
     {
         mDownload->cancel();
-        delete mDownload;
-        mDownload = nullptr;
+
+        // Make sure thread is gone before deleting the ServersListModel
+        mDownload.reset();
     }
+
     delete mServersListModel;
 }
 
@@ -262,6 +261,7 @@ void ServerDialog::action(const gcn::ActionEvent &event)
         else
         {
             mDownload->cancel();
+
             mQuitButton->setEnabled(false);
             mConnectButton->setEnabled(false);
             mDeleteButton->setEnabled(false);
@@ -374,7 +374,7 @@ void ServerDialog::logic()
         else if (mDownloadStatus == DOWNLOADING_IN_PROGRESS)
         {
             mDownloadText->setCaption(strprintf(_("Downloading server list..."
-                                                 "%2.2f%%"),
+                                                 "%2.0f%%"),
                                       mDownloadProgress * 100));
         }
         else if (mDownloadStatus == DOWNLOADING_IDLE)
@@ -406,7 +406,8 @@ void ServerDialog::downloadServerList()
     if (listFile.empty())
         listFile = "https://www.manasource.org/serverlist.xml";
 
-    mDownload = new Net::Download(this, listFile, &downloadUpdate);
+    mDownload = std::make_unique<Net::Download>(this, listFile,
+                                                &ServerDialog::downloadUpdate);
     mDownload->setFile(mDir + "/serverlist.xml");
     mDownload->start();
 }
@@ -494,7 +495,6 @@ void ServerDialog::loadServers()
         server.version.first = gui->getFont()->getWidth(version);
         server.version.second = version;
 
-        MutexLocker lock(&mMutex);
         // Add the server to the local list if it's not already present
         bool found = false;
         int i = 0;
@@ -562,17 +562,18 @@ void ServerDialog::saveCustomServers(const ServerInfo &currentServer, int index)
 }
 
 int ServerDialog::downloadUpdate(void *ptr, DownloadStatus status,
-                                 size_t total, size_t remaining)
+                                 size_t dltotal, size_t dlnow)
 {
     if (status == DOWNLOAD_STATUS_CANCELLED)
         return -1;
 
     auto *sd = reinterpret_cast<ServerDialog*>(ptr);
-    bool finished = false;
+    MutexLocker lock(&sd->mMutex);
 
     if (status == DOWNLOAD_STATUS_COMPLETE)
     {
-        finished = true;
+        sd->loadServers();
+        sd->mDownloadStatus = DOWNLOADING_COMPLETE;
     }
     else if (status < 0)
     {
@@ -582,27 +583,13 @@ int ServerDialog::downloadUpdate(void *ptr, DownloadStatus status,
     }
     else
     {
-        float progress = (float) remaining / total;
+        float progress = 0.0f;
+        if (dltotal > 0)
+            progress = static_cast<float>(dlnow) / dltotal;
 
-        if (progress != progress)
-            progress = 0.0f; // check for NaN
-        else if (progress < 0.0f)
-            progress = 0.0f; // no idea how this could ever happen, but why not check for it anyway.
-        else if (progress > 1.0f)
-            progress = 1.0f;
-
-        MutexLocker lock(&sd->mMutex);
         sd->mDownloadStatus = DOWNLOADING_IN_PROGRESS;
         sd->mDownloadProgress = progress;
     }
 
-    if (finished)
-    {
-        sd->loadServers();
-
-        MutexLocker lock(&sd->mMutex);
-        sd->mDownloadStatus = DOWNLOADING_COMPLETE;
-    }
-
     return 0;
 }
diff --git a/src/gui/serverdialog.h b/src/gui/serverdialog.h
index b61689bd..bf2a9512 100644
--- a/src/gui/serverdialog.h
+++ b/src/gui/serverdialog.h
@@ -33,6 +33,7 @@
 #include <guichan/listmodel.hpp>
 #include <guichan/selectionlistener.hpp>
 
+#include <memory>
 #include <string>
 #include <vector>
 
@@ -131,7 +132,7 @@ class ServerDialog : public Window,
         void loadCustomServers();
 
         static int downloadUpdate(void *ptr, DownloadStatus status,
-                                  size_t total, size_t remaining);
+                                  size_t dltotal, size_t dlnow);
 
         Label  *mDescription;
         Button *mQuitButton;
@@ -158,11 +159,11 @@ class ServerDialog : public Window,
         /** Status of the current download. */
         ServerDialogDownloadStatus mDownloadStatus = DOWNLOADING_PREPARING;
 
-        Net::Download *mDownload = nullptr;
+        std::unique_ptr<Net::Download> mDownload;
         Label *mDownloadText;
 
         Mutex mMutex;
-        float mDownloadProgress = -1.0f;
+        float mDownloadProgress = 0.0f;
 
         ServerInfos mServers;
         ServerInfo *mServerInfo;
diff --git a/src/gui/updaterwindow.cpp b/src/gui/updaterwindow.cpp
index bd98637b..852d3ade 100644
--- a/src/gui/updaterwindow.cpp
+++ b/src/gui/updaterwindow.cpp
@@ -46,8 +46,8 @@
 #include <iostream>
 #include <fstream>
 
-const std::string xmlUpdateFile = "resources.xml";
-const std::string txtUpdateFile = "resources2.txt";
+constexpr char xmlUpdateFile[] = "resources.xml";
+constexpr char txtUpdateFile[] = "resources2.txt";
 
 /**
  * Load the given file into a vector of updateFiles.
@@ -174,9 +174,10 @@ UpdaterWindow::~UpdaterWindow()
     {
         mDownload->cancel();
 
-        delete mDownload;
-        mDownload = nullptr;
+        // Make sure thread is gone before freeing the memory buffer
+        mDownload.reset();
     }
+
     free(mMemoryBuffer);
 }
 
@@ -251,19 +252,8 @@ void UpdaterWindow::loadNews()
         return;
     }
 
-    // Reallocate and include terminating 0 character
-    mMemoryBuffer = (char*)realloc(mMemoryBuffer, mDownloadedBytes + 1);
-    mMemoryBuffer[mDownloadedBytes] = '\0';
-
     mBrowserBox->clearRows();
-
-    // Tokenize and add each line separately
-    char *line = strtok(mMemoryBuffer, "\n");
-    while (line)
-    {
-        mBrowserBox->addRow(line);
-        line = strtok(nullptr, "\n");
-    }
+    mBrowserBox->addRows(std::string_view(mMemoryBuffer, mDownloadedBytes));
 
     // Free the memory buffer now that we don't need it anymore
     free(mMemoryBuffer);
@@ -273,7 +263,7 @@ void UpdaterWindow::loadNews()
 }
 
 int UpdaterWindow::updateProgress(void *ptr, DownloadStatus status,
-                                  size_t dt, size_t dn)
+                                  size_t dltotal, size_t dlnow)
 {
     auto *uw = reinterpret_cast<UpdaterWindow *>(ptr);
 
@@ -287,20 +277,15 @@ int UpdaterWindow::updateProgress(void *ptr, DownloadStatus status,
         uw->mDownloadStatus = UPDATE_ERROR;
     }
 
-    float progress = (float) dn / dt;
-
-    if (progress != progress)
-        progress = 0.0f; // check for NaN
-    if (progress < 0.0f)
-        progress = 0.0f; // no idea how this could ever happen, but why not check for it anyway.
-    if (progress > 1.0f)
-        progress = 1.0f;
+    float progress = 0.0f;
+    if (dltotal > 0)
+        progress = static_cast<float>(dlnow) / dltotal;
 
     uw->setLabel(
             uw->mCurrentFile + " (" + toString((int) (progress * 100)) + "%)");
     uw->setProgress(progress);
 
-    if (Client::getState() != STATE_UPDATE || uw->mDownloadStatus == UPDATE_ERROR)
+    if (Client::getState() != STATE_UPDATE)
     {
         // If the action was canceled return an error code to stop the mThread
         return -1;
@@ -309,15 +294,15 @@ int UpdaterWindow::updateProgress(void *ptr, DownloadStatus status,
     return 0;
 }
 
-size_t UpdaterWindow::memoryWrite(void *ptr, size_t size, size_t nmemb, void *stream)
+size_t UpdaterWindow::memoryWrite(char *ptr, size_t size, size_t nmemb, void *stream)
 {
     auto *uw = reinterpret_cast<UpdaterWindow *>(stream);
-    size_t totalMem = size * nmemb;
+    const size_t totalMem = size * nmemb;
     uw->mMemoryBuffer = (char*) realloc(uw->mMemoryBuffer,
                                         uw->mDownloadedBytes + totalMem);
     if (uw->mMemoryBuffer)
     {
-        memcpy(&(uw->mMemoryBuffer[uw->mDownloadedBytes]), ptr, totalMem);
+        memcpy(uw->mMemoryBuffer + uw->mDownloadedBytes, ptr, totalMem);
         uw->mDownloadedBytes += totalMem;
     }
 
@@ -326,8 +311,9 @@ size_t UpdaterWindow::memoryWrite(void *ptr, size_t size, size_t nmemb, void *st
 
 void UpdaterWindow::download()
 {
-    mDownload = new Net::Download(this, mUpdateHost + "/" + mCurrentFile,
-                                  updateProgress);
+    mDownload = std::make_unique<Net::Download>(this,
+                                                mUpdateHost + "/" + mCurrentFile,
+                                                &UpdaterWindow::updateProgress);
 
     if (mStoreInMemory)
     {
@@ -335,15 +321,11 @@ void UpdaterWindow::download()
     }
     else
     {
+        std::optional<unsigned long> adler32;
         if (mDownloadStatus == UPDATE_RESOURCES)
-        {
-            mDownload->setFile(mUpdatesDir + "/" + mCurrentFile,
-                               mCurrentChecksum);
-        }
-        else
-        {
-            mDownload->setFile(mUpdatesDir + "/" + mCurrentFile);
-        }
+            adler32 = mCurrentChecksum;
+
+        mDownload->setFile(mUpdatesDir + "/" + mCurrentFile, adler32);
     }
 
     if (mDownloadStatus != UPDATE_RESOURCES)
@@ -365,8 +347,8 @@ void UpdaterWindow::loadUpdates()
         if (mUpdateFiles.empty())
         {
             logger->log("Warning this server does not have a"
-                        " %s file falling back to %s", xmlUpdateFile.c_str(),
-                        txtUpdateFile.c_str());
+                        " %s file falling back to %s", xmlUpdateFile,
+                        txtUpdateFile);
             mUpdateFiles = loadTxtFile(mUpdatesDir + "/" + txtUpdateFile);
         }
     }
@@ -398,20 +380,19 @@ void UpdaterWindow::logic()
 
     switch (mDownloadStatus)
     {
-        case UPDATE_ERROR:
-            // TODO: Only send complete sentences to gettext
-            mBrowserBox->addRow(std::string());
-            mBrowserBox->addRow(_("##1  The update process is incomplete."));
-            // TRANSLATORS: Continues "you try again later.".
-            mBrowserBox->addRow(_("##1  It is strongly recommended that"));
-            // TRANSLATORS: Begins "It is strongly recommended that".
-            mBrowserBox->addRow(_("##1  you try again later."));
-
-            mBrowserBox->addRow(mDownload->getError());
+        case UPDATE_ERROR: {
+            std::string error = "##1";
+            error += mDownload->getError();
+            error += "\n\n";
+            error += _("The update process is incomplete. "
+                       "It is strongly recommended that you try again later.");
+            mBrowserBox->addRows(error);
+
             mScrollArea->setVerticalScrollAmount(
                     mScrollArea->getVerticalMaxScroll());
             mDownloadStatus = UPDATE_COMPLETE;
             break;
+        }
         case UPDATE_NEWS:
             if (mDownloadComplete)
             {
diff --git a/src/gui/updaterwindow.h b/src/gui/updaterwindow.h
index 65ee1d88..5c749e18 100644
--- a/src/gui/updaterwindow.h
+++ b/src/gui/updaterwindow.h
@@ -113,12 +113,12 @@ private:
      * A download callback for progress updates.
      */
     static int updateProgress(void *ptr, DownloadStatus status,
-                              size_t dt, size_t dn);
+                              size_t dltotal, size_t dlnow);
 
     /**
      * A libcurl callback for writing to memory.
      */
-    static size_t memoryWrite(void *ptr, size_t size, size_t nmemb,
+    static size_t memoryWrite(char *ptr, size_t size, size_t nmemb,
                               void *stream);
 
     enum UpdateDownloadStatus
@@ -165,13 +165,13 @@ private:
     bool mUserCancel = false;
 
     /** Byte count currently downloaded in mMemoryBuffer. */
-    int mDownloadedBytes = 0;
+    size_t mDownloadedBytes = 0;
 
     /** Buffer for files downloaded to memory. */
     char *mMemoryBuffer = nullptr;
 
     /** Download handle. */
-    Net::Download *mDownload = nullptr;
+    std::unique_ptr<Net::Download> mDownload;
 
     /** List of files to download. */
     std::vector<UpdateFile> mUpdateFiles;