Merge lp:~widelands-dev/widelands/bug-1829623-assert_is_present into lp:widelands

[Benedikt Straub]

Do not send soldiers to attack if they are not stationed in a militarysite anymore

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 5036. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/536862919.
Appveyor build 4816. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1829623_assert_is_present-4816.

[GunChleoc (gunchleoc)]

Code LTGM.

I think we should replace "deserted" with "has left the building", because he has not fled the tribe.

review: Approve

[GunChleoc (gunchleoc)]

I retested with the savegame attached to the bug and this does not fix the problem

review: Needs Fixing

[lp:~widelands-dev/widelands/bug-1829623-assert_is_present]

9118. By Nordfriese on 2019-05-25

Do not send soldiers that no longer exist. Took care of another cornercase.

[Benedikt Straub (nordfriese)]

Found and fixed another potential source of asserts and/or segfaults. And it appears the the assert cannot be guaranteed in certain cornercases.

review: Needs Resubmitting

[GunChleoc (gunchleoc)]

This fixed it, thanks!

@bunnybot merge

review: Approve

=== modified file 'src/logic/map_objects/tribes/militarysite.cc'
--- src/logic/map_objects/tribes/militarysite.cc	2019-05-18 20:43:25 +0000
+++ src/logic/map_objects/tribes/militarysite.cc	2019-05-25 10:54:54 +0000
@@ -885,10 +885,18 @@
 }
 
 void MilitarySite::send_attacker(Soldier& soldier, Building& target) {
-	assert(is_present(soldier));
+	if (!is_present(soldier)) {
+		// The soldier may not be present anymore due to having been kicked out. Most of the time
+		// the function calling us will notice this, but there are cornercase where it might not,
+		// e.g. when a soldier was ordered to leave but did not physically quit the building yet.
+		log("MilitarySite(%3dx%3d)::send_attacker: Not sending soldier %u because he left the building\n",
+				get_position().x, get_position().y, soldier.serial());
+		return;
+	}
 
-	if (has_soldier_job(soldier))
+	if (has_soldier_job(soldier)) {
 		return;
+	}
 
 	SoldierJob sj;
 	sj.soldier = &soldier;

=== modified file 'src/logic/player.cc'
--- src/logic/player.cc	2019-05-25 10:47:18 +0000
+++ src/logic/player.cc	2019-05-25 10:54:54 +0000
@@ -962,8 +962,17 @@
 			if (const AttackTarget* attack_target = building->attack_target()) {
 				if (attack_target->can_be_attacked()) {
 					for (Soldier* temp_attacker : soldiers) {
-						upcast(MilitarySite, ms, temp_attacker->get_location(egbase()));
-						ms->send_attacker(*temp_attacker, *building);
+						assert(temp_attacker);
+						assert(temp_attacker->get_owner() == this);
+						if (upcast(MilitarySite, ms, temp_attacker->get_location(egbase()))) {
+							assert(ms->get_owner() == this);
+							ms->send_attacker(*temp_attacker, *building);
+						} else {
+							// The soldier may not be in a militarysite anymore if he was kicked out
+							// in the short delay between sending and executing a playercommand
+							log("Player(%u)::enemyflagaction: Not sending soldier %u because he left the building\n",
+									player_number(), temp_attacker->serial());
+						}
 					}
 				}
 			}

=== modified file 'src/logic/playercommand.cc'
--- src/logic/playercommand.cc	2019-05-07 12:26:11 +0000
+++ src/logic/playercommand.cc	2019-05-25 10:54:54 +0000
@@ -1609,7 +1609,9 @@
 			    1 < player->vision(Map::get_index(building->get_position(), game.map().get_width()))) {
 				std::vector<Soldier*> result;
 				for (Serial s : soldiers) {
-					result.push_back(dynamic_cast<Soldier*>(game.objects().get_object(s)));
+					if (Soldier* soldier = dynamic_cast<Soldier*>(game.objects().get_object(s))) {
+						result.push_back(soldier);
+					}
 				}
 				player->enemyflagaction(*flag, sender(), result);
 			} else {