[PATCHv2 2/2] glx: Drop DestroyWindow hook

[Kristian Høgsberg]

2010/4/16 Michel Dänzer <michel at daenzer.net>:
> On Thu, 2010-04-15 at 12:13 -0400, Kristian Høgsberg wrote:
>> On Thu, 15 Apr 2010 15:48:00 +0200, Michel Dänzer <michel at daenzer.net> wrote:
>> > On Thu, 2010-04-15 at 08:26 -0400, Kristian Høgsberg wrote:
>> > > 2010/4/15 Kristian Høgsberg <krh at bitplanet.net>:
>> > > > 2010/4/15 Michel Dänzer <michel at daenzer.net>:
>> > > >> On Thu, 2010-04-15 at 07:58 -0400, Kristian Høgsberg wrote:
>> > > >>> 2010/4/15 Michel Dänzer <michel at daenzer.net>:
>> > > >>> > On Wed, 2010-04-14 at 15:13 -0400, Kristian Høgsberg wrote:
>> > > >>> >>
>> > > >>> >> The real fix is the patch from attachment 25038, not the DestroyWindow
>> > > >>> >> hook.  If the context is destroyed first, it will remove itself from
>> > > >>> >> the glxAllContexts list so the DrawableGone destructor won't touch it.
>> > > >>> >>  On the other hand, if the drawable is destroyed first, thanks to your
>> > > >>> >> patch, it will detach itself from the context properly so context
>> > > >>> >> destruction (whether at resource cleanup time or at client shutdown
>> > > >>> >> time) wont touch a free drawable.
>> > > >>> >
>> > > >>> > Right, but that will only work if DestroyWindow of the X window also
>> > > >>> > destroys the corresponding GLX drawable. Are you saying that's
>> > > >>> > guaranteed anyway now, and have you tested that none of MacSlow's
>> > > >>> > rgba-glx demos crashes the X server on exit anymore without this hook?
>> > > >>>
>> > > >>> Yes, the GLX drawable is a client allocated resource that gets cleaned
>> > > >>> up when the client exits.
>> > > >>
>> > > >> And if it doesn't? :) What's to stop a client from binding a window to a
>> > > >> GLX context, destroying the X window and then doing more stuff with the
>> > > >> GLX context?
>> > > >
>> > > > We handle that in DrawableGone.  You wrote that patch.
>> > >
>> > > Just to clarify, when a window is destroyed, all resources with the
>> > > same ID are destroyed.  So as long as there are no dependencies
>> > > between resource destructors for the different types of resources with
>> > > that ID, there should be no problem.
>> >
>> > It all sounds great in theory, but I still don't understand why the hook
>> > was necessary (or at least made a difference) when I added it, but that
>> > should no longer be the case now. :( Is it that the GLX drawable could
>> > have a different ID back then but no longer can now?
>>
>> I just ran master X server (that is, with the DestroyWindow hook in and
>> neither of the two patches in this series) and it crashes when I exit
>> rgba-glx by pressing escape.  Reading the patch and the resource code
>> again, I can't see that the hook ever did anything.  The way resources
>> are tracked, all resources with the same ID are in the same bucket in a
>> linked list.  Resources are prepended to the list and a window/pixmap is
>> always the first resource added for a given ID.  Destruction traverses
>> the list from head to tail, so the RT_WINDOW resource is the last to be
>> destroyed, and that's when the DestroyWindow hook is called.  At that
>> point there are no long any resources with the ID left in the bucket and
>> FreeResources(), as called from glxDestroyWindow(), doesn't do anything.
>
> It's starting to make sense. Thanks for bearing with me, my

Likewise, I'm glad we got to the bottom of this.

> Reviewed-by: Michel Dänzer <michel at daenzer.net>
>
> for this patch is well deserved. :)

Thanks.

>> The case you mentioned with using an GLX drawable after destroying the X
>> drawable is indeed a problem.  However, it only happens for GLX
>> drawables created using the glXCreateWindow/Pixmap functions. The
>> problem is that the DrawableGone hook doesn't get called for the GLX
>> drawable when the window is destroyed, since the GLX drawable has a
>> different ID. Neither my DRI2-drawables-as-resources or the hook fixes
>> this.  I confirmed the problem by adding a glXSwapBuffer() call to
>> rgba-glx after the XDestroyWindow() call and running it in indirect mode
>> (in direct rendering the DRI2 protocol code throws an BadDrawable error
>> before we get to dereference any freed drawables). The server crashes.
>> The patch below fixes the problem.
>
> Reviewed-by: Michel Dänzer <michel at daenzer.net>
>
> for this one as well.

Cool, resent the patch series with the patch from the thread with
Keith about how to clean up the pbuffer pixmap properly.

Kristian