[RFC] glx: fix DRI2 memory leak

[Jesse Barnes]

On Thu, 26 Mar 2009 22:02:52 +0200
Vasily Khoruzhick <anarsoul at gmail.com> wrote:

> On Wednesday 25 March 2009 19:21:51 Jesse Barnes wrote:
> 
> > Has anyone else seen this leak?  Anyone care to educate me a bit
> > more about GLX drawable lifetime rules?
> 
> Yep, my system suffer from this leak too.

Yeah sounds pretty widespread; it'll only get worse as the radeon DRI2
bits become more common.

> > diff --git a/glx/glxext.c b/glx/glxext.c
> > index c882372..73e5a9b 100644
> > --- a/glx/glxext.c
> > +++ b/glx/glxext.c
> > @@ -127,9 +127,9 @@ static Bool DrawableGone(__GLXdrawable
> > *glxPriv, XID xid) break;
> >      }
> >
> > -    glxPriv->pDraw = NULL;
> >      glxPriv->drawId = 0;
> >      __glXUnrefDrawable(glxPriv);
> 
> Maybe, we should somehow check if glxPriv is still valid after Unref
> before assigning something to the pDraw? (I'm not sure, I'm not
> familiar with X internals)

Yeah, I'm just learning this stuff too...  Now that I've walked through
the life of a GLX pixmap a bit things look even more convoluted.  GLX
pixmaps are created by __glXDRIscreenCreateDrawable at various times,
either explicitly by something like __glXGetDrawable or implicitly by a
glXCreateWindow call.  All of these end up in DRI2CreateDrawable which
either bumps the refcount of an existing pixmap or creates a new one
(though with no buffers attached).

The drawables get destroyed by their own destroy hook, which points at
__glXDRIdrawableDestroy (yes this and the one above should probably
have been called glXDRI2...), which is either explicitly called by a
cleanup routine (error handling in drawable creation for example), free
context time, free screen time, or when a drawable is deref'd and the
count drops to 0.

What's weird is that __glXUnrefDrawable might be called by DrawableGone
(the top level X managed resource for glx drawables) due to
__glXUnrefDrawable calling FreeResourceByType, but then DrawableGone
will again call __glXUnrefDrawable... I think krh noticed this today
and suggested we check the refcount in DrawableGone... But it seems
like since __glXUnrefDrawable only frees the resource if the refcount
reaches 0, we shouldn't need another unref in DrawableGone...  So maybe
that's what my fix was running into.

Anyway back to more testing & reading.

-- 
Jesse Barnes, Intel Open Source Technology Center