Use a RAII class to block game event handler cleanup

This ensures that throwing an exception through
game_events::manager::execute_on_events() won't corrupt the stack frame
counter and disable event handler cleanup forever.

I also added a safety check in case there are some kind of ephemeral
event handlers which run a nested game loop and never return. Saving in
such a state wouldn't be safe.

src/game_events/manager.cpp

@@ -35,6 +35,33 @@ static lg::log_domain log_engine("engine");
 static lg::log_domain log_event_handler("event_handler");
 #define DBG_EH LOG_STREAM(debug, log_event_handler)
 
+namespace
+{
+	// Event handlers can't be destroyed as long as at least one of these locks exist.
+	class event_handler_list_lock
+	{
+	public:
+		event_handler_list_lock()
+		{
+			++num_locks_;
+		}
+
+		~event_handler_list_lock()
+		{
+			--num_locks_;
+		}
+
+		static bool none()
+		{
+			return num_locks_ == 0u;
+		}
+	private:
+		static unsigned int num_locks_;
+	};
+
+	unsigned int event_handler_list_lock::num_locks_ = 0u;
+}
+
 namespace game_events
 {
 /** Create an event handler. */
@@ -127,59 +154,64 @@ void manager::execute_on_events(const std::string& event_id, manager::event_func
 	// even if new events are added to the queue.
 	const unsigned saved_end = active_handlers.size();
 
-	// It's possible for this function to call itself again. If list cleanup occurred
-	// after each recursive call, the saved_end variable in the call above it would no
-	// longer be a valid end index. To work around that, we delay cleanup until the
-	// toplevel call is done running through the events (ie, when recursion_depth is 0).
-	static unsigned recursion_depth = 0;
-	++recursion_depth;
 
-	for(unsigned i = 0; i < saved_end; ++i) {
-		handler_ptr handler = nullptr;
+	{
+		// Ensure that event handlers won't be cleaned up while we're iterating them.
+		event_handler_list_lock lock;
 
-		try {
-			handler = active_handlers.at(i);
-		} catch(const std::out_of_range&) {
-			continue;
-		}
+		for (unsigned i = 0; i < saved_end; ++i) {
+			handler_ptr handler = nullptr;
 
-		// Shouldn't happen, but we're just being safe.
-		if(!handler || handler->disabled()) {
-			continue;
-		}
+			try {
+				handler = active_handlers.at(i);
+			}
+			catch (const std::out_of_range&) {
+				continue;
+			}
 
-		// Could be more than one.
-		for(const std::string& name : handler->names()) {
-			bool matches = false;
+			// Shouldn't happen, but we're just being safe.
+			if (!handler || handler->disabled()) {
+				continue;
+			}
 
-			if(utils::might_contain_variables(name)) {
-				// If we don't have gamedata, we can't interpolate variables, so there's
-				// no way the name will match. Move on to the next one in that case.
-				if(!gd) {
-					continue;
+			// Could be more than one.
+			for (const std::string& name : handler->names()) {
+				bool matches = false;
+
+				if (utils::might_contain_variables(name)) {
+					// If we don't have gamedata, we can't interpolate variables, so there's
+					// no way the name will match. Move on to the next one in that case.
+					if (!gd) {
+						continue;
+					}
+
+					matches = standardized_event_id ==
+						event_handlers::standardize_name(utils::interpolate_variables_into_string(name, *gd));
+				}
+				else {
+					matches = standardized_event_id == name;
 				}
 
-				matches = standardized_event_id ==
-					event_handlers::standardize_name(utils::interpolate_variables_into_string(name, *gd));
-			} else {
-				matches = standardized_event_id == name;
-			}
-
-			if(matches) {
-				func(*this, handler);
-				break;
+				if (matches) {
+					func(*this, handler);
+					break;
+				}
 			}
 		}
 	}
 
-	--recursion_depth;
-
 	// Clean up expired ptrs. This saves us effort later since it ensures every ptr is valid.
-	if(recursion_depth == 0) {
+	if(event_handler_list_lock::none()) {
 		event_handlers_->clean_up_expired_handlers(standardized_event_id);
 	}
 }
 
+bool manager::is_event_running() const
+{
+	// If there is an event handler list lock, an event is being processed.
+	return !event_handler_list_lock::none();
+}
+
 game_events::wml_event_pump& manager::pump()
 {
 	return *pump_;

src/game_events/manager.hpp

@@ -73,6 +73,8 @@ public:
 	using event_func_t = std::function<void(game_events::manager&, handler_ptr&)>;
 	void execute_on_events(const std::string& event_id, event_func_t func);
 
+	bool is_event_running() const;
+
 	game_events::wml_event_pump& pump();
 
 	game_events::wmi_manager& wml_menu_items()

src/play_controller.cpp

@@ -820,6 +820,10 @@ replay& play_controller::get_replay() {
 void play_controller::save_game()
 {
 	if(save_blocker::try_block()) {
+		// Saving while an event is running isn't supported
+		// because it may lead to expired event handlers being saved.
+		assert(!gamestate().events_manager_->is_event_running());
+
 		save_blocker::save_unblocker unblocker;
 		scoped_savegame_snapshot snapshot(*this);
 		savegame::ingame_savegame save(saved_game_, preferences::save_compression_format());