libcamera: signal: Disconnect signal automatically on slot deletion

When a signal is connected to a member function slot, the slot is not
disconnected when the slot object is deleted. This can lead to calling a
member function of a deleted object if the signal isn't disconnected
manually by the slot object's destructor.

Make signal handling easier by implementing a base Object class that
tracks all connected signals and disconnects from them automatically
when the object is deleted, using template specialization resolution in
the Signal class.

As inheriting from the Object class may to a too harsh requirement for
Signal usage in applications, keep the existing behaviour working if the
slot doesn't inherit from the Object class. We may reconsider this later
and require all slot objects to inherit from the Object class.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

8 files changed, 209 insertions, 35 deletions

diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
index b78fb3a0..3e2b7fd9 100644
--- a/Documentation/Doxyfile.in
+++ b/Documentation/Doxyfile.in
@@ -860,7 +860,9 @@ EXCLUDE_PATTERNS       =
 # Note that the wildcards are matched against the file with absolute path, so to
 # exclude all test directories use the pattern */test/*
 
-EXCLUDE_SYMBOLS        = libcamera::SlotBase \
+EXCLUDE_SYMBOLS        = libcamera::SignalBase \
+                         libcamera::SlotArgs \
+                         libcamera::SlotBase \
                          libcamera::SlotMember \
                          libcamera::SlotStatic
 
diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
index 5788e9bb..3f4d1e28 100644
--- a/include/libcamera/meson.build
+++ b/include/libcamera/meson.build
@@ -5,6 +5,7 @@ libcamera_api = files([
     'event_dispatcher.h',
     'event_notifier.h',
     'libcamera.h',
+    'object.h',
     'request.h',
     'signal.h',
     'stream.h',
diff --git a/include/libcamera/object.h b/include/libcamera/object.h
new file mode 100644
index 00000000..eadd41f9
--- /dev/null
+++ b/include/libcamera/object.h
@@ -0,0 +1,35 @@
+/* SPDX-License-Identifier: LGPL-2.1-or-later */
+/*
+ * Copyright (C) 2019, Google Inc.
+ *
+ * object.h - Base object
+ */
+#ifndef __LIBCAMERA_OBJECT_H__
+#define __LIBCAMERA_OBJECT_H__
+
+#include <list>
+
+namespace libcamera {
+
+class SignalBase;
+template<typename... Args>
+class Signal;
+
+class Object
+{
+public:
+	virtual ~Object();
+
+private:
+	template<typename... Args>
+	friend class Signal;
+
+	void connect(SignalBase *signal);
+	void disconnect(SignalBase *signal);
+
+	std::list<SignalBase *> signals_;
+};
+
+}; /* namespace libcamera */
+
+#endif /* __LIBCAMERA_OBJECT_H__ */
diff --git a/include/libcamera/signal.h b/include/libcamera/signal.h
index 6e8b876e..c8f3243e 100644
--- a/include/libcamera/signal.h
+++ b/include/libcamera/signal.h
@@ -10,32 +10,47 @@
 #include <list>
 #include <vector>
 
+#include <libcamera/object.h>
+
 namespace libcamera {
 
 template<typename... Args>
 class Signal;
 
-template<typename... Args>
 class SlotBase
 {
 public:
-	SlotBase(void *obj)
-		: obj_(obj) {}
+	SlotBase(void *obj, bool isObject)
+		: obj_(obj), isObject_(isObject) {}
 	virtual ~SlotBase() {}
 
+	void *obj() { return obj_; }
+	bool isObject() const { return isObject_; }
+
+protected:
+	void *obj_;
+	bool isObject_;
+};
+
+template<typename... Args>
+class SlotArgs : public SlotBase
+{
+public:
+	SlotArgs(void *obj, bool isObject)
+		: SlotBase(obj, isObject) {}
+
 	virtual void invoke(Args... args) = 0;
 
 protected:
 	friend class Signal<Args...>;
-	void *obj_;
 };
 
 template<typename T, typename... Args>
-class SlotMember : public SlotBase<Args...>
+class SlotMember : public SlotArgs<Args...>
 {
 public:
-	SlotMember(T *obj, void (T::*func)(Args...))
-		: SlotBase<Args...>(obj), func_(func) {}
+	SlotMember(T *obj, bool isObject, void (T::*func)(Args...))
+		: SlotArgs<Args...>(obj, isObject), func_(func) {}
 
 	void invoke(Args... args) { (static_cast<T *>(this->obj_)->*func_)(args...); }
 
@@ -45,11 +60,11 @@ private:
 };
 
 template<typename... Args>
-class SlotStatic : public SlotBase<Args...>
+class SlotStatic : public SlotArgs<Args...>
 {
 public:
 	SlotStatic(void (*func)(Args...))
-		: SlotBase<Args...>(nullptr), func_(func) {}
+		: SlotArgs<Args...>(nullptr, false), func_(func) {}
 
 	void invoke(Args... args) { (*func_)(args...); }
 
@@ -58,21 +73,57 @@ private:
 	void (*func_)(Args...);
 };
 
+class SignalBase
+{
+public:
+	template<typename T>
+	void disconnect(T *object)
+	{
+		for (auto iter = slots_.begin(); iter != slots_.end(); ) {
+			SlotBase *slot = *iter;
+			if (slot->obj() == object) {
+				iter = slots_.erase(iter);
+				delete slot;
+			} else {
+				++iter;
+			}
+		}
+	}
+
+protected:
+	friend class Object;
+	std::list<SlotBase *> slots_;
+};
+
 template<typename... Args>
-class Signal
+class Signal : public SignalBase
 {
 public:
 	Signal() {}
 	~Signal()
 	{
-		for (SlotBase<Args...> *slot : slots_)
+		for (SlotBase *slot : slots_) {
+			if (slot->isObject())
+				static_cast<Object *>(slot->obj())->disconnect(this);
 			delete slot;
+		}
+	}
+
+#ifndef __DOXYGEN__
+	template<typename T, typename std::enable_if<std::is_base_of<Object, T>::value>::type * = nullptr>
+	void connect(T *object, void (T::*func)(Args...))
+	{
+		object->connect(this);
+		slots_.push_back(new SlotMember<T, Args...>(object, true, func));
 	}
 
+	template<typename T, typename std::enable_if<!std::is_base_of<Object, T>::value>::type * = nullptr>
+#else
 	template<typename T>
+#endif
 	void connect(T *object, void (T::*func)(Args...))
 	{
-		slots_.push_back(new SlotMember<T, Args...>(object, func));
+		slots_.push_back(new SlotMember<T, Args...>(object, false, func));
 	}
 
 	void connect(void (*func)(Args...))
@@ -82,7 +133,7 @@ public:
 
 	void disconnect()
 	{
-		for (SlotBase<Args...> *slot : slots_)
+		for (SlotBase *slot : slots_)
 			delete slot;
 		slots_.clear();
 	}
@@ -90,27 +141,21 @@ public:
 	template<typename T>
 	void disconnect(T *object)
 	{
-		for (auto iter = slots_.begin(); iter != slots_.end(); ) {
-			SlotBase<Args...> *slot = *iter;
-			if (slot->obj_ == object) {
-				iter = slots_.erase(iter);
-				delete slot;
-			} else {
-				++iter;
-			}
-		}
+		SignalBase::disconnect(object);
 	}
 
 	template<typename T>
 	void disconnect(T *object, void (T::*func)(Args...))
 	{
 		for (auto iter = slots_.begin(); iter != slots_.end(); ) {
-			SlotBase<Args...> *slot = *iter;
+			SlotArgs<Args...> *slot = static_cast<SlotArgs<Args...> *>(*iter);
 			/*
-			 * If the obj_ pointer matches the object types must
-			 * match, so we can safely cast to SlotMember<T, Args>.
+			 * If the obj() pointer matches the object, the slot is
+			 * guaranteed to be a member slot, so we can safely
+			 * cast it to SlotMember<T, Args...> and access its
+			 * func_ member.
 			 */
-			if (slot->obj_ == object &&
+			if (slot->obj() == object &&
 			    static_cast<SlotMember<T, Args...> *>(slot)->func_ == func) {
 				iter = slots_.erase(iter);
 				delete slot;
@@ -123,8 +168,8 @@ public:
 	void disconnect(void (*func)(Args...))
 	{
 		for (auto iter = slots_.begin(); iter != slots_.end(); ) {
-			SlotBase<Args...> *slot = *iter;
-			if (slot->obj_ == nullptr &&
+			SlotArgs<Args...> *slot = *iter;
+			if (slot->obj() == nullptr &&
 			    static_cast<SlotStatic<Args...> *>(slot)->func_ == func) {
 				iter = slots_.erase(iter);
 				delete slot;
@@ -140,13 +185,11 @@ public:
 		 * Make a copy of the slots list as the slot could call the
 		 * disconnect operation, invalidating the iterator.
 		 */
-		std::vector<SlotBase<Args...> *> slots{ slots_.begin(), slots_.end() };
-		for (SlotBase<Args...> *slot : slots)
-			slot->invoke(args...);
+		std::vector<SlotBase *> slots{ slots_.begin(), slots_.end() };
+		for (SlotBase *slot : slots) {
+			static_cast<SlotArgs<Args...> *>(slot)->invoke(args...);
+		}
 	}
-
-private:
-	std::list<SlotBase<Args...> *> slots_;
 };
 
 } /* namespace libcamera */
diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
index c5354c13..8384cd0a 100644
--- a/src/libcamera/meson.build
+++ b/src/libcamera/meson.build
@@ -10,6 +10,7 @@ libcamera_sources = files([
     'log.cpp',
     'media_device.cpp',
     'media_object.cpp',
+    'object.cpp',
     'pipeline_handler.cpp',
     'request.cpp',
     'signal.cpp',
diff --git a/src/libcamera/object.cpp b/src/libcamera/object.cpp
new file mode 100644
index 00000000..826eed6f
--- /dev/null
+++ b/src/libcamera/object.cpp
@@ -0,0 +1,50 @@
+/* SPDX-License-Identifier: LGPL-2.1-or-later */
+/*
+ * Copyright (C) 2019, Google Inc.
+ *
+ * object.cpp - Base object
+ */
+
+#include <libcamera/object.h>
+#include <libcamera/signal.h>
+
+/**
+ * \file object.h
+ * \brief Base object to support automatic signal disconnection
+ */
+
+namespace libcamera {
+
+/**
+ * \class Object
+ * \brief Base object to support automatic signal disconnection
+ *
+ * The Object class simplifies signal/slot handling for classes implementing
+ * slots. By inheriting from Object, an object is automatically disconnected
+ * from all connected signals when it gets destroyed.
+ *
+ * \sa Signal
+ */
+
+Object::~Object()
+{
+	for (SignalBase *signal : signals_)
+		signal->disconnect(this);
+}
+
+void Object::connect(SignalBase *signal)
+{
+	signals_.push_back(signal);
+}
+
+void Object::disconnect(SignalBase *signal)
+{
+	for (auto iter = signals_.begin(); iter != signals_.end(); ) {
+		if (*iter == signal)
+			iter = signals_.erase(iter);
+		else
+			iter++;
+	}
+}
+
+}; /* namespace libcamera */
diff --git a/src/libcamera/signal.cpp b/src/libcamera/signal.cpp
index 8d62b5be..cb7daa11 100644
--- a/src/libcamera/signal.cpp
+++ b/src/libcamera/signal.cpp
@@ -47,6 +47,11 @@ namespace libcamera {
  * \brief Connect the signal to a member function slot
  * \param object The slot object pointer
  * \param func The slot member function
+ *
+ * If the typename T inherits from Object, the signal will be automatically
+ * disconnected from the \a func slot of \a object when \a object is destroyed.
+ * Otherwise the caller shall disconnect signals manually before destroying \a
+ * object.
  */
 
 /**
diff --git a/test/signal.cpp b/test/signal.cpp
index ab69ca1b..19a52c60 100644
--- a/test/signal.cpp
+++ b/test/signal.cpp
@@ -8,6 +8,7 @@
 #include <iostream>
 #include <string.h>
 
+#include <libcamera/object.h>
 #include <libcamera/signal.h>
 
 #include "test.h"
@@ -22,6 +23,15 @@ static void slotStatic(int value)
 	valueStatic_ = value;
 }
 
+class SlotObject : public Object
+{
+public:
+	void slot()
+	{
+		valueStatic_ = 1;
+	}
+};
+
 class SignalTest : public Test
 {
 protected:
@@ -139,6 +149,33 @@ protected:
 			return TestFail;
 		}
 
+		/*
+		 * Test automatic disconnection on object deletion. Connect the
+		 * slot twice to ensure all instances are disconnected.
+		 */
+		signalVoid_.disconnect();
+
+		SlotObject *slotObject = new SlotObject();
+		signalVoid_.connect(slotObject, &SlotObject::slot);
+		signalVoid_.connect(slotObject, &SlotObject::slot);
+		delete slotObject;
+		valueStatic_ = 0;
+		signalVoid_.emit();
+		if (valueStatic_ != 0) {
+			cout << "Signal disconnection on object deletion test failed" << endl;
+			return TestFail;
+		}
+
+		/*
+		 * Test that signal deletion disconnects objects. This shall
+		 * not generate any valgrind warning.
+		 */
+		Signal<> *signal = new Signal<>();
+		slotObject = new SlotObject();
+		signal->connect(slotObject, &SlotObject::slot);
+		delete signal;
+		delete slotObject;
+
 		return TestPass;
 	}