Re: [sdljava-users] Possible memory leak in SWIG_SDLVideo / SDL_Surface / SWIG_SDLImage

SourceForge Headquarters 1320 Columbia Street Suite 310 San Diego, CA 92101 +1 (858) 422-6466

Greetings,

I've implemented the fixes as suggested and the code is checked into CVS.

Before I checked in the changes I tagged the source as 
"PRE_MEMORY_LEAK_FIX" and afterwards I tagged as "POST_MEMORY_LEAK_FIXES".

Please let me know if there are any problems.

Cheers,
-Ivan/

Patric Rufflar wrote:

>Hi Ivan,
>
>You're welcome.
>I'm glad that I could help improving sdljava. :)
>
>By the way: SWIG_SDLTTF and SWIG_SDLGfx contain faulty constructors of 
>SDL_Surfaces, too. Please set false to true to fix this.
>
>Now I cannot see any other memory leaks (for now ;-) )
>
>And for now, I can't await that other points of my wishlist will come true :)
>- new sdljava release :)
>- new sdljava release for windows (oh, please. But maybe I have some free time 
>in the future and have a look how to set up a suitable build environment)
>- Support ffmpeg
>- full YUV Support (or do we have a complete and tested support already?)
>- Investigation of some rare core dumps. For example, SDLVideo.videoDriverName() 
>always crashes on my machine (windows, directx).
>- Support SDL_sound library
>
>Patric
>
>
>
>Ivan Z. Ganza schrieb:
>  
>
>>Hi Patric,
>>
>>I didn't even have a chance to look at it properly and you've already 
>>solved it! 
>>
>>Thanks so much for the detailed look you had at this.  Will go over it, 
>>it looks good.  The only problem is we have to change the way SWIG 
>>auto-generates the code.  For now I'll just patch the file as you've 
>>shown.  Eventually I'll need to figure out how to get SWIG to generate 
>>the wrapper code with that change in place.  We don't really generate 
>>the wrapper code often so it should be alright.  The only time would be 
>>when making the wrappers for a new OS or if it was required for some 
>>other reason like a new SDL version, etc....
>>
>>Will let you know when its in.
>>
>>Cheers,
>>-Ivan/
>>
>>Patric Rufflar wrote:
>>
>>    
>>
>>>Hi Ivan, hi all,
>>>
>>>After playing around with the patched source I realized that
>>>there is another critical memory problem.
>>>
>>>At first, please have a look at the jni definition inside SWIG_SDLVideoJNI
>>>
>>>public final static native void SDL_FreeSurface(long jarg1);
>>>public final static native void delete_SDL_Surface(long jarg1);
>>>public final static native long new_SDL_Surface();
>>>
>>>
>>>The first method, SDL_FreeSurface(), is ok. It does what it name 
>>>suggests, it calls the corresponding native SDL method.
>>>
>>>The other two methods seems to me dummy native methods for 
>>>contructors/destructors.  The original SDL_surface.c does not contain 
>>>any constructors/destructors, because it's pure c.
>>>So I think these jni calls do nothing.
>>>
>>>
>>>Ok, let's now have a look at SDLSurface.freeSurface() which will be 
>>>called on its finalization.
>>>
>>>    public void freeSurface() throws SDLException {
>>>	SWIG_SDLVideo.SDL_FreeSurface(swigSurface);
>>>	swigSurface = null;
>>>	pixelData   = null;
>>>    }
>>>
>>>As we can see, the real surface of the embedded SDL_Surface proxy object 
>>>will be freed. But what happens if there are other valid references to 
>>>this proxy object? It's memory pointer to the sdl surface is not valid 
>>>anymore, freeing this object may end up in a core dump.
>>>
>>>Now let's have a look at SDL_Surface finalization method, delete():
>>>
>>>  public void delete() {
>>>    if(swigCPtr != 0 && swigCMemOwn) {
>>>      swigCMemOwn = false;
>>>      SWIG_SDLVideoJNI.delete_SDL_Surface(swigCPtr);
>>>    }
>>>    swigCPtr = 0;
>>>  }
>>>
>>>This method calls the jni delete method, which is only a dummy. The real 
>>>memory is not freed. This is the memory leak.
>>>
>>>Because some SDL_Surface objects seem to be created without referenced 
>>>by a SDLSurface. But I have to admit that I cannot give an example in 
>>>the source right now.
>>>
>>>To fix these two problems, I suggest the following solution:
>>>
>>>Changes at SDLSurface:
>>>    public void freeSurface() throws SDLException {
>>>	SWIG_SDLVideo.SDL_FreeSurface(swigSurface); <<--delete this line
>>>	swigSurface = null;
>>>	pixelData   = null;
>>>    }
>>>
>>>
>>>Changes at SDL_Surface:
>>>  public void delete() {
>>>    if(swigCPtr != 0 && swigCMemOwn) {
>>>      SWIG_SDLVideo.SDL_FreeSurface(this); // As you can see, 		 
>>>//delete_SDL_Surface() has been replaced by the correct method.
>>>      swigCMemOwn = false;
>>>    }
>>>    swigCPtr = 0;
>>>  }
>>>
>>>
>>>This patch will ensure, that the finalization of the proxy object 
>>>(SDLSurface) will lead to freeing the surface not it's sdljava interface 
>>>to it (SDL_Surface). This fixed the memory problems in my application ;)
>>>
>>>To find those problems I am creating / deleting many surfaces in my 
>>>application. And It seems that there is at least another (less lossy) 
>>>memory leak which eats up memory.
>>>
>>>And I also want to add that the methods of SWIG_SDLImage seems to be 
>>>affected by the memory leak which I previously described, too.
>>>
>>>
>>>Best Regards
>>>Patric Rufflar
>>>
>>>
>>>
>>>Ivan Z. Ganza schrieb:
>>> 
>>>
>>>      
>>>
>>>>Greetings,
>>>>
>>>>Thanks for finding this.  Yes this is a serious problem.
>>>>
>>>>I need to do figure out how to get SWIG to set that flag to true instead 
>>>>of false, then the leak should be eleminated.  The code there is 
>>>>auto-generated by SWIG from the .i files.  In the worst case I can 
>>>>manually run a script after to post-process that stuff and fix it that 
>>>>way.  Either way it will be fixed.
>>>>
>>>>Cheers,
>>>>-Ivan/
>>>>
>>>>Jesper Juul / Soup Games wrote:
>>>>
>>>>   
>>>>
>>>>        
>>>>
>>>>>I wanted to post about this for a long time - I agre there is a leak 
>>>>>. So far I've worked around it by wrapping all Surfaces in a custom 
>>>>>object that calls dispose() in the finalizer.
>>>>>
>>>>>
>>>>>-Jesper
>>>>>
>>>>>
>>>>>
>>>>>     
>>>>>
>>>>>          
>>>>>
>>>>>>I was wondering a long time why my application needs more and more RAM.
>>>>>>I took a while to track down a possible memory leak.
>>>>>>It seems to be located in sdljava.x.swig.SWIG_SDLVideo .
>>>>>>
>>>>>>The following methods seems to be affected:
>>>>>>SDL_ConvertSurface(), SDL_DisplayFormat() and SDL_DisplayFormatAlpha()
>>>>>>
>>>>>>I am not sure at the moment, but maybe other methods have the same problem:
>>>>>>SDL_SetVideoMode(), SDL_CreateRGBSurface(), SWIG_SDL_LoadBMP(),
>>>>>>SWIG_SDL_CreateRGBSurfaceFrom()
>>>>>>
>>>>>>Please have a look at the last line of all methods:
>>>>>>return (cPtr == 0) ? null : new SDL_Surface(cPtr, false);
>>>>>>
>>>>>>As you can easily see, false indicates that the new surface cannot free
>>>>>>the memory because some other surface seems to also use it. But this is
>>>>>>not true, the surface has just been created, no other surface does own
>>>>>>this memory area.
>>>>>>
>>>>>>So this bug should be fixed when setting the second argument of the
>>>>>>surface constructors to true.
>>>>>>
>>>>>>Ivan, would you please verify the problem?
>>>>>>Thank you.
>>>>>>
>>>>>>Best Regards,
>>>>>>Patric
>>>>>>  
>>>>>>
>>>>>>       
>>>>>>
>>>>>>            
>>>>>>
>>>>>-------------------------------------------------------------------------
>>>>>Using Tomcat but need to do more? Need to support web services, security?
>>>>>Get stuff done quickly with pre-integrated technology to make your job easier
>>>>>Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
>>>>>http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
>>>>>_______________________________________________
>>>>>sdljava-users mailing list
>>>>>sdl...@li...
>>>>>https://lists.sourceforge.net/lists/listinfo/sdljava-users
>>>>>
>>>>>
>>>>>     
>>>>>
>>>>>          
>>>>>
>>>>-------------------------------------------------------------------------
>>>>Using Tomcat but need to do more? Need to support web services, security?
>>>>Get stuff done quickly with pre-integrated technology to make your job easier
>>>>Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
>>>>http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
>>>>_______________________________________________
>>>>sdljava-users mailing list
>>>>sdl...@li...
>>>>https://lists.sourceforge.net/lists/listinfo/sdljava-users
>>>>
>>>>
>>>>   
>>>>
>>>>        
>>>>
>>>-------------------------------------------------------------------------
>>>Take Surveys. Earn Cash. Influence the Future of IT
>>>Join SourceForge.net's Techsay panel and you'll get the chance to share your
>>>opinions on IT & business topics through brief surveys -- and earn cash
>>>http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
>>>_______________________________________________
>>>sdljava-users mailing list
>>>sdl...@li...
>>>https://lists.sourceforge.net/lists/listinfo/sdljava-users
>>> 
>>>
>>>      
>>>
>>-------------------------------------------------------------------------
>>Take Surveys. Earn Cash. Influence the Future of IT
>>Join SourceForge.net's Techsay panel and you'll get the chance to share your
>>opinions on IT & business topics through brief surveys -- and earn cash
>>http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
>>_______________________________________________
>>sdljava-users mailing list
>>sdl...@li...
>>https://lists.sourceforge.net/lists/listinfo/sdljava-users
>>
>>
>>    
>>
>
>-------------------------------------------------------------------------
>Take Surveys. Earn Cash. Influence the Future of IT
>Join SourceForge.net's Techsay panel and you'll get the chance to share your
>opinions on IT & business topics through brief surveys -- and earn cash
>http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
>_______________________________________________
>sdljava-users mailing list
>sdl...@li...
>https://lists.sourceforge.net/lists/listinfo/sdljava-users
>  
>