From dbe67bf32b66f53c52556a10445c54cb9dd5cea3 Mon Sep 17 00:00:00 2001
From: Jim Broadus <jbroadus@gmail.com>
Date: Sat, 11 Jan 2020 00:10:53 -0800
Subject: [PATCH] Fix closure timing hole.

When a closure involves an ObjectHelper, a connection is made from the
receiver's destroyed signal and the helper object's deleteLater slot. Since
the signal between the sender and the helper object isn't disconnected until
either object is actually destroyed, this leaves a hole where the helper
holds a pointer to an invalid receiver object, but is still able to receive
the signal connected to its Invoke slot.

Instead of connecting the destroyed signal to deleteLater, connect it to a new
TearDown slot that immediately disconnects the signal then calls deleteLater.
---
 ext/libclementine-common/core/closure.cpp | 11 +++++++++--
 ext/libclementine-common/core/closure.h   | 11 +++++++++--
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/ext/libclementine-common/core/closure.cpp b/ext/libclementine-common/core/closure.cpp
index ff0ed120f..aa5ea98e0 100644
--- a/ext/libclementine-common/core/closure.cpp
+++ b/ext/libclementine-common/core/closure.cpp
@@ -37,8 +37,15 @@ ObjectHelper* ClosureBase::helper() const { return helper_; }
 ObjectHelper::ObjectHelper(QObject* sender, const char* signal,
                            ClosureBase* closure)
     : closure_(closure) {
-  connect(sender, signal, SLOT(Invoked()));
-  connect(sender, SIGNAL(destroyed()), SLOT(deleteLater()));
+  connection_ = connect(sender, signal, SLOT(Invoked()));
+  connect(sender, SIGNAL(destroyed()), SLOT(TearDown()));
+}
+
+void ObjectHelper::TearDown() {
+  // For the case that the receiver has been destroyed, disconnect the signal
+  // so that Invoke isn't called.
+  disconnect(connection_);
+  deleteLater();
 }
 
 void ObjectHelper::Invoked() {
diff --git a/ext/libclementine-common/core/closure.h b/ext/libclementine-common/core/closure.h
index 1d44cd212..fafc69abc 100644
--- a/ext/libclementine-common/core/closure.h
+++ b/ext/libclementine-common/core/closure.h
@@ -58,10 +58,14 @@ class ObjectHelper : public QObject {
  public:
   ObjectHelper(QObject* parent, const char* signal, ClosureBase* closure);
 
+ public slots:
+  void TearDown();
+
  private slots:
   void Invoked();
 
  private:
+  QMetaObject::Connection connection_;
   std::unique_ptr<ClosureBase> closure_;
   Q_DISABLE_COPY(ObjectHelper)
 };
@@ -97,8 +101,11 @@ class Closure : public ClosureBase {
     const int index = meta_receiver->indexOfSlot(normalised_slot.constData());
     Q_ASSERT(index != -1);
     slot_ = meta_receiver->method(index);
-    QObject::connect(receiver_, SIGNAL(destroyed()), helper_,
-                     SLOT(deleteLater()));
+    // Use a direct connection to insure that this is called immediately when
+    // the sender is destroyed. This will run on the signal thread, so TearDown
+    // must be thread safe.
+    QObject::connect(receiver_, SIGNAL(destroyed()), helper_, SLOT(TearDown()),
+                     Qt::DirectConnection);
   }
 
   virtual void Invoke() { function_(); }