Merge lp:~widelands-dev/widelands/bug-1636966-one-soldier-crash into lp:widelands/build19

[GunChleoc]

Abort battle if opponent is nullptr to avoid segfaults.

There is no easy way to reproduce this, so I have followed the solution suggested in the bug.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 1524. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/171031811.
Appveyor build 1367. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1636966_one_soldier_crash-1367.

[SirVer (sirver)]

I feel uneasy getting this into b19.

We do not know what the root cause of the bug is - why the other soldier can be zero. We know it happens - which breaks our codes assumptions already. This branch deals with this unexpected situation, potentially carrying it further and masking more bugs down the line. My argument is that crashing early is way better than hiding further bugs.

A quick qblame showed me that this code was introduced in r5877 in 2011 to fix bug 612348 - to which we also did not have a good understanding it seems. It seems this bug has been around sufficiently long to not warrant this fix for b19.

Agreed?

[GunChleoc (gunchleoc)]

You have convinced me.

=== modified file 'src/logic/map_objects/tribes/soldier.cc'
--- src/logic/map_objects/tribes/soldier.cc	2016-08-04 15:49:05 +0000
+++ src/logic/map_objects/tribes/soldier.cc	2016-10-27 08:33:31 +0000
@@ -1253,8 +1253,14 @@
 	}
 
 	Map& map = game.map();
-	Soldier& opponent = *battle_->opponent(*this);
-	if (opponent.get_position() != get_position()) {
+	Soldier* opponent = battle_->opponent(*this);
+	// If the opponent disappeared from under us, cancel the battle.
+	// TODO(GunChleoc): Analyze why the opponent can be nullptr.
+	if (!opponent) {
+		battle_->cancel(game, *this);
+		return;
+	}
+	if (opponent->get_position() != get_position()) {
 		if (is_a(Building, map[get_position()].get_immovable())) {
 			// Note that this does not use the "leavebuilding" task,
 			// because that task is geared towards orderly workers leaving
@@ -1273,19 +1279,19 @@
 			return skip_act();  //  we will get a signal via set_battle()
 		} else {
 			if (combat_walking_ != CD_COMBAT_E) {
-				opponent.send_signal(game, "wakeup");
+				opponent->send_signal(game, "wakeup");
 				return start_task_move_in_battle(game, CD_WALK_E);
 			}
 		}
 	} else {
-		if (opponent.stay_home() && (this == battle_->second())) {
+		if (opponent->stay_home() && (this == battle_->second())) {
 			// Wait until correct roles are assigned
 			new Battle(game, *battle_->second(), *battle_->first());
 			return schedule_act(game, 10);
 		}
 
-		if (opponent.get_position() != get_position()) {
-			Coords dest = opponent.get_position();
+		if (opponent->get_position() != get_position()) {
+			Coords dest = opponent->get_position();
 
 			if (upcast(Building, building, map[dest].get_immovable()))
 				dest = building->base_flag().get_position();
@@ -1318,8 +1324,8 @@
 					    static_cast<unsigned int>(owner().player_number()) % get_position().x %
 					    get_position().y %
 					    (immovable_position ? immovable_position->descr().descname().c_str() : ("no")) %
-					    opponent.descr().descname().c_str() % opponent.serial() %
-					    static_cast<unsigned int>(opponent.owner().player_number()) % dest.x % dest.y %
+					    opponent->descr().descname().c_str() % opponent->serial() %
+					    static_cast<unsigned int>(opponent->owner().player_number()) % dest.x % dest.y %
 					    (immovable_dest ? immovable_dest->descr().descname().c_str() : ("no")) %
 					    descr().descname().c_str())
 					      .str();
@@ -1329,22 +1335,22 @@
 					            "images/ui_basic/menu_help.png", _("Logic error"),
 					            (boost::format("<rt><p font-size=12>%s</p></rt>") % messagetext).str(),
 					            get_position(), serial_));
-					opponent.owner().add_message(
+					opponent->owner().add_message(
 					   game, *new Message(
 					            Message::Type::kGameLogic, game.get_gametime(), descr().descname(),
 					            "images/ui_basic/menu_help.png", _("Logic error"),
 					            (boost::format("<rt><p font-size=12>%s</p></rt>") % messagetext).str(),
-					            opponent.get_position(), serial_));
+					            opponent->get_position(), serial_));
 					game.game_controller()->set_desired_speed(0);
 					return pop_task(game);
 				}
 			}
 		} else {
-			assert(opponent.get_position() == get_position());
-			assert(battle_ == opponent.get_battle());
+			assert(opponent->get_position() == get_position());
+			assert(battle_ == opponent->get_battle());
 
-			if (opponent.is_walking()) {
-				molog("[battle]: Opponent '%d' is walking, sleeping\n", opponent.serial());
+			if (opponent->is_walking()) {
+				molog("[battle]: Opponent '%d' is walking, sleeping\n", opponent->serial());
 				// We should be woken up by our opponent, but add a timeout anyway for robustness
 				return start_task_idle(game, descr().get_animation("idle"), 5000);
 			}
@@ -1352,13 +1358,13 @@
 			if (battle_->first()->serial() == serial()) {
 				if (combat_walking_ != CD_COMBAT_W) {
 					molog("[battle]: Moving west\n");
-					opponent.send_signal(game, "wakeup");
+					opponent->send_signal(game, "wakeup");
 					return start_task_move_in_battle(game, CD_WALK_W);
 				}
 			} else {
 				if (combat_walking_ != CD_COMBAT_E) {
 					molog("[battle]: Moving east\n");
-					opponent.send_signal(game, "wakeup");
+					opponent->send_signal(game, "wakeup");
 					return start_task_move_in_battle(game, CD_WALK_E);
 				}
 			}