[RFC] glx: fix DRI2 memory leak

[Jesse Barnes]

On Thu, 26 Mar 2009 14:20:07 -0700
Jesse Barnes <jbarnes at virtuousgeek.org> wrote:
> 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.

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.

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center


diff --git a/glx/glxcmds.c b/glx/glxcmds.c
index ab2d91b..08b866d 100644
--- a/glx/glxcmds.c
+++ b/glx/glxcmds.c
@@ -1222,7 +1222,7 @@ static int DoDestroyDrawable(__GLXclientState *cl, XID glxdrawable, int type)
 	}
     }
 
-    FreeResource(glxdrawable, FALSE);
+    __glXUnrefDrawable(pGlxDraw);
 
     return Success;
 }
diff --git a/glx/glxdri.c b/glx/glxdri.c
index 223b06e..253d868 100644
--- a/glx/glxdri.c
+++ b/glx/glxdri.c
@@ -237,8 +237,6 @@ __glXDRIdrawableDestroy(__GLXdrawable *drawable)
 			   serverClient, drawable->pDraw);
 	__glXleaveServer(GL_FALSE);
     }
-
-    xfree(private);
 }
 
 static GLboolean
diff --git a/glx/glxdri2.c b/glx/glxdri2.c
index 4e76c71..4fdc7df 100644
--- a/glx/glxdri2.c
+++ b/glx/glxdri2.c
@@ -106,8 +106,6 @@ __glXDRIdrawableDestroy(__GLXdrawable *drawable)
      * aready have taken care of this, so only call if pDraw isn't NULL. */
     if (drawable->pDraw != NULL)
 	DRI2DestroyDrawable(drawable->pDraw);
-
-    xfree(private);
 }
 
 static void
diff --git a/glx/glxext.c b/glx/glxext.c
index c882372..0b6c752 100644
--- a/glx/glxext.c
+++ b/glx/glxext.c
@@ -120,6 +120,8 @@ static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid)
 {
     ScreenPtr pScreen = glxPriv->pDraw->pScreen;
 
+    glxPriv->destroy(glxPriv);
+
     switch (glxPriv->type) {
 	case GLX_DRAWABLE_PIXMAP:
 	case GLX_DRAWABLE_PBUFFER:
@@ -129,7 +131,7 @@ static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid)
 
     glxPriv->pDraw = NULL;
     glxPriv->drawId = 0;
-    __glXUnrefDrawable(glxPriv);
+    xfree(glxPriv);
 
     return True;
 }
diff --git a/glx/glxutil.c b/glx/glxutil.c
index bc71087..61323f5 100644
--- a/glx/glxutil.c
+++ b/glx/glxutil.c
@@ -52,11 +52,9 @@ void
 __glXUnrefDrawable(__GLXdrawable *glxPriv)
 {
     glxPriv->refCount--;
-    if (glxPriv->refCount == 0) {
+    if (glxPriv->refCount == 0)
 	/* remove the drawable from the drawable list */
 	FreeResourceByType(glxPriv->drawId, __glXDrawableRes, FALSE);
-	glxPriv->destroy(glxPriv);
-    }
 }
 
 GLboolean