Re: memory leak - cairo/x11 backend

[Fred Kiefer]

On 18.12.2013 18:45, Eric Wasylishen wrote:
> Hi Riccardo, +Fred +gnustep-dev
> 
> On Dec 18, 2013, at 7:40 AM, Riccardo Mottola
> <address@hidden> wrote:
> 
>> Hi,
>> 
>> Eric Wasylishen wrote:
>>> 
>>> Hi Riccardo,
>>> 
>>> As a starting point for debugging I would put NSLog's in
>>> XGCairoModernSurface -initWithDevice: and -dealloc. This is where
>>> we retain / release the Cairo surface that is holding memory in
>>> the x server.
>>> 
>>> Also I would put logging in -NSImage set name:, log [nameDict
>>> allKeys] to see which images are kept in memory by nameDict.
>>> 
>> I did so, in setName.. It does not get called from laternaMagica
>> when switching images. WHile the application loads and shows its
>> windows I see stuff like:
>> 
>> 2013-12-18 15:36:39.356 LaternaMagica[18906] NSimage setName, keys:
>> ("common_ret H", "common_Nibble", "common_HomeDirectory",
>> NSApplicationIcon, "common_UnknownT ool", GSMenuArrow,
>> "common_Mount", "common_DownloadFolder", "common_ArrowDown",
>> "laternamagica_48.tif", "common_Home", "common_ArrowLeftH",
>> "common_Close", "com mon_3DArrowLeft", "common_Desktop",
>> "common_ImageFolder", GSMenuSelected, "commo n_3DArrowRightH",
>> GSSwitch, GSSwitchSelected, "common_GSFolder", "common_Dimple" ,
>> "common_Unmount", "common_DimpleHoriz", "common_ArrowRightH",
>> "common_DocsFold er", "common_Unknown", "common_ArrowUpH",
>> GSMenuMixed, NSFolder, "common_ArrowUp ", "common_3DArrowUp",
>> "common_ArrowRight", "common_3DArrowDown", "common_ArrowD ownH",
>> "common_ret", "common_ArrowLeft", "common_SliderHoriz")
>> 
>> 
>> however when I load images from disk and cycle between them,
>> nothing gets printed, meaning that setName doesn't get called.
>> 
>> Riccardo
> 
> Actually, I was on the wrong track; I just found the problem.
> 
> During the creation of a window, we run -[XGServerWindow
> setWindowdevice:forContext:]. The problem is, inside that method we
> call [self _createBuffer] which does window->buffer =
> XCreatePixmap(…), but the [self _createBuffer] call is *before*
> window->gdriverProtocol is initialized. window->gdriverProtocol is
> set from the -init… method of XGCairoModernSurface.
> 
> The call sequence that causes the leak looks like:
> 
> -[XGServerWindow setWindowdevice:forContext:] -[XGServerWindow
> _createBuffer] (performs window->buffer = XCreatePixmap because
> window->gdriverProtocol == 0) GSSetDevice() -[XGCairoModernSurface
> initWIthDevice:] (sets window->gdriverProtocol to
> GDriverHandlesExpose | GDriverHandlesBacking) …later…. 
> -[XGServerWindow termwindow] doesn’t free window->buffer because
> window->gdriverProtocol has the GDriverHandlesBacking bit set.
> 
> 
> 
> As a quick fix, I reordered this section of  -[XGServerWindow
> setWindowdevice:forContext:]
> 
> if (window->buffer == 0) { [self _createBuffer: window]; }
> 
> [self styleoffsets: &l : &r : &t : &b :
> window->win_attrs.window_style : window->ident]; GSSetDevice(ctxt,
> window, l, NSHeight(window->xframe) + b);
> 
> to:
> 
> [self styleoffsets: &l : &r : &t : &b :
> window->win_attrs.window_style : window->ident]; GSSetDevice(ctxt,
> window, l, NSHeight(window->xframe) + b); if (window->buffer == 0) { 
> [self _createBuffer: window]; }
> 
> 
> which fixed the leak, however it causes apps to segfault with the
> xlib backend. Probably some code in -styleoffsets or GSSetDevice
> attempts to use window->buffer.
> 
> I just committed a more foolproof fix, which is just disabling all of
> the code in -_createBuffer if we are building with the cairo backend.
> As I noted in the comment there, I don’t like the
> window->gdriverProtocol abstraction as it’s trying to handle
> different library configurations in “too dynamic” of a way.
> 
> Testing is welcome; the leak is gone for me and LaternaMagica seems a
> lot faster too.
> 
> Cheers, Eric

Excellent, you found the source of the issue and fixed it. I agree that
the fix doesn't look very nice. I will think hard to come up with a
cleaner way to achieve the same.

Thank you and also a thank you to Riccardo for noticing this issue,

Fred