[Pingus-Devel] Memory leak in Pingus 0.7.2 (developer question)

[Hans Jorgensen]

Hi,
I have tried to track down the memory leaks in Pingus, using valgrind and 
recompile code -g -O0. There are several memory leaks, but for now i will 
focus on the biggest of them.

SDL_FreeSurface() is not always called. I see two solutions to this, and do 
not know who can take the decision about which to implement, or if others 
have a better solution.

1. Track down all the places failing to call SDL_FreeSurface() and then fix it
     I think the two mentionend below are the only ones, but there might be
     others.

2. Modify SurfaceImpl to remove the delete_surface parameter, and always
    call SDL_FreeSurface(), last pointer to SurfaceImpl is removed. I did a
    search in the code and could not find other references to
    SDL_FreeSurface() meaning that the code always expects this to happen when
    the last pointer is removed.


FYI here is a description of why SDL_FreeSurface() is not always called:
If an object of class Surface is created it also creates an protected class 
SurfaceImpl using a boost::shared_ptr<SurfaceImpl> pointer.

SurfaceImpl contains an pointer to SDL_Surface. When the last pointer to 
SurfaceImpl is removed and delete_surface == true (set during construction of 
SurfaceImpl) then SDL_FreeSurface() is called.

This means that:
Surface::Surface(int width, int height)
  : impl(new SurfaceImpl())

Would require that SDL_FreeSurface() is called frome somewhere outside 
SurfaceImpl because SurfaceImpl::SurfaceImpl() sets delete_surface = false as 
default.

Unfortunately class MapTile (ground_map.cpp:L36) does not call 
SDL_FreeSurface() , and therfore creates a memory leak. This can easily be 
fixed in the destructor of 
MapTile.

But there is another problem.
Surface::mod(ResourceModifierNS::ResourceModifier modifier)
does some interesting things depending on the variable modifier
ResourceModifierNS::ROT0 creates a clone of itself with delete_surface = true
ResourceModifierNS::ROTEverythingElse uses Blitter::rotateXXXX which ends up 
with delete_surface = false.

This strange behaveure makes the class CollisionMask fail to free the 
SDL_Surface if Resource::load_surface rotates the resource, but working fine 
if it is not rotated. Actually there is another twist because   
Resource::load_surface does not call Surface::mod in case of 
ResourceModifierNS::ROT0 but the result is the same.

I hope someone can take this decision, and then i will try to create a patch 
to solve this.
Hans