Merge lp:~widelands-dev/widelands/bug-1636966-segfault-in-battle into lp:widelands

[GunChleoc]

Assume that we have no battle if the opponent is nullptr. Added some assertions. And a bit of refactoring.

After going through the code, I am making the following assumption: the opponent has died.

These are the commands sent by Battle when a soldier dies:

    soldier.start_task_die(game);
    molog("[battle] waking up winner %d\n", opponent(soldier)->serial());
    opponent(soldier)->send_signal(game, "wakeup");
    return schedule_destroy(game);

The wakeup signal then ends up leading to the part in the code that crashes. The wakeup is supposed to happen 10 ticks after the Battle is destroyed, but I guess that with very low frametime, this can get its wires crossed. This would also explain the rarity of the crash. I haven't dug really deeply into this part though.

There is already a comment in the code that running the command queue depends on frametime and that it's a bad idea.

[Notabilis (notabilis27)]

I think your argumentation makes sense and it is quite likely that the game was lagging when I encountered the bug. I wasn't able to force the bug by fighting with a low framerate, though.
Diff looks good, too.

When I read the code right, the soldier dies / is deleted after 1 second gametime while the opponent receives the signal after 10 ticks. Seems kind of strange to use two "time"scales in the game, maybe someone should try to clean this up at some time.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 2225. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/234398380.
Appveyor build 2060. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1636966_segfault_in_battle-2060.

[GunChleoc (gunchleoc)]

Thanks for the review :)

@bunnybot merge

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 2229. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/234503157.
Appveyor build 2064. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1636966_segfault_in_battle-2064.

=== modified file 'src/logic/map_objects/tribes/battle.cc'
--- src/logic/map_objects/tribes/battle.cc	2017-04-23 12:11:19 +0000
+++ src/logic/map_objects/tribes/battle.cc	2017-05-21 09:25:30 +0000
@@ -53,23 +53,23 @@
      last_attack_hits_(false) {
 }
 
-Battle::Battle(Game& game, Soldier& First, Soldier& Second)
+Battle::Battle(Game& game, Soldier* first_soldier, Soldier* second_soldier)
    : MapObject(&g_battle_descr),
-     first_(&First),
-     second_(&Second),
+     first_(first_soldier),
+     second_(second_soldier),
      readyflags_(0),
      damage_(0),
      first_strikes_(true) {
-	assert(First.get_owner() != Second.get_owner());
+	assert(first_soldier->get_owner() != second_soldier->get_owner());
 	{
 		StreamWrite& ss = game.syncstream();
 		ss.unsigned_32(0x00e111ba);  // appears as ba111e00 in a hexdump
-		ss.unsigned_32(First.serial());
-		ss.unsigned_32(Second.serial());
+		ss.unsigned_32(first_soldier->serial());
+		ss.unsigned_32(second_soldier->serial());
 	}
 
-	// Ensures only live soldiers eganges in a battle
-	assert(First.get_current_health() && Second.get_current_health());
+	// Ensures only live soldiers engage in a battle
+	assert(first_soldier->get_current_health() && second_soldier->get_current_health());
 
 	init(game);
 }
@@ -129,7 +129,8 @@
 	return first_->get_position() == second_->get_position();
 }
 
-Soldier* Battle::opponent(Soldier& soldier) {
+Soldier* Battle::opponent(const Soldier& soldier) const {
+	assert(&soldier != nullptr);
 	assert(first_ == &soldier || second_ == &soldier);
 	Soldier* other_soldier = first_ == &soldier ? second_ : first_;
 	return other_soldier;

=== modified file 'src/logic/map_objects/tribes/battle.h'
--- src/logic/map_objects/tribes/battle.h	2017-04-23 12:11:19 +0000
+++ src/logic/map_objects/tribes/battle.h	2017-05-21 09:25:30 +0000
@@ -49,7 +49,7 @@
 	const BattleDescr& descr() const;
 
 	Battle();                           //  for loading an existing battle from a savegame
-	Battle(Game&, Soldier&, Soldier&);  //  to create a new battle in the game
+	Battle(Game&, Soldier*, Soldier*);  //  to create a new battle in the game
 
 	// Implements MapObject.
 	bool init(EditorGameBase&) override;
@@ -75,9 +75,9 @@
 	}
 
 	// Returns the other soldier involved in this battle. CHECKs that the given
-	// soldier is participating in this battle. Can return nullptr, but I have
-	// no idea what that means.
-	Soldier* opponent(Soldier&);
+	// soldier is participating in this battle. Can return nullptr, probably when the
+	// opponent has died.
+	Soldier* opponent(const Soldier&) const;
 
 	// Called by the battling soldiers once they've met on a common node and are
 	// idle.

=== modified file 'src/logic/map_objects/tribes/soldier.cc'
--- src/logic/map_objects/tribes/soldier.cc	2017-05-13 18:48:26 +0000
+++ src/logic/map_objects/tribes/soldier.cc	2017-05-21 09:25:30 +0000
@@ -1047,7 +1047,8 @@
 			for (Bob* temp_bob : soldiers) {
 				if (upcast(Soldier, temp_soldier, temp_bob)) {
 					if (temp_soldier->can_be_challenged()) {
-						new Battle(game, *this, *temp_soldier);
+						assert(temp_soldier != nullptr);
+						new Battle(game, this, temp_soldier);
 						return start_task_battle(game);
 					}
 				}
@@ -1134,7 +1135,8 @@
 
 		if (target.dist <= 1) {
 			molog("[defense] starting battle with %u!\n", target.s->serial());
-			new Battle(game, *this, *(target.s));
+			assert(target.s != nullptr);
+			new Battle(game, this, target.s);
 			return start_task_battle(game);
 		}
 
@@ -1257,7 +1259,8 @@
 		}
 	}
 
-	if (!battle_) {
+	// The opponent might have died on us
+	if (!battle_ || battle_->opponent(*this) == nullptr) {
 		if (combat_walking_ == CD_COMBAT_W) {
 			return start_task_move_in_battle(game, CD_RETURN_W);
 		}
@@ -1287,7 +1290,7 @@
 	if (stay_home()) {
 		if (this == battle_->first()) {
 			molog("[battle] stay_home, so reverse roles\n");
-			new Battle(game, *battle_->second(), *battle_->first());
+			new Battle(game, battle_->second(), battle_->first());
 			return skip_act();  //  we will get a signal via set_battle()
 		} else {
 			if (combat_walking_ != CD_COMBAT_E) {
@@ -1298,7 +1301,7 @@
 	} else {
 		if (opponent.stay_home() && (this == battle_->second())) {
 			// Wait until correct roles are assigned
-			new Battle(game, *battle_->second(), *battle_->first());
+			new Battle(game, battle_->second(), battle_->first());
 			return schedule_act(game, 10);
 		}
 
@@ -1488,7 +1491,7 @@
 		if (commit && !foundbattle && !multiplesoldiers) {
 			if (foundsoldier->owner().is_hostile(*get_owner()) && foundsoldier->can_be_challenged()) {
 				molog("[check_node_blocked] attacking a soldier (%u)\n", foundsoldier->serial());
-				new Battle(game, *this, *foundsoldier);
+				new Battle(game, this, foundsoldier);
 			}
 		}