[RFC] glx: fix DRI2 memory leak

[Jesse Barnes]

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,
-- 
Jesse Barnes, Intel Open Source Technology Center

diff --git a/glx/glxext.c b/glx/glxext.c
index c882372..fec45b7 100644
--- a/glx/glxext.c
+++ b/glx/glxext.c
@@ -119,6 +119,7 @@ static int ContextGone(__GLXcontext* cx, XID id)
 static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid)
 {
     ScreenPtr pScreen = glxPriv->pDraw->pScreen;
+    int refcount;
 
     switch (glxPriv->type) {
 	case GLX_DRAWABLE_PIXMAP:
@@ -127,9 +128,12 @@ static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid)
 	    break;
     }
 
-    glxPriv->pDraw = NULL;
-    glxPriv->drawId = 0;
+    refcount = glxPriv->refCount;
     __glXUnrefDrawable(glxPriv);
+    if (refcount > 1) {
+	glxPriv->pDraw = NULL;
+	glxPriv->drawId = 0;
+    }
 
     return True;
 }