From 53126f685645da7cc018faf32a4f7b741b7e5e97 Mon Sep 17 00:00:00 2001
From: Jyrki Vesterinen <sandgtx@gmail.com>
Date: Fri, 2 Nov 2018 22:37:00 +0200
Subject: [PATCH] Mouse events: don't keep around references to fighting units

Keeping references is bad if a unit dies and the dying unit has a halo
and a long-running listener for the "last breath" event, like in AtS E2
S10. In that case keeping the reference means that the halo will remain
as long as the event runs.

Closes #3673.
---
 src/mouse_events.cpp | 48 +++++++++++++++++++++++++++++++-------------
 src/mouse_events.hpp |  6 ++++++
 2 files changed, 40 insertions(+), 14 deletions(-)

diff --git a/src/mouse_events.cpp b/src/mouse_events.cpp
index c7881fa3dd15..dbea3b65494b 100644
--- a/src/mouse_events.cpp
+++ b/src/mouse_events.cpp
@@ -633,6 +633,18 @@ unit_map::const_iterator mouse_handler::find_unit(const map_location& hex) const
 	return pc_.gamestate().board_.find_visible_unit(hex, viewing_team());
 }
 
+unit* mouse_handler::find_unit_nonowning(const map_location& hex)
+{
+	unit_map::iterator it = pc_.gamestate().board_.find_visible_unit(hex, viewing_team());
+	return it.valid() ? &*it : nullptr;
+}
+
+const unit* mouse_handler::find_unit_nonowning(const map_location& hex) const
+{
+	unit_map::const_iterator it = pc_.gamestate().board_.find_visible_unit(hex, viewing_team());
+	return it.valid() ? &*it : nullptr;
+}
+
 const map_location mouse_handler::hovered_hex() const
 {
 	int x = -1;
@@ -833,7 +845,7 @@ void mouse_handler::move_action(bool browse)
 	//	}
 
 	unit_map::iterator u;
-	unit_map::iterator clicked_u;
+	const unit* clicked_u = nullptr;
 
 	map_location src;
 	pathfind::paths orig_paths;
@@ -849,7 +861,7 @@ void mouse_handler::move_action(bool browse)
 			u->set_goto(map_location());
 		}
 
-		clicked_u = find_unit(hex);
+		clicked_u = find_unit_nonowning(hex);
 
 		src = selected_hex_;
 		orig_paths = current_paths_;
@@ -863,7 +875,7 @@ void mouse_handler::move_action(bool browse)
 			return;
 		}
 
-		if(((u.valid() && u->side() == side_num_) || pc_.get_whiteboard()->is_active()) && clicked_u.valid()) {
+		if(((u.valid() && u->side() == side_num_) || pc_.get_whiteboard()->is_active()) && clicked_u != nullptr) {
 			if(attack_from == selected_hex_) { // no move needed
 				int choice = -1;
 				{
@@ -988,7 +1000,7 @@ void mouse_handler::move_action(bool browse)
 		} else {
 			// Don't move if the unit already has actions
 			// from the whiteboard.
-			if(pc_.get_whiteboard()->unit_has_actions(u ? &*u : &*clicked_u)) {
+			if(pc_.get_whiteboard()->unit_has_actions(u ? &*u : clicked_u)) {
 				return;
 			}
 
@@ -1307,18 +1319,26 @@ void mouse_handler::attack_enemy_(const map_location& att_loc, const map_locatio
 	const map_location attacker_loc = att_loc;
 	const map_location defender_loc = def_loc;
 
-	unit_map::iterator attacker = find_unit(attacker_loc);
-	if(!attacker || attacker->side() != side_num_ || attacker->incapacitated()) {
-		return;
-	}
+	unit* attacker = nullptr;
+	const unit* defender = nullptr;
+	std::vector<battle_context> bc_vector;
 
-	unit_map::iterator defender = find_unit(defender_loc);
-	if(!defender || current_team().is_enemy(defender->side()) == false || defender->incapacitated()) {
-		return;
-	}
+	{
+		unit_map::iterator attacker_it = find_unit(attacker_loc);
+		if(!attacker_it || attacker_it->side() != side_num_ || attacker_it->incapacitated()) {
+			return;
+		}
 
-	std::vector<battle_context> bc_vector;
-	fill_weapon_choices(bc_vector, attacker, defender);
+		unit_map::iterator defender_it = find_unit(defender_loc);
+		if(!defender_it || current_team().is_enemy(defender_it->side()) == false || defender_it->incapacitated()) {
+			return;
+		}
+
+		fill_weapon_choices(bc_vector, attacker_it, defender_it);
+
+		attacker = &*attacker_it;
+		defender = &*defender_it;
+	}
 
 	if(size_t(choice) >= bc_vector.size()) {
 		return;
diff --git a/src/mouse_events.hpp b/src/mouse_events.hpp
index c7e5fe80e2a8..7def91b7c0e3 100644
--- a/src/mouse_events.hpp
+++ b/src/mouse_events.hpp
@@ -148,6 +148,12 @@ class mouse_handler : public mouse_handler_base {
 	void show_attack_options(const unit_map::const_iterator &u);
 	unit_map::const_iterator find_unit(const map_location& hex) const;
 	unit_map::iterator find_unit(const map_location& hex);
+	/*
+	 * These return raw pointers instead of smart pointers.
+	 * Useful if you don't want to increase the unit reference count.
+	 */
+	unit* find_unit_nonowning(const map_location& hex);
+	const unit* find_unit_nonowning(const map_location& hex) const;
 	bool unit_in_cycle(unit_map::const_iterator it);
 private:
 	team& viewing_team();