Memory leak in DownloadJobImpl

[Michi Henning]

Here is a test case that drops a Downloader on the floor without calling finishDownload():

TEST_F(DownloadTest, abandoned)
{
    set_provider(unique_ptr<provider::ProviderBase>(new MockProvider()));

    Item root;
    {
        unique_ptr<ItemJob> j(acc_.get("root_id"));
        QSignalSpy spy(j.get(), &ItemJob::statusChanged);
        spy.wait(SIGNAL_WAIT_TIME);
        root = j->item();
    }

    Item child;
    {
        unique_ptr<ItemJob> j(acc_.get("child_id"));
        QSignalSpy spy(j.get(), &ItemJob::statusChanged);
        spy.wait(SIGNAL_WAIT_TIME);
        child = j->item();
    }

    unique_ptr<Downloader> downloader(child.createDownloader());
    EXPECT_TRUE(downloader->isValid());

    {
        QSignalSpy spy(downloader.get(), &Downloader::statusChanged);
        spy.wait(SIGNAL_WAIT_TIME);
        auto arg = spy.takeFirst();
        EXPECT_EQ(Downloader::Status::Ready, qvariant_cast<Downloader::Status>(arg.at(0)));
    }
}

Valgrind complains about this:

==43641== 1,385 (272 direct, 1,113 indirect) bytes in 1 blocks are definitely lost in loss record 216 of 227
==43641== at 0x4C2D1AF: operator new(unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==43641== by 0x4FDE17C: boost::promise<void>::promise() (future.hpp:2570)
==43641== by 0x4FDEB64: boost::make_ready_future() (future.hpp:4175)
==43641== by 0x4FDDC04: unity::storage::provider::internal::DownloadJobImpl::cancel(unity::storage::provider::DownloadJob&) (DownloadJobImpl.cpp:161)
==43641== by 0x4FF47E4: void unity::storage::provider::internal::PendingJobs::cancel_job<unity::storage::provider::DownloadJob>(std::shared_ptr<unity::storage::provider::DownloadJob> const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (PendingJobs.cpp:181)
==43641== by 0x4FF0B25: unity::storage::provider::internal::PendingJobs::~PendingJobs() (PendingJobs.cpp:55)
==43641== by 0x4FF0DFB: unity::storage::provider::internal::PendingJobs::~PendingJobs() (PendingJobs.cpp:61)
==43641== by 0x4F95CB3: std::default_delete<unity::storage::provider::internal::PendingJobs>::operator()(unity::storage::provider::internal::PendingJobs*) const (unique_ptr.h:76)
==43641== by 0x4F94BE7: std::unique_ptr<unity::storage::provider::internal::PendingJobs, std::default_delete<unity::storage::provider::internal::PendingJobs> >::~unique_ptr() (unique_ptr.h:236)
==43641== by 0x4F908EF: unity::storage::provider::internal::AccountData::~AccountData() (AccountData.h:51)
==43641== by 0x505BACA: void __gnu_cxx::new_allocator<unity::storage::provider::internal::AccountData>::destroy<unity::storage::provider::internal::AccountData>(unity::storage::provider::internal::AccountData*) (new_allocator.h:124)
==43641== by 0x505B9CE: void std::allocator_traits<std::allocator<unity::storage::provider::internal::AccountData> >::destroy<unity::storage::provider::internal::AccountData>(std::allocator<unity::storage::provider::internal::AccountData>&, unity::storage::provider::internal::AccountData*) (alloc_traits.h:467)

[James Henstridge (jamesh)]

Before classifying this as a definite leak, it'd be good to work out some way to return control to the event loop for some period after this clean-up.

Cancelling a job is potentially an asynchronous operation, so the future returned by cancel() needs to be kept alive until the future is ready. We wait for this to happen by attaching a continuation to the future that will be called later in the event loop.

[Michi Henning (michihenning)]

This is in part related to some of the leaks:

https://svn.boost.org/trac/boost/ticket/12220

We are still compiling with BOOST_THREAD_VERSION=4, which might prevent the boost fix from being effective. (Need to check both xenial and zesty.)