Configure economy window disappears if only headquarter is available

[kaputtnik]

Steps to reproduce:

1. Start a new game, e.g. with map Checkmate
2. Click on the flag of the headquarter and open the Configure economy settings
3. Click on the flag of the headquarter again

Result:
The window of configure economy will disappear. Sometimes immediately, sometimes after a short time (1 second or so)

Posted on the forums by LightonMan: https://wl.widelands.org/forum/topic/4143/

[GunChleoc (gunchleoc)]

I can't reproduce this.

[GunChleoc (gunchleoc)]

Ah, I need the full description from the forum - this does reproduce:

Here's the sequence: Start widelands from a terminal (only so I can see messages) Press spacebar to bring up the next screen Click on single player; then on New Game Select "Checkmate" ; click on OK; then Start game

All usual stuff and the game starts with the messages window displayed; Right click on the messages window to close it. Click on the Flag outside the Headquarters (At this stage it's the only one!) Click on the configure economy icon to bring up the economy options window Press spacebar to display building sites Build something linking the road back to headquarters using ctrl-click - I built a quarry next to the rocks at the bottom of the screen. Build something (another quarry, a mine, a well, whatever) using ctrl-click to link the road back to Headquarters As soon as I click on the Headquarters flag to build the second road, the economy options window disappears!

[kaputtnik (franku)]

During testing today the configure economy window disappears close after open it the first time, without clicking twice on the flag.

[Leighton Man (leightonman)]

The first time I encountered this was after several hours of play when the configure economy window became unusable as it was only visible for a few seconds before disappearing. I saved the game and tried to find a way to reproduce the problem. I got as far as when I had three barracks, stopped two and then dismantled one of the stopped ones the bug appeared. I also tried to find a sequence of simple events which produced the bug. That led to the sequence above but as the final check before posting, I upgraded to bzr8490(trunk) and retested the sequence. Interestingly, the saved game no longer exhibits the bug and plays normally!

[GunChleoc (gunchleoc)]

I can still reproduce this with bzr8490.

[Leighton Man (leightonman)]

Yes, the sequence I posted on the forum still works

[Leighton Man (leightonman)]

Here's another variation which is 100% repeatable on my system. Follow the instructions in past post to start the game. Select a map. I have tried about six different ones and it works for all. Start the game and right click on the messages window to close it. Click on the flag outside headquarters and open the configure economy window. A second or two later it closes. Do not not press space bar to show building sites.

[Jukka Pakarinen (flegu)]

I can reproduce both #2 and #7 with bzr8495 on Debian 9.1. I also found an other way to reproduce this,

Start the game and right click on the messages window to close it.
Build a building
Do not connect the building to the headquaters.
Cancel road building
Click the flag of the building and open the configure economy window
Wait a moment and the window disappears
You can open multiple times the configure economy window and it always disappears, only waiting time vary.

I think the new way to reproduce the bug is same as #7 bacause the building can be destroyed and the flag which stays there can still reporduce the problem. But if I add an other flag and open the configure economy window from there, the window doesn't disappear.

I did this with Valgrind and the following happened ones. I'm not sure which triggered it but it was definitely when I played with flags and the configure economy window.

==28544== Invalid write of size 1
==28544== at 0x100535B: Widelands::Economy::set_has_window(bool) (economy.h:198)
==28544== by 0x1003529: EconomyOptionsWindow::~EconomyOptionsWindow() (economy_options_window.cc:56)
==28544== by 0x1003575: EconomyOptionsWindow::~EconomyOptionsWindow() (economy_options_window.cc:57)
==28544== by 0xD0D972: UI::Panel::free_children() (panel.cc:127)
==28544== by 0xD0D81F: UI::Panel::~Panel() (panel.cc:99)
==28544== by 0xD9C52A: InteractiveBase::~InteractiveBase() (interactive_base.cc:191)
==28544== by 0xDBCB81: InteractiveGameBase::~InteractiveGameBase() (interactive_gamebase.h:57)
==28544== by 0xDBE427: InteractivePlayer::~InteractivePlayer() (interactive_player.h:44)
==28544== by 0xDBE44F: InteractivePlayer::~InteractivePlayer() (interactive_player.h:44)
==28544== by 0xB46BAF: std::default_delete<InteractiveBase>::operator()(InteractiveBase*) const (unique_ptr.h:76)
==28544== by 0xB45CFE: std::unique_ptr<InteractiveBase, std::default_delete<InteractiveBase> >::reset(InteractiveBase*) (unique_ptr.h:347)
==28544== by 0xB40F4E: Widelands::EditorGameBase::set_ibase(InteractiveBase*) (editor_game_base.cc:138)
==28544== Address 0x20c2efbc is 252 bytes inside a block of size 304 free'd
==28544== at 0x4C2D2DB: operator delete(void*) (vg_replace_malloc.c:576)
==28544== by 0xF3AABC: Widelands::Economy::remove_flag(Widelands::Flag&) (economy.cc:275)
==28544== by 0xF50AC4: Widelands::Flag::cleanup(Widelands::EditorGameBase&) (flag.cc:693)
==28544== by 0xBC15D4: Widelands::MapObject::remove(Widelands::EditorGameBase&) (map_object.cc:405)
==28544== by 0xBC02EF: Widelands::ObjectManager::cleanup(Widelands::EditorGameBase&) (map_object.cc:155)
==28544== by 0xA50791: Widelands::EditorGameBase::cleanup_objects() (editor_game_base.h:168)
==28544== by 0xB515B8: Widelands::Game::run(UI::ProgressWindow*, Widelands::Game::StartGameType, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, bool, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (game.cc:522)
==28544== by 0xA28306: WLApplication::new_game() (wlapplication.cc:1254)
==28544== by 0xA27713: WLApplication::mainmenu_singleplayer() (wlapplicati...

[Jukka Pakarinen (flegu)]

I got the Valgrind message again and it popped up when I destroyed the flag before the configure economy window disappears.

[Jukka Pakarinen (flegu)]

When the economy options window is linked to an unconnected (unfinnished) building or a flag, the window is removed after some time period.

If I start a game and open the economy window from the flag of Headquaters, the window disappears at first, but after opening it again it stays on. Then, if I start to build a building without connecting it to the Headquaters and open the economy window from the flag of the building, the window disappears. But after I have connected the building to the Headquaters it doesn't disappear.

I think that there is some routine which checks things and removes objects that are not linked to the economy.

The Valgrind log I posted seems to be just an effect from the periodically happening cleaning process but it may lead to the origin of the problem.

I looked the code few hours and couldn't figure out how it works. There is too many raw pointers and references between objects.

[GunChleoc (gunchleoc)]

We have 2 things happening here:

1. Something in the economy merge code goes wrong, which makes the window disappear. The code is clean here, but semantically wrong. It also only happens when I have an economy that consists of 1 building and 1 flag. It can be any building type.

2. When destroying the window, its owner_ might already be nullptr or something, triggering the memory leak.

I can reproduce 2. like this:

- Build an economy that has at least 2 flags
- Open an economy window
- Leave the game

[SirVer (sirver)]

I found two issues in the code.

1) EconomyNote does not contain the player number - so the economy option window subscribes to all changes of an economy. The computer player creates and deletes economies (his economies), but the window thinks its economies have changed and closes itself. Simple fix: add a player number NoteEconomy. This is fixed in https://code.launchpad.net/~widelands-dev/widelands/add_player_to_note_economy/+merge/334371, first commit.

2) The second one is harder to fix. This is what happens:
- When you build a lumberjack, it creates a new economy, so we have {eco0 (headquarters), eco1 (lumberjack)}
- Now you connect the road.
- This calls Economy::merge, like eco1.merge(eco0).
- This will send NoteEconomy{kMerged, 0 into 1}. The window updates that it should care for eco1 (with economy_number_ = 1).
- Last line of Economy::merge is 'delete &eco0'.
- This calls the destructor of Economy which calls Player::remove_economy. It also sends a Note that economy0 is deleted, but nobody cares about that.
- Player::remove_economy looks in its vector of economies for &eco0, finds it at the beginning and removes it. Now there is only one economy, conceptually with an economy_number_ = 0.
- Next frame, EconomyOptionsWindow calls player->get_economy_by_number(1) which crashes now.

The problem is that the economy_number is not uniquely identifying the economy, since they are relabeled on deletion. The player API get_economy_by_number() is dangerous and misleading. I suggest two changes, but I might not have time to make them both:

1) Change NoteEconomy to identify economies by pointer, instead of (player_number, index). This should work fine since this is only for UI connections, nothing that will ever be send over the network. I did this in https://code.launchpad.net/~widelands-dev/widelands/add_player_to_note_economy/+merge/334371, second commit.

