Merge lp:~flegu/widelands/fix_some_memory_leaks into lp:widelands

[Jukka Pakarinen]

There happens memory leaks detected with Valgrind in multiple menu views. The changes in the branch fixes most of the cases. The leaks were found when the game was built from trunk (revno 8490) on Debian 9.1. I think these leaks are definitely happening on every operating system.

[Klaus Halfmann (klaus-halfmann)]

Hellu Jukka (If got you first name correct).

Thanks for finding/fixing those Issues. We actually have
a long standing problem with Memory corruption when playing
for a longer time e.g. #1730204 so anyonw with knowledge and
tooling is welcome.

If you are a registered widelands developer,
you may consdier putting such branches into the public
space, I can assit you how to do so.

You can find me in the lobby and on widelands.org as Hasi50.
I am devloping mostly on OSX with clang / macports. But I can
switch to Win10 or Ubuntu, if needed.

Now let me check that code

[Klaus Halfmann (klaus-halfmann)]

Compiled and tested all Dialogs I could think for, ok

testing with malloc debug settings:
#! /bin/bash
export MallocGuardEdges=1
export MallocScribble=1
export MallocStackLogging=1
export MallocCheckHeapStart=0
export MallocCheckHeapEach=1
export MallocCheckHeapSleep=600

this gives an segfault on closing, but its the same on trunk.

plese check inline comment. Otherwise code LGTM.

Gun: get me mereg a private bracnh wit bunnybot and all,
or must his me moved to wideldans namespace, first.

review: Approve (compile, test, review)

[Klaus Halfmann (klaus-halfmann)]

Gun: can we merge a private branch with bunnybot and all,
or must this be moved to wideldans namespace, first?

[GunChleoc (gunchleoc)]

Thanks for cleaning up after me :)

Merging this with bunnybot is no problem.

I have added you to widelands-dev, so you we can work together on branches in the future. Just don't push directly to trunk :)

I think we can have a std::unique_ptr here for all the variables that you touched. It is clear who owns it, so we don't need shared_ptr, and raw pointers are something we want to get rid of.

The only raw pointers that are OK are for UI::Panel and its subclasses, because the pointers are managed well within UI::Panel and it would be a huge job to refactor that.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 2809. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/303894554.
Appveyor build 2620. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_flegu_widelands_fix_some_memory_leaks-2620.

[Jukka Pakarinen (flegu)]

> Hellu Jukka (If got you first name correct).
You got it right.

> space, I can assit you how to do so.
Thanks for offering help!

[Jukka Pakarinen (flegu)]

> I have added you to widelands-dev, so you we can work together on branches in
Thanks!

> the future. Just don't push directly to trunk :)
I keep that in mind!

> I think we can have a std::unique_ptr here for all the variables that you
> touched. It is clear who owns it, so we don't need shared_ptr, and raw
> pointers are something we want to get rid of.
Sounds good to me. I can try to change those to unique ones.

> The only raw pointers that are OK are for UI::Panel and its subclasses,
> because the pointers are managed well within UI::Panel and it would be a huge
> job to refactor that.
Good to know!

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 2813. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/304031725.
Appveyor build 2624. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_flegu_widelands_fix_some_memory_leaks-2624.

[GunChleoc (gunchleoc)]

Code LGTM :)

You now need to add some headers to make Codecheck happy, then this can go in.

widelands/src/ui_fsmenu/loadgame.cc:117: This file uses unique_ptr but does not include <memory>.

widelands/src/wui/load_or_save_game.cc:171: This file uses unique_ptr but does not include <memory>.

widelands/src/wui/load_or_save_game.h:46: This file uses unique_ptr but does not include <memory>.

The reason for this rule is that some compilers will barf if unique_ptr is used without this header.

Documentation on how to run codecheck yourself: https://wl.widelands.org/wiki/RegressionTests/

[Jukka Pakarinen (flegu)]

> Documentation on how to run codecheck yourself:
> https://wl.widelands.org/wiki/RegressionTests/

Thanks for guiding me!

[GunChleoc (gunchleoc)]

You're welcome! We're happy to have you on board and helping with those memory leaks :)

[GunChleoc (gunchleoc)]

Now we're getting a compiler error:

Building CXX object src/wui/CMakeFiles/wui_common.dir/mapdetails.cc.o

/home/travis/build/widelands/widelands/src/wui/load_or_save_game.cc: In member function ‘std::unique_ptr<const SavegameData> LoadOrSaveGame::entry_selected()’:

/home/travis/build/widelands/widelands/src/wui/load_or_save_game.cc:201:9: error: could not convert ‘result’ from ‘std::unique_ptr<SavegameData>’ to ‘std::unique_ptr<const SavegameData>’

  return result;

         ^

/home/travis/build/widelands/widelands/src/wui/load_or_save_game.cc:202:1: error: control reaches end of non-void function [-Werror=return-type]

 }

 ^

[Jukka Pakarinen (flegu)]

That's weird! The code compiles fine with gcc 6.3.0. The code doesn't comple if I put "const" where it is missing. Maybe I just drop out the const.

[SirVer (sirver)]

+1e6!!!! thank you for helping fighting these leaks!

review: Approve

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 2824. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/305434917.
Appveyor build 2634. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_flegu_widelands_fix_some_memory_leaks-2634.

