On Thu, 15 Apr 2010 15:48:00 +0200, Michel Dänzer <[email protected]> wrote: > On Thu, 2010-04-15 at 08:26 -0400, Kristian Høgsberg wrote: > > 2010/4/15 Kristian Høgsberg <[email protected]>: > > > 2010/4/15 Michel Dänzer <[email protected]>: > > >> On Thu, 2010-04-15 at 07:58 -0400, Kristian Høgsberg wrote: > > >>> 2010/4/15 Michel Dänzer <[email protected]>: > > >>> > 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 From d599ec9b5452d550b96fe285fd67928e1806b8dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kristian=20H=C3=B8gsberg?= <[email protected]> Date: Thu, 15 Apr 2010 12:07:16 -0400 Subject: [PATCH] glx: Track GLX 1.3 style GLX drawables under their X drawable ID as well MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This ensures that the DrawableGone callback gets called as necessary when the X drawable goes away. Otherwise, using a GLX drawable (say, glXSwapBuffers) in indirect mode after the X drawable has been destroyed will crash the server. Signed-off-by: Kristian Høgsberg <[email protected]> --- glx/glxcmds.c | 12 ++++++++++++ glx/glxext.c | 11 +++++++++++ 2 files changed, 23 insertions(+), 0 deletions(-) diff --git a/glx/glxcmds.c b/glx/glxcmds.c index 77afbf4..04c6d40 100644 --- a/glx/glxcmds.c +++ b/glx/glxcmds.c @@ -161,7 +161,11 @@ validGlxDrawable(ClientPtr client, XID id, int type, int access_mode, return FALSE; } + /* If the ID of the glx drawable we looked up doesn't match the id + * we looked for, it's because we looked it up under the X + * drawable ID (see DoCreateGLXDrawable). */ if (rc == BadValue || + (*drawable)->drawId != id || (type != GLX_DRAWABLE_ANY && type != (*drawable)->type)) { client->errorValue = id; switch (type) { @@ -1128,6 +1132,14 @@ DoCreateGLXDrawable(ClientPtr client, __GLXscreen *pGlxScreen, __GLXconfig *conf return BadAlloc; } + /* Add the glx drawable under the XID of the underlying X drawable + * too. That way we'll get a callback in DrawableGone and can + * clean up properly when the drawable is destroyed. */ + if (!AddResource(pDraw->id, __glXDrawableRes, pGlxDraw)) { + pGlxDraw->destroy (pGlxDraw); + return BadAlloc; + } + return Success; } diff --git a/glx/glxext.c b/glx/glxext.c index 59bcfbe..89e58b0 100644 --- a/glx/glxext.c +++ b/glx/glxext.c @@ -126,6 +126,17 @@ static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid) { __GLXcontext *c; + /* If this drawable was created using glx 1.3 drawable + * constructors, we added it as a glx drawable resource under both + * its glx drawable ID and it X drawable ID. Remove the other + * resource now so we don't a callback for freed memory. */ + if (glxPriv->drawId != glxPriv->pDraw->id) { + if (xid == glxPriv->drawId) + FreeResourceByType(glxPriv->pDraw->id, __glXDrawableRes, TRUE); + else + FreeResourceByType(glxPriv->drawId, __glXDrawableRes, TRUE); + } + for (c = glxAllContexts; c; c = c->next) { if (c->isCurrent && (c->drawPriv == glxPriv || c->readPriv == glxPriv)) { int i; -- 1.7.0.1 _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
