Issue 4613 - fix leak of Thread

TimeoutThread was not deleted, the change fixes it. It also contains refactoring which should help to delete Thread when known exception is thrown inside Thread::Run, which is going to be a part of #3594.

[Oleksandr, 2016-11-07 10:31:32]

Just two comments. Also, I think the refactoring deserves an issue.

https://codereview.adblockplus.org/29361582/diff/29361583/src/Thread.cpp
File src/Thread.cpp (right):

https://codereview.adblockplus.org/29361582/diff/29361583/src/Thread.cpp#newc...
src/Thread.cpp:82: : m_deleteSelfOnFinish(deleteSelfOnFinish)
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)?

https://codereview.adblockplus.org/29361582/diff/29361583/src/Thread.cpp#newc...
src/Thread.cpp:90: void Thread::Start()
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:13]

On 2016/11/07 10:31:32, Oleksandr wrote:
> Just two comments. Also, I think the refactoring deserves an issue.
Done.

https://codereview.adblockplus.org/29361582/diff/29361583/src/Thread.cpp
File src/Thread.cpp (right):

https://codereview.adblockplus.org/29361582/diff/29361583/src/Thread.cpp#newc...
src/Thread.cpp:82: : m_deleteSelfOnFinish(deleteSelfOnFinish)
On 2016/11/07 10:31:31, Oleksandr wrote:
> 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)?

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.

https://codereview.adblockplus.org/29361582/diff/29361583/src/Thread.cpp#newc...
src/Thread.cpp:90: void Thread::Start()
On 2016/11/07 10:31:31, Oleksandr wrote:
> How about renaming this to something like 'LaunchAndSelfDestroy'? It is
> important for callers to be very aware that an object can be dismissed.

No, because it won't be always destroyed.

https://codereview.adblockplus.org/29361582/diff/29361583/src/Thread.cpp#newc...
src/Thread.cpp:111: if (thread->m_deleteSelfOnFinish)
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.

[Oleksandr, 2016-11-20 23:52:02]

ok, LGTM

[sergei, 2016-11-21 10:03:59]

https://codereview.adblockplus.org/29361582/diff/29361583/src/FileSystemJsObj...
File src/FileSystemJsObject.cpp (right):

https://codereview.adblockplus.org/29361582/diff/29361583/src/FileSystemJsObj...
src/FileSystemJsObject.cpp:22: #include <thread>
Sorry, overlooked, it's not used, so I won't include it into commit.