[RFC] glx: fix DRI2 memory leak

[Pierre Willenbrock]

Jesse Barnes schrieb:
> 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,

If the DestroyPixmap call frees the pixmap structure, i think it should
be moved after the __glxUnrefDrawable, so __glxUnrefDrawable does not
access an already freed pixmap structure.

Regards,
  Pierre