[RFC] glx: fix DRI2 memory leak

[Kristian Høgsberg]

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;
    }

Did you try that?

cheers,
Kristian


> 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
>