widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

GunChleoc has proposed merging lp:~widelands-dev/widelands/bug-1734046-statistics_menu into lp:widelands.

Commit message:
Replaced UniqueWindow::Registry on_create and on_delete with boost signals. This fixes a memory problem.

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1734046 in widelands: "Use of deallocated memory from statistics menu"
  https://bugs.launchpad.net/widelands/+bug/1734046

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1734046-statistics_menu/+merge/334398

The problem was that after the game statistics window is closed and a child windows still open, the on_delete function was still linked with boost::bind.

The windows that have graphs in them are still leaking memory, but that is a different problem. ASan report should be clear now for all other toolbar and game statistics windows.
-- 
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1734046-statistics_menu into lp:widelands.

=== modified file 'src/ui_basic/unique_window.cc'
--- src/ui_basic/unique_window.cc	2017-02-22 07:14:15 +0000
+++ src/ui_basic/unique_window.cc	2017-11-28 20:46:50 +0000
@@ -41,6 +41,7 @@
 			window->restore();
 		window->move_to_top();
 	}
+	opened();
 }
 
 /**
@@ -50,6 +51,7 @@
 	if (window) {
 		window->die();
 	}
+	closed();
 }
 
 /**
@@ -58,8 +60,10 @@
 void UniqueWindow::Registry::toggle() {
 	if (window) {
 		window->die();
+		closed();
 	} else {
 		open_window();
+		opened();
 	}
 }
 
@@ -69,21 +73,7 @@
 */
 UniqueWindow::Registry::~Registry() {
 	delete window;
-}
-
-void UniqueWindow::Registry::assign_toggle_button(UI::Button* button) {
-	assert(!on_create);
-	assert(!on_delete);
-	on_create = boost::bind(&UI::Button::set_style, button, UI::Button::Style::kPermpressed);
-	on_delete = boost::bind(&UI::Button::set_style, button, UI::Button::Style::kRaised);
-	if (window) {
-		button->set_style(UI::Button::Style::kPermpressed);
-	}
-}
-
-void UniqueWindow::Registry::unassign_toggle_button() {
-	on_create = 0;
-	on_delete = 0;
+	closed();
 }
 
 /**
@@ -104,9 +94,6 @@
 			set_pos(Vector2i(registry_->x, registry_->y));
 			usedefaultpos_ = false;
 		}
-		if (registry_->on_create) {
-			registry_->on_create();
-		}
 	}
 }
 
@@ -121,10 +108,6 @@
 		registry_->x = get_x();
 		registry_->y = get_y();
 		registry_->valid_pos = true;
-
-		if (registry_->on_delete) {
-			registry_->on_delete();
-		}
 	}
 }
 }

=== modified file 'src/ui_basic/unique_window.h'
--- src/ui_basic/unique_window.h	2017-01-25 18:55:59 +0000
+++ src/ui_basic/unique_window.h	2017-11-28 20:46:50 +0000
@@ -22,6 +22,8 @@
 
 #include <functional>
 
+#include <boost/signals2.hpp>
+
 #include "ui_basic/button.h"
 #include "ui_basic/window.h"
 
@@ -41,11 +43,10 @@
 		// closed.
 		std::function<void()> open_window;
 
-		// Called when the window opens.
-		std::function<void()> on_create;
-
-		// Called when the window is deleted (i.e. closed).
-		std::function<void()> on_delete;
+		// Triggered when the window opens
+		boost::signals2::signal<void()> opened;
+		// Triggered when the window closes
+		boost::signals2::signal<void()> closed;
 
 		void create();
 		void destroy();
@@ -60,15 +61,6 @@
 		Registry() : window(nullptr), x(0), y(0), valid_pos(false) {
 		}
 		~Registry();
-
-		/// The 'button' will be permpressed or not depending on whether this window is
-		/// open (on_create/on_delete callback function hooks).
-		/// This can be assigned only once.
-		void assign_toggle_button(UI::Button* button);
-
-		/// Remove the callback as a safeguard in case somewhere else in the
-		/// code someone would overwrite our hooks.
-		void unassign_toggle_button();
 	};
 
 	UniqueWindow(Panel* parent,

=== modified file 'src/wui/game_options_menu.cc'
--- src/wui/game_options_menu.cc	2017-02-21 07:56:18 +0000
+++ src/wui/game_options_menu.cc	2017-11-28 20:46:50 +0000
@@ -114,14 +114,14 @@
 	exit_game_.sigclicked.connect(
 	   boost::bind(&GameOptionsMenu::clicked_exit_game, boost::ref(*this)));
 
-	windows_.sound_options.assign_toggle_button(&sound_);
+	windows_.sound_options.opened.connect(boost::bind(&UI::Button::set_style, &sound_, UI::Button::Style::kPermpressed));
+	windows_.sound_options.closed.connect(boost::bind(&UI::Button::set_style, &sound_, UI::Button::Style::kRaised));
 
 	if (get_usedefaultpos())
 		center_to_parent();
 }
 
 GameOptionsMenu::~GameOptionsMenu() {
-	windows_.sound_options.unassign_toggle_button();
 }
 
 void GameOptionsMenu::clicked_save_game() {

=== modified file 'src/wui/game_statistics_menu.cc'
--- src/wui/game_statistics_menu.cc	2017-02-21 07:56:18 +0000
+++ src/wui/game_statistics_menu.cc	2017-11-28 20:46:50 +0000
@@ -72,10 +72,8 @@
 	                  g_gr->images().get("images/" + image_basename + ".png"), tooltip_text);
 	box_.add(button);
 	if (window) {
-		if (!window->on_create) {
-			window->assign_toggle_button(button);
-			registries_.push_back(*window);
-		}
+		window->opened.connect(boost::bind(&UI::Button::set_style, button, UI::Button::Style::kPermpressed));
+		window->closed.connect(boost::bind(&UI::Button::set_style, button, UI::Button::Style::kRaised));
 		button->sigclicked.connect(
 		   boost::bind(&UI::UniqueWindow::Registry::toggle, boost::ref(*window)));
 	}
@@ -83,7 +81,4 @@
 }
 
 GameStatisticsMenu::~GameStatisticsMenu() {
-	for (auto& registry : registries_) {
-		registry.unassign_toggle_button();
-	}
 }

=== modified file 'src/wui/game_statistics_menu.h'
--- src/wui/game_statistics_menu.h	2017-02-12 09:10:57 +0000
+++ src/wui/game_statistics_menu.h	2017-11-28 20:46:50 +0000
@@ -47,10 +47,6 @@
 	InteractivePlayer& player_;
 	InteractivePlayer::GameMainMenuWindows& windows_;
 	UI::Box box_;
-
-	// These get collected by add_button
-	// so we can call unassign_toggle_button on them in the destructor.
-	std::vector<UI::UniqueWindow::Registry> registries_;
 };
 
 #endif  // end of include guard: WL_WUI_GAME_STATISTICS_MENU_H

=== modified file 'src/wui/interactive_base.cc'
--- src/wui/interactive_base.cc	2017-09-17 18:17:50 +0000
+++ src/wui/interactive_base.cc	2017-11-28 20:46:50 +0000
@@ -192,9 +192,6 @@
 	if (buildroad_) {
 		abort_build_road();
 	}
-	for (auto& registry : registries_) {
-		registry.unassign_toggle_button();
-	}
 }
 
 const InteractiveBase::BuildhelpOverlay*
@@ -286,8 +283,9 @@
 	   g_gr->images().get("images/" + image_basename + ".png"), tooltip_text);
 	toolbar_.add(button);
 	if (window) {
-		window->assign_toggle_button(button);
-		registries_.push_back(*window);
+		window->opened.connect(boost::bind(&UI::Button::set_style, button, UI::Button::Style::kPermpressed));
+		window->closed.connect(boost::bind(&UI::Button::set_style, button, UI::Button::Style::kRaised));
+
 		if (bind_default_toggle) {
 			button->sigclicked.connect(
 			   boost::bind(&UI::UniqueWindow::Registry::toggle, boost::ref(*window)));

=== modified file 'src/wui/interactive_base.h'
--- src/wui/interactive_base.h	2017-09-13 07:27:00 +0000
+++ src/wui/interactive_base.h	2017-11-28 20:46:50 +0000
@@ -260,10 +260,6 @@
 	MapView map_view_;
 	ChatOverlay* chat_overlay_;
 
-	// These get collected by add_toolbar_button
-	// so we can call unassign_toggle_button on them in the destructor.
-	std::vector<UI::UniqueWindow::Registry> registries_;
-
 	UI::Box toolbar_;
 	// No unique_ptr on purpose: 'minimap_' is a UniqueWindow, its parent will
 	// delete it.