[PATCHv2 2/2] glx: Drop DestroyWindow hook

[Kristian Høgsberg]

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.

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.

Kristian