From 21755c7f20426689965bf28ef6b534d110f603b6 Mon Sep 17 00:00:00 2001
From: itsmattkc <itsmattkc@gmail.com>
Date: Sun, 16 Feb 2020 13:01:38 +1100
Subject: [PATCH] taskmanager: ensure task is finished before deleting it

Fixes race condition that could result in a segfault if a Task hadn't emitted
Finished() before it was destroyed by TaskManager.
---
 app/task/taskmanager.cpp | 55 +++++++++++++++++++---------------------
 app/task/taskmanager.h   |  4 +--
 2 files changed, 28 insertions(+), 31 deletions(-)

diff --git a/app/task/taskmanager.cpp b/app/task/taskmanager.cpp
index b43ecfbb7..840a3c0b9 100644
--- a/app/task/taskmanager.cpp
+++ b/app/task/taskmanager.cpp
@@ -88,6 +88,7 @@ void TaskManager::AddTask(Task* t)
   // Connect Task's status signal to the Callback
   connect(t, &Task::Succeeeded, this, &TaskManager::TaskSucceeded, Qt::QueuedConnection);
   connect(t, &Task::Failed, this, &TaskManager::TaskFailed, Qt::QueuedConnection);
+  connect(t, &Task::Finished, this, &TaskManager::TaskFinished, Qt::QueuedConnection);
 
   // Add the Task to the queue
   tasks_.append({t, kWaiting});
@@ -170,15 +171,36 @@ void TaskManager::DeleteTask(Task *t)
   }
 }
 
-void TaskManager::TaskFinished(Task* task)
+void TaskManager::TaskFinished()
 {
+  Task* task_sender = static_cast<Task*>(sender());
+
   // Set this thread's active value to false
   for (int i=0;i<threads_.size();i++) {
-    if (threads_.at(i).thread == task->thread()) {
+    if (threads_.at(i).thread == task_sender->thread()) {
       threads_[i].active = false;
     }
   }
 
+  // See if we can delete this task
+  if (GetTaskStatus(task_sender) == kFinished) {
+    DeleteTask(task_sender);
+  } else if (GetTaskStatus(task_sender) == kError) {
+    // If this task has already been deleted, we'll free its memory now
+    bool was_deleted = true;
+
+    for (int i=0;i<tasks_.size();i++) {
+      if (tasks_.at(i).task == task_sender) {
+        was_deleted = false;
+        break;
+      }
+    }
+
+    if (was_deleted) {
+      delete task_sender;
+    }
+  }
+
   // Decrement the active thread count
   active_thread_count_--;
 
@@ -211,35 +233,10 @@ void TaskManager::SetTaskStatus(Task *t, TaskStatus status)
 
 void TaskManager::TaskSucceeded()
 {
-  Task* task_sender = static_cast<Task*>(sender());
-
-  SetTaskStatus(task_sender, kFinished);
-
-  TaskFinished(task_sender);
-
-  // Delete this task
-  DeleteTask(task_sender);
+  SetTaskStatus(static_cast<Task*>(sender()), kFinished);
 }
 
 void TaskManager::TaskFailed()
 {
-  Task* task_sender = static_cast<Task*>(sender());
-
-  SetTaskStatus(task_sender, kError);
-
-  TaskFinished(task_sender);
-
-  // If this task has already been deleted, we'll free its memory now
-  bool was_deleted = true;
-
-  for (int i=0;i<tasks_.size();i++) {
-    if (tasks_.at(i).task == task_sender) {
-      was_deleted = false;
-      break;
-    }
-  }
-
-  if (was_deleted) {
-    delete task_sender;
-  }
+  SetTaskStatus(static_cast<Task*>(sender()), kError);
 }
diff --git a/app/task/taskmanager.h b/app/task/taskmanager.h
index c32372dfe..1a47994db 100644
--- a/app/task/taskmanager.h
+++ b/app/task/taskmanager.h
@@ -142,8 +142,6 @@ private:
    */
   void DeleteTask(Task* t);
 
-  void TaskFinished(Task *task);
-
   TaskStatus GetTaskStatus(Task* t);
 
   void SetTaskStatus(Task* t, TaskStatus status);
@@ -173,6 +171,8 @@ private slots:
 
   void TaskFailed();
 
+  void TaskFinished();
+
 };
 
 #endif // TASKMANAGER_H