Fix a memory leak in map::generate reported by LSAN in the CI build

[Qrox]

Summary

SUMMARY: Bugfixes "Fix a memory leak during map generation"

Purpose of change

Recent CI builds consistently reported a memory leak in map::generate, for example in this and this report. After extensive test and analysis I finally found the reason. Basically it goes like this:

In the following lines in map::generate, a map extra has a chance to be applied (MapExtras::apply_function) when generating an overmap chunk (2x2 submaps). When this happens, the new submaps constructed by the map has not yet been added to the map buffer, so if the map is a tinymap, and the overmap chunk was not previous generated, the submaps in the map buffer is still nullptr at this time.

Cataclysm-DDA/src/mapgen.cpp

Lines 145 to 154 in 5f963d1

	// At some point, we should add region information so we can grab the appropriate extras
	map_extras ex = region_settings_map["default"].region_extras[terrain_type->get_extras()];
	if( ex.chance > 0 && one_in( ex.chance ) ) {
	std::string *extra = ex.values.pick();
	if( extra == nullptr ) {
	debugmsg( "failed to pick extra for type %s", terrain_type->get_extras() );
	} else {
	MapExtras::apply_function( *( ex.values.pick() ), *this, abs_sub );
	}
	}

However, in MapExtras::apply_function, run_mapgen_update_func is called, which in turn calls update_mapgen_function_json::update_map( const tripoint &, const point &, mission *, bool ), in which a tinymap is further constructed and loaded (map::load). Since the submaps generated before are yet to be added to the map buffer, map::loadn determines (wrongly) that the submaps are not yet generated, and calls map::generate again, which constructs yet another set of new submaps and save them to the map buffer (or, if map extras are again randomly applied in this call to map::generate, the nested calls will continue until no map extra is applied in a final call to map::generate). When the final nested call to map::generate finishes, the outer calls to map::generate fail to add their own constructed submaps to the map buffer (since submaps have already been added to the corresponding submap locations in the final call to map::generate), but do not deallocate them, causing the memory leaks reported by LSAN.

Cataclysm-DDA/src/map_extras.cpp

Lines 2749 to 2752 in 5f963d1

	case map_extra_method::update_mapgen: {
	run_mapgen_update_func( extra.generator_id, sm_to_omt_copy( abs_sub ) );
	break;
	}

Cataclysm-DDA/src/mapgen.cpp

Lines 7746 to 7751 in 5f963d1

	bool update_mapgen_function_json::update_map( const tripoint &omt_pos, const point &offset,
	mission *miss, bool verify ) const
	{
	tinymap update_tmap;
	const tripoint sm_pos = omt_to_sm_copy( omt_pos );
	update_tmap.load( sm_pos, true );

This also explains why these memory leaks have been intermittent among recent CI builds, since the memory leaks only happen if a map extra is applied in map::generate.

Describe the solution

1. Moved map rotation in update_mapgen_function_json::update_map( const tripoint &, const point &, mission *, bool ) to its called function update_mapgen_function_json::update_map( mapgendata &, const point &, bool ), and removed the call to map::save. What map::save really does is only adding the submaps to the map buffer, which is already done in map::loadn, so the call to map::save was redundant. This means the behavior of update_mapgen_function_json::update_map( const tripoint &, const point &, mission *, bool ) has not been changed. On the other hand, update_mapgen_function_json::update_map( mapgendata &, const point &, bool ) now rotates the map passed to it, but it was previously only called in get_changed_ids_from_update, and rotating the map in that case is the desired behavior.
2. Added a new run_mapgen_update_func function that accepts mapgendata as its parameter. It has the same behavior as the old run_mapgen_update_func, except that it updates the map reference in mapgendata instead of a newly loaded tinymap.
3. Changed MapExtras::apply_function to call the new run_mapgen_update_func function instead, so the map extra is applied on top of the exact map MapExtras::apply_function is invoked with, instead of a newly loaded tinymap. This ensures that the new submaps constructed in map::generate is always visible to update_mapgen_function_json::update_map, so map::load doesn't need to be called and wrongly assume that the submaps still needs generation, thus avoiding the nested calls to map::generate and the memory leak.

Describe alternatives you've considered

The whole mapgen code needs some revision, but I'm not trying to delve too deep into that area now.

Testing

Tested in game and map generation was working as before. However I don't have LSAN in my environment, so I can't test locally whether the memory leak has been fixed. However my analysis strongly suggests this is the correct fix.