Issue 4613 - fix leak of Thread

src/Thread.cpp

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2016 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 #ifndef WIN32
 #include <unistd.h>
 #endif
 
 #include "Thread.h"
 
 using namespace AdblockPlus;
 
-namespace
-{
-  void CallRun(Thread* thread)
-  {
-    thread->Run();
-  }
-}
-
 void AdblockPlus::Sleep(const int millis)
 {
 #ifdef WIN32
   ::Sleep(millis);
 #else
   usleep(millis * 1000);
 #endif
 }
 
 Mutex::Mutex()
@@ skipping 35 matching lines @@
 Lock::Lock(Mutex& mutex) : mutex(mutex)
 {
   mutex.Lock();
 }
 
 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()
 {
 }
 
 void Thread::Start()

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

How about renaming this to something like 'LaunchAndSelfDestroy'? It is
important for callers to be very aware that an object can be dismissed.

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

No, because it won't be always destroyed.

 {
 #ifdef WIN32
   nativeThread = CreateThread(0, 0, (LPTHREAD_START_ROUTINE)&CallRun, this, 0, 0);
 #else
   pthread_create(&nativeThread, 0, (void* (*)(void*)) &CallRun, this);
 #endif
 }
 
 void Thread::Join()
 {
 #ifdef WIN32
   WaitForSingleObject(nativeThread, INFINITE);
 #else
   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;
+}