Issue 4613 - fix leak of Thread

src/Thread.cpp

Index: src/Thread.cpp
diff --git a/src/Thread.cpp b/src/Thread.cpp
index 277dd386ce87c614fb6b771dff09ada8243b3476..91bfe61fdf727b428f63b9d413bc9bef130a6d39 100644
--- a/src/Thread.cpp
+++ b/src/Thread.cpp
@@ -23,14 +23,6 @@
 
 using namespace AdblockPlus;
 
-namespace
-{
-  void CallRun(Thread* thread)
-  {
-    thread->Run();
-  }
-}
-
 void AdblockPlus::Sleep(const int millis)
 {
 #ifdef WIN32
@@ -86,6 +78,11 @@ Lock::~Lock()
   mutex.Unlock();
 }
 
+Thread::Thread(bool deleteSelfOnFinish)
+  : m_deleteSelfOnFinish(deleteSelfOnFinish)

[Oleksandr, 2016/11/07 10:31:31]

Do we have a case where we would not want to delete self on finish? Can't we
just assume m_deleteSelfOnFinish is always true and rename the Start method (see
below)?

[sergei, 2016/11/07 13:04:12]

Yes, we have it already in tests, in those cases Thread is allocated on stack
and is deleted when the execution goes out of scope.
https://github.com/adblockplus/libadblockplus/blob/master/test/Thread.cpp

I thought about having it as default or "always" behaviour but decided to go
with the current approach to reduce the amount of changes and avoid possibly
complicated changes in the interface and structure. For instance, it affects not
only destructor but also Thread::Join method, if it's always functionality then
we cannot simple use it because the instance of Thread can be already deleted,
to support it we need to modify the structure of Thread. Currently, it's about
as it was before, dangerous only because of developer's mistake.

Actually, I think about getting rid of that class in a future and use
std::thread for instance. This approach looks pretty dangerous.

+{
+}
+
 Thread::~Thread()
 {
 }
@@ -107,3 +104,10 @@ void Thread::Join()
   pthread_join(nativeThread, 0);
 #endif
 }
+
+void Thread::CallRun(Thread* thread)
+{
+  thread->Run();
+  if (thread->m_deleteSelfOnFinish)

[sergei, 2016/11/07 13:04:12]

BTW, this code is pretty dangerous because it's very easy to get a data race.
We should always use join before calling of a destructor of Thread(false)
because otherwise it can happen that Thread is already destroyed but thread
function is trying to read m_deleteSelfOnFinish.

+    delete thread;
+}