This will fix this bug, but it still has the problem that economy_numbers do not identify economies. For example, CmdSetWareTargetQuantity (besides others) uses the number as well. It could happen that the economy number no longer identifies the same economy after the command arrived through the network which will still crash or change the wrong economy.

Fundamentally these changes should happen:

1) Remove Player::get_economy_by_number and Player::get_nr_economies. Replace through a const std::vector<Economy*>& Player::economies(). In all but two places we iterate all economies anyways which will make that explicit.

2) Change economy_number to an economy_id that is unique and monotonously increasing and also doesn't change through loading and saving. Very similar to MapObject.serial(). This is the only way to fixing the underlying issue proper.

[GunChleoc (gunchleoc)]

For the economy_id, how about having a static Serial Economy::last_economy_id_ to identify the economy numbers, then increment the number any time a new economy is created?

Player::economies() could also be a std::map<Serial, unique_ptr<Economy>> with serials matching the economy ids.

I'm all for getting rid of economy number per player, it was such a pain in the butt when I first implemented the notification for this window.

[SirVer (sirver)]

I fully agree with #13, sounds good. Of course last_economy_ needs to be uniquely serialized somehow .

However, as mentioned, these changes are bigger than I can currently invest.

[Klaus Halfmann (klaus-halfmann)]

Suggested changes in https://code.launchpad.net/~widelands-dev/widelands/bug-1732765-economy-refactoring/+merge/345277 are done.

Was uanble to reproduce this in bzr8713[trunk], though.

[GunChleoc (gunchleoc)]

Fixed in build20-rc1