[RFC] glx: fix DRI2 memory leak

[Magnus Kessler]

On Monday 30 March 2009, Shuang He wrote:
> Jesse Barnes wrote:
> > On Fri, 27 Mar 2009 13:20:25 -0400
> >
> > Kristian Høgsberg <krh at bitplanet.net> wrote:
> >> On Thu, Mar 26, 2009 at 6:33 PM, Jesse Barnes
> >>
> >> <jbarnes at virtuousgeek.org> wrote:
> >>> Ok, I think this is where the leak was:
> >>> __glXUnrefDrawable->DrawableGone->__glXUnrefDrawable.  This sequence
> >>> meant the glxPriv would stay around (since it was used), but
> >>> DrawableGone had generally already disposed of the pDrawable before
> >>> the call to Unref, so we never got into DRI2DestroyBuffers, thus
> >>> the leak.
> >>>
> >>> The loop seems broken to me.  It *looks* like DrawableGone should be
> >>> the real free routine, only called when a drawable really is free,
> >>> so I've fixed up the code to conform to that.  This means removing
> >>> the GLX priv frees from DRI and DRI2 routines and putting them here
> >>> and using the GLX unref routine everwhere (since it will only free
> >>> the resource when the refcount reaches 0).
> >>>
> >>> Any thoughts?  I'd appreciate some more testers too...  I'm not
> >>> quite sure about the generic DoDestroyDrawable change, but if the
> >>> refcount is always 1 there anyway it shouldn't make a difference
> >>> and seems more correct.
> >>
> >> The __GLXdrawable is a reference counted object, and the glx code
> >> references it in a couple of places: when there's an X resource
> >> pointing to it (a GLXWindow, GLXPixmap or GLXPbuffer) and when it's
> >> the current drawable or readable for a context.  The __GLXdrawable is
> >> allocated by the backend in use (dri, dri2 or swrast) and must be
> >> freed by the same backend (don't mix alloc and free across abstraction
> >> barriers).  We unref the __GLXdrawable when we either set a different
> >> drawable/readable and when the X resource goes away.
> >>
> >> The leak is (as you pointed out before) because we NULL the pDraw
> >> pointer before calling the backend destructor, which then can't unref
> >> the DRI2Drawable, which we then leak.  You had the right fix in the
> >> originial post, we just need to not touch glxPriv after
> >> __glXUnrefDrawable if it got destroyed.  I suggest this change to
> >> DrawableGone in irc:
> >>
> >>     refcount = glxPriv->refcount;
> >>     __glXUnrefDrawable(glxPriv);
> >>     if (refcount > 1) {
> >>         glxPriv->pDraw = NULL;
> >>         glxPriv->drawId = 0;
> >>     }
> >
> > Yep, that seems to work too...  Magnus or Vasily can you guys confirm?
> >
> > Thanks,
>
> Memory leak problem seems resolved, but I still see X crash with:
>
> (gdb) bt
> #0  0xb7fd5424 in __kernel_vsyscall ()
> #1  0x03155660 in raise () from /lib/libc.so.6
> #2  0x03157028 in abort () from /lib/libc.so.6
> #3  0x031925bd in __libc_message () from /lib/libc.so.6
> #4  0x031987e4 in malloc_printerr () from /lib/libc.so.6
> #5  0x0319c441 in _int_realloc () from /lib/libc.so.6
> #6  0x0319d176 in realloc () from /lib/libc.so.6
> #7  0x08131002 in Xrealloc (ptr=0x6, amount=0) at utils.c:1133
> #8  0x0806d10b in dixAllocatePrivate (privates=0x91487c8, key=0xb7e90a3c)
>     at privates.c:129
> #9  0x0806d1cc in dixSetPrivate (privates=0x91487c8, key=0xb7e90a3c,
> val=0x0) at privates.c:193
> #10 0xb7e8eca1 in DRI2DestroyDrawable (pDraw=0x91487b0) at dri2.c:218
> #11 0xb7eee668 in __glXDRIdrawableDestroy (drawable=0x9205ff0) at
> glxdri2.c:108 #12 0xb7ee49bb in __glXUnrefDrawable (glxPriv=0x9205ff0) at
> glxutil.c:58 #13 0xb7ee3183 in DrawableGone (glxPriv=0x9205ff0,
> xid=12583326) at glxext.c:131
> #14 0x0806efdc in FreeResource (id=12583326, skipDeleteFuncType=0)
>     at resource.c:561
> #15 0xb7edffa6 in DoDestroyDrawable (cl=<value optimized out>,
>     glxdrawable=12583326, type=1) at glxcmds.c:1225
> #16 0xb7ee340a in __glXDispatch (client=0x8d79db8) at glxext.c:523
> #17 0x080874cf in Dispatch () at dispatch.c:437
> ---Type <return> to continue, or q <return> to quit---
> #18 0x0806c69d in main (argc=2, argv=0xbf9d2754, envp=Cannot access
> memory at address 0xbde
>
> Thanks
> 	--Shuang
> ) at main.c:397

I have observed this crash once as well, but haven't yet found a way to 
reproduce it. Can you post a few steps that make this crash more likely? On 
the bug report (http://bugs.freedesktop.org/show_bug.cgi?id=20704) you 
mention it has something to do with resizing by small amounts.

Thanks

Magnus