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

Proposed by Benedikt Straub
Status: Merged
Merged at revision: 9124
Proposed branch: lp:~widelands-dev/widelands/bug-1829623-assert_is_present
Merge into: lp:widelands
Diff against target: 63 lines (+24/-5)
3 files modified
src/logic/map_objects/tribes/militarysite.cc (+10/-2)
src/logic/player.cc (+11/-2)
src/logic/playercommand.cc (+3/-1)
To merge this branch: bzr merge lp:~widelands-dev/widelands/bug-1829623-assert_is_present
Reviewer Review Type Date Requested Status
GunChleoc Approve
Benedikt Straub Needs Resubmitting
Review via email: mp+367912@code.launchpad.net

Commit message

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

To post a comment you must log in.
Revision history for this message
bunnybot (widelandsofficial) wrote :

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.

Revision history for this message
GunChleoc (gunchleoc) wrote :

Code LTGM.

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

review: Approve
Revision history for this message
GunChleoc (gunchleoc) wrote :

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

review: Needs Fixing
9118. By Nordfriese

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

Revision history for this message
Benedikt Straub (nordfriese) wrote :

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
Revision history for this message
GunChleoc (gunchleoc) wrote :

This fixed it, thanks!

@bunnybot merge

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/logic/map_objects/tribes/militarysite.cc'
2--- src/logic/map_objects/tribes/militarysite.cc 2019-05-18 20:43:25 +0000
3+++ src/logic/map_objects/tribes/militarysite.cc 2019-05-25 10:54:54 +0000
4@@ -885,10 +885,18 @@
5 }
6
7 void MilitarySite::send_attacker(Soldier& soldier, Building& target) {
8- assert(is_present(soldier));
9+ if (!is_present(soldier)) {
10+ // The soldier may not be present anymore due to having been kicked out. Most of the time
11+ // the function calling us will notice this, but there are cornercase where it might not,
12+ // e.g. when a soldier was ordered to leave but did not physically quit the building yet.
13+ log("MilitarySite(%3dx%3d)::send_attacker: Not sending soldier %u because he left the building\n",
14+ get_position().x, get_position().y, soldier.serial());
15+ return;
16+ }
17
18- if (has_soldier_job(soldier))
19+ if (has_soldier_job(soldier)) {
20 return;
21+ }
22
23 SoldierJob sj;
24 sj.soldier = &soldier;
25
26=== modified file 'src/logic/player.cc'
27--- src/logic/player.cc 2019-05-25 10:47:18 +0000
28+++ src/logic/player.cc 2019-05-25 10:54:54 +0000
29@@ -962,8 +962,17 @@
30 if (const AttackTarget* attack_target = building->attack_target()) {
31 if (attack_target->can_be_attacked()) {
32 for (Soldier* temp_attacker : soldiers) {
33- upcast(MilitarySite, ms, temp_attacker->get_location(egbase()));
34- ms->send_attacker(*temp_attacker, *building);
35+ assert(temp_attacker);
36+ assert(temp_attacker->get_owner() == this);
37+ if (upcast(MilitarySite, ms, temp_attacker->get_location(egbase()))) {
38+ assert(ms->get_owner() == this);
39+ ms->send_attacker(*temp_attacker, *building);
40+ } else {
41+ // The soldier may not be in a militarysite anymore if he was kicked out
42+ // in the short delay between sending and executing a playercommand
43+ log("Player(%u)::enemyflagaction: Not sending soldier %u because he left the building\n",
44+ player_number(), temp_attacker->serial());
45+ }
46 }
47 }
48 }
49
50=== modified file 'src/logic/playercommand.cc'
51--- src/logic/playercommand.cc 2019-05-07 12:26:11 +0000
52+++ src/logic/playercommand.cc 2019-05-25 10:54:54 +0000
53@@ -1609,7 +1609,9 @@
54 1 < player->vision(Map::get_index(building->get_position(), game.map().get_width()))) {
55 std::vector<Soldier*> result;
56 for (Serial s : soldiers) {
57- result.push_back(dynamic_cast<Soldier*>(game.objects().get_object(s)));
58+ if (Soldier* soldier = dynamic_cast<Soldier*>(game.objects().get_object(s))) {
59+ result.push_back(soldier);
60+ }
61 }
62 player->enemyflagaction(*flag, sender(), result);
63 } else {

Subscribers

People subscribed via source and target branches

to status/vote changes: