Fix implicit move that could lead to UB

In some scenarios we could end up with UB. I added simple example in test case where we add class
instance to vector and as we know it may move its elements when resizing.

Maybe we could allow to move and just unlisten previous listener but this would be very annoying as
user needs to remember what would happen in every case. It is better to let user decide and force
him to think about it.

lib/src/dexode/eventbus/Listener.hpp

@@ -29,12 +29,9 @@ public:
 	}
 
 	Listener(const Listener& other) = delete;
-
+	// To see why move is disabled search for tag: FORBID_MOVE_LISTENER in tests
-	Listener(Listener&& other) noexcept
+	// Long story short, what if we capture 'this' in lambda during registering listener in ctor
-		: _id(other._id) // we don't have to reset listener ID as _bus is moved and we won't call
+	Listener(Listener&& other) = delete;
-						 // unlistenAll
-		, _bus(std::move(other._bus))
-	{}
 
 	~Listener()
 	{
@@ -45,24 +42,8 @@ public:
 	}
 
 	Listener& operator=(const Listener& other) = delete;
-
+	// To see why move is disabled search for tag: FORBID_MOVE_LISTENER in tests
-	Listener& operator=(Listener&& other) noexcept
+	Listener& operator=(Listener&& other) = delete;
-	{
-		if(this == &other)
-		{
-			return *this;
-		}
-
-		if(_bus != nullptr)
-		{
-			unlistenAll(); // remove previous
-		}
-		// we don't have reset listener ID as bus is moved and we won't call unlistenAll
-		_id = other._id;
-		_bus = std::move(other._bus);
-
-		return *this;
-	}
 
 	template <class Event, typename _ = void>
 	constexpr void listen(std::function<void(const Event&)>&& callback)
@@ -127,6 +108,28 @@ public:
 		internal::ListenerAttorney<Bus>::unlisten(*_bus, _id, internal::event_id<Event>());
 	}
 
+	// We want more explicit move so user knows what is going on
+	void transfer(Listener&& from)
+	{
+		if(this == &from)
+		{
+			throw std::runtime_error("Self transfer not allowed");
+		}
+
+		if(_bus != nullptr)
+		{
+			unlistenAll(); // remove previous
+		}
+		// we don't have to reset listener ID as bus is moved and we won't call unlistenAll
+		_id = from._id;
+		_bus = std::move(from._bus);
+	}
+
+	const std::shared_ptr<Bus>& getBus() const
+	{
+		return _bus;
+	}
+
 private:
 	std::uint32_t _id = 0;
 	std::shared_ptr<Bus> _bus = nullptr;

test/unit/src/dexode/eventbus/test/SuiteListener.cpp

@@ -1,3 +1,5 @@
+#include <vector>
+
 #include <catch2/catch.hpp>
 
 #include "dexode/EventBus.hpp"
@@ -46,7 +48,7 @@ TEST_CASE("Should unlisten all events When listener instance is overriden", "[Ev
 	REQUIRE(bus.process() == 1);
 	REQUIRE(callCount == 1);
 
-	listener = Listener{};
+	listener.transfer(Listener{});
 
 	bus.postpone(event::Value{2});
 	REQUIRE(bus.process() == 1);
@@ -90,7 +92,7 @@ TEST_CASE("Should keep listeners When listener is moved", "[EventBus][Listener]"
 		REQUIRE(bus->process() == 1);
 		REQUIRE(callCount == 1);
 
-		transferOne = std::move(listener);
+		transferOne.transfer(std::move(listener));
 	}
 
 	bus->postpone(event::Value{3});
@@ -231,4 +233,68 @@ TEST_CASE("Should compile", "[EventBus][Listener]")
 	}
 }
 
+class TestClazz
+{
+public:
+	static int counter;
+
+	TestClazz(int id, const std::shared_ptr<EventBus>& bus)
+		: _id{id}
+		, _listener{bus}
+	{
+		registerListener();
+	}
+
+	TestClazz(TestClazz&& other)
+		: _id{other._id}
+		, _listener{other._listener.getBus()}
+	{
+		// We need to register again
+		registerListener();
+	}
+
+	~TestClazz()
+	{
+		_id = 0;
+	}
+
+private:
+	int _id = 0;
+	EventBus::Listener _listener;
+
+	void registerListener()
+	{
+		_listener.listen([this](const event::Value& event) {
+			if(_id == 1)
+			{
+				++counter;
+			}
+		});
+	}
+};
+
+int TestClazz::counter = 0;
+
+TEST_CASE("Should not allow for mistake with move ctor", "[EventBus][Listener]")
+{
+	/**
+	 * Test case TAG: FORBID_MOVE_LISTENER
+	 *
+	 * This case is little bit complicated.
+	 * We can't move EventBus::Listener as it capture 'this' in ctor so whenever we would use it it
+	 * would lead to UB.
+	 */
+	std::shared_ptr<EventBus> bus = std::make_shared<EventBus>();
+
+	std::vector<TestClazz> vector;
+	vector.emplace_back(1, bus);
+	vector.emplace_back(2, bus);
+	vector.emplace_back(3, bus);
+
+	bus->postpone(event::Value{100});
+	bus->process();
+
+	REQUIRE(TestClazz::counter == 1);
+}
+
 } // namespace dexode::eventbus::test