From 36179a7197f24d684b263c802a1459fef6c9fb16 Mon Sep 17 00:00:00 2001
From: Jim Broadus <jbroadus@gmail.com>
Date: Sat, 28 Dec 2019 16:22:49 -0800
Subject: [PATCH] Fix gpodder sync memory leaks in success cases.

A closure created by NewClosure that handles Qt signals is destroyed if the
signal object is destroyed, the slot object is destroyed, or the signal is
invoked. In the case where the sender is passed as a shared pointer, the
reference prevents the sender from being destroyed before the closure.

So for closures built to handle responses returned from ApiRequest in
GPodderSync, the closure object and the response object will only be destroyed
after the signal is invoked. In some cases, separate closures are built for
error signals as well. For these, only one closure will be destroyed. The other
closures and the response object will be leaked.

A simple fix for the success cases is to remove the unnecessary error case
closures and directly connect the signals to slots. This is low hanging fruit
and still leaves leaks in the error cases. Those cases will require a more
complete solution to properly manage the life cycle of the response object.
---
 src/internet/podcasts/gpoddersync.cpp | 31 ++++++++++++++++-----------
 src/internet/podcasts/gpoddersync.h   |  6 ++++--
 2 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/src/internet/podcasts/gpoddersync.cpp b/src/internet/podcasts/gpoddersync.cpp
index 81aac6031..46aeff1b5 100644
--- a/src/internet/podcasts/gpoddersync.cpp
+++ b/src/internet/podcasts/gpoddersync.cpp
@@ -157,14 +157,17 @@ void GPodderSync::GetUpdatesNow() {
       api_->deviceUpdates(username_, DeviceId(), timestamp));
   NewClosure(reply, SIGNAL(finished()), this,
              SLOT(DeviceUpdatesFinished(mygpo::DeviceUpdatesPtr)), reply);
-  NewClosure(reply, SIGNAL(parseError()), this,
-             SLOT(DeviceUpdatesFailed(mygpo::DeviceUpdatesPtr)), reply);
-  NewClosure(reply, SIGNAL(requestError(QNetworkReply::NetworkError)), this,
-             SLOT(DeviceUpdatesFailed(mygpo::DeviceUpdatesPtr)), reply);
+  connect(reply.data(), SIGNAL(parseError()), SLOT(DeviceUpdatesParseError()));
+  connect(reply.data(), SIGNAL(requestError(QNetworkReply::NetworkError)),
+          SLOT(DeviceUpdatesRequestError(QNetworkReply::NetworkError)));
 }
 
-void GPodderSync::DeviceUpdatesFailed(mygpo::DeviceUpdatesPtr reply) {
-  qLog(Warning) << "Failed to get gpodder.net device updates";
+void GPodderSync::DeviceUpdatesParseError() {
+  qLog(Warning) << "Failed to get gpodder device updates: parse error";
+}
+
+void GPodderSync::DeviceUpdatesRequestError(QNetworkReply::NetworkError error) {
+  qLog(Warning) << "Failed to get gpodder device updates:" << error;
 }
 
 void GPodderSync::DeviceUpdatesFinished(mygpo::DeviceUpdatesPtr reply) {
@@ -344,15 +347,19 @@ void GPodderSync::FlushUpdateQueue() {
   NewClosure(reply, SIGNAL(finished()), this,
              SLOT(AddRemoveFinished(mygpo::AddRemoveResultPtr, QList<QUrl>)),
              reply, all_urls.toList());
-  NewClosure(reply, SIGNAL(parseError()), this,
-             SLOT(AddRemoveFailed(mygpo::AddRemoveResultPtr)), reply);
-  NewClosure(reply, SIGNAL(requestError(QNetworkReply::NetworkError)), this,
-             SLOT(AddRemoveFailed(mygpo::AddRemoveResultPtr)), reply);
+  connect(reply.data(), SIGNAL(parseError()), SLOT(AddRemoveParseError()));
+  connect(reply.data(), SIGNAL(requestError(QNetworkReply::NetworkError)),
+          SLOT(AddRemoveRequestError(QNetworkReply::NetworkError)));
 }
 
-void GPodderSync::AddRemoveFailed(mygpo::AddRemoveResultPtr reply) {
+void GPodderSync::AddRemoveParseError() {
   flushing_queue_ = false;
-  qLog(Warning) << "Failed to update gpodder.net subscriptions";
+  qLog(Warning) << "Failed to update gpodder subscriptions: parse error";
+}
+
+void GPodderSync::AddRemoveRequestError(QNetworkReply::NetworkError err) {
+  flushing_queue_ = false;
+  qLog(Warning) << "Failed to update gpodder subscriptions:" << err;
 }
 
 void GPodderSync::AddRemoveFinished(mygpo::AddRemoveResultPtr reply,
diff --git a/src/internet/podcasts/gpoddersync.h b/src/internet/podcasts/gpoddersync.h
index 49b1b9cbd..844bee166 100644
--- a/src/internet/podcasts/gpoddersync.h
+++ b/src/internet/podcasts/gpoddersync.h
@@ -77,7 +77,8 @@ class GPodderSync : public QObject {
                      const QString& password);
 
   void DeviceUpdatesFinished(mygpo::DeviceUpdatesPtr reply);
-  void DeviceUpdatesFailed(mygpo::DeviceUpdatesPtr reply);
+  void DeviceUpdatesParseError();
+  void DeviceUpdatesRequestError(QNetworkReply::NetworkError error);
 
   void NewPodcastLoaded(PodcastUrlLoaderReply* reply, const QUrl& url,
                         const QList<mygpo::EpisodePtr>& actions);
@@ -91,7 +92,8 @@ class GPodderSync : public QObject {
 
   void AddRemoveFinished(mygpo::AddRemoveResultPtr reply,
                          const QList<QUrl>& affected_urls);
-  void AddRemoveFailed(mygpo::AddRemoveResultPtr reply);
+  void AddRemoveParseError();
+  void AddRemoveRequestError(QNetworkReply::NetworkError error);
 
  private:
   void LoadQueue();