[GunChleoc (gunchleoc)]

All ready now! :D

@bunnybot merge

=== modified file 'src/ui_fsmenu/campaign_select.cc'
--- src/ui_fsmenu/campaign_select.cc	2017-03-05 17:55:29 +0000
+++ src/ui_fsmenu/campaign_select.cc	2017-11-21 18:37:33 +0000
@@ -424,10 +424,10 @@
  */
 void FullscreenMenuCampaignMapSelect::fill_table() {
 	// read in the campaign config
-	Profile* prof;
+	std::unique_ptr<Profile> prof;
 	std::string campsection;
 	if (is_tutorial_) {
-		prof = new Profile("campaigns/tutorials.conf", nullptr, "maps");
+		prof.reset(new Profile("campaigns/tutorials.conf", nullptr, "maps"));
 
 		// Set subtitle of the page
 		const std::string subtitle1 = _("Pick a tutorial from the list, then hit \"OK\".");
@@ -439,7 +439,7 @@
 		campsection = "tutorials";
 
 	} else {
-		prof = new Profile("campaigns/campaigns.conf", nullptr, "maps");
+		prof.reset(new Profile("campaigns/campaigns.conf", nullptr, "maps"));
 
 		Section& global_s = prof->get_safe_section("global");
 

=== modified file 'src/ui_fsmenu/loadgame.cc'
--- src/ui_fsmenu/loadgame.cc	2017-11-06 20:19:56 +0000
+++ src/ui_fsmenu/loadgame.cc	2017-11-21 18:37:33 +0000
@@ -19,6 +19,8 @@
 
 #include "ui_fsmenu/loadgame.h"
 
+#include <memory>
+
 #include "base/i18n.h"
 #include "wui/gamedetails.h"
 
@@ -114,7 +116,7 @@
 		return;
 	}
 
-	const SavegameData* gamedata = load_or_save_.entry_selected();
+	std::unique_ptr<SavegameData> gamedata = load_or_save_.entry_selected();
 	if (gamedata && gamedata->errormessage.empty()) {
 		filename_ = gamedata->filename;
 		end_modal<FullscreenMenuBase::MenuTarget>(FullscreenMenuBase::MenuTarget::kOk);

=== modified file 'src/ui_fsmenu/options.cc'
--- src/ui_fsmenu/options.cc	2017-11-12 08:04:06 +0000
+++ src/ui_fsmenu/options.cc	2017-11-21 18:37:33 +0000
@@ -450,7 +450,7 @@
 
 				std::string name = i18n::make_ligatures(table->get_string("name").c_str());
 				const std::string sortname = table->get_string("sort_name");
-				LanguageEntry* entry = new LanguageEntry(localename, name);
+				std::unique_ptr<LanguageEntry> entry(new LanguageEntry(localename, name));
 				entries.insert(std::make_pair(sortname, *entry));
 				language_entries_.insert(std::make_pair(localename, *entry));
 

=== modified file 'src/wui/load_or_save_game.cc'
--- src/wui/load_or_save_game.cc	2017-11-06 20:19:56 +0000
+++ src/wui/load_or_save_game.cc	2017-11-21 18:37:33 +0000
@@ -20,6 +20,7 @@
 #include "wui/load_or_save_game.h"
 
 #include <ctime>
+#include <memory>
 
 #include <boost/algorithm/string.hpp>
 #include <boost/format.hpp>
@@ -168,8 +169,8 @@
 	return r1.savetimestamp < r2.savetimestamp;
 }
 
-const SavegameData* LoadOrSaveGame::entry_selected() {
-	SavegameData* result = new SavegameData();
+std::unique_ptr<SavegameData> LoadOrSaveGame::entry_selected() {
+	std::unique_ptr<SavegameData> result(new SavegameData());
 	size_t selections = table_.selections().size();
 	if (selections == 1) {
 		delete_->set_tooltip(
@@ -178,7 +179,7 @@
 		      _("Delete this replay") :
 		      /** TRANSLATORS: Tooltip for the delete button. The user has selected 1 file */
 		      _("Delete this game"));
-		result = &games_data_[table_.get_selected()];
+		result.reset(new SavegameData(games_data_[table_.get_selected()]));
 	} else if (selections > 1) {
 		delete_->set_tooltip(
 		   filetype_ == FileType::kReplay ?

=== modified file 'src/wui/load_or_save_game.h'
--- src/wui/load_or_save_game.h	2017-08-12 08:03:19 +0000
+++ src/wui/load_or_save_game.h	2017-11-21 18:37:33 +0000
@@ -20,6 +20,8 @@
 #ifndef WL_WUI_LOAD_OR_SAVE_GAME_H
 #define WL_WUI_LOAD_OR_SAVE_GAME_H
 
+#include <memory>
+
 #include "logic/game.h"
 #include "ui_basic/box.h"
 #include "ui_basic/panel.h"
@@ -43,7 +45,7 @@
 	               bool localize_autosave);
 
 	/// Update gamedetails and tooltips and return information about the current selection
-	const SavegameData* entry_selected();
+	std::unique_ptr<SavegameData> entry_selected();
 
 	/// Whether the table has a selection
 	bool has_selection() const;