2010/9/23 Kristian Høgsberg <k...@bitplanet.net>: > 2010/9/23 Jeremy Huddleston <jerem...@apple.com>: >> That seems off to me. This is doing more than changing the c->next >> dereference. You're now freeing it where you weren't before. >> >> Previously, you freed it inside: >> if (c->isCurrent && (c->drawPriv == glxPriv || c->readPriv == glxPriv)) >> if(!c->idExists) >> >> Now, you free it inside: >> if (!c->idExists && !c->isCurrent) >> >> So you're missing the check for (c->drawPriv == glxPriv || c->readPriv == >> glxPriv) ... is that intentional and we had a leak before, or are we now >> over-releasing? > > All the contexts on the context list have either idExists or isCurrent > or both true, otherwise we would have already destroyed them. If a > context that was current is destroyed, we set idExists to NULL (it no > longer has an XID) but keep it around while it's still current. If > the context is not current, we just destroy it right away and take it > out of the list. > > The loop above, iterates through the list of all contexts to see if > there is a context that has been destroyed but is still current with > the drawable that we're getting the DrawableGone() callback for. We > treat that as an unbind, which triggers the context destruction. As > Jon mentions, that may not be the right thing to do, but in the scope > of this patch, we're just delaying the destroy until we're done > touching the context.
Jeremy, does the above explanation satisfy your concerns? Keith, do you want to pick this up for master? >> On Sep 23, 2010, at 06:04, Kristian Høgsberg wrote: >> >>> Signed-off-by: Kristian Høgsberg <k...@bitplanet.net> >>> --- >>> >>> Chris Wilson points out that we were still accessing c->next after free. >>> Here's an updated version that fixes that. >>> >>> Kristian >>> >>> glx/glxext.c | 11 +++++------ >>> 1 files changed, 5 insertions(+), 6 deletions(-) >>> >>> diff --git a/glx/glxext.c b/glx/glxext.c >>> index e203156..f5ebe4f 100644 >>> --- a/glx/glxext.c >>> +++ b/glx/glxext.c >>> @@ -124,7 +124,7 @@ static int glxBlockClients; >>> */ >>> static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid) >>> { >>> - __GLXcontext *c; >>> + __GLXcontext *c, *next; >>> >>> /* If this drawable was created using glx 1.3 drawable >>> * constructors, we added it as a glx drawable resource under both >>> @@ -137,7 +137,8 @@ static Bool DrawableGone(__GLXdrawable *glxPriv, XID >>> xid) >>> FreeResourceByType(glxPriv->drawId, __glXDrawableRes, TRUE); >>> } >>> >>> - for (c = glxAllContexts; c; c = c->next) { >>> + for (c = glxAllContexts; c; c = next) { >>> + next = c->next; >>> if (c->isCurrent && (c->drawPriv == glxPriv || c->readPriv == >>> glxPriv)) { >>> int i; >>> >>> @@ -160,15 +161,13 @@ static Bool DrawableGone(__GLXdrawable *glxPriv, XID >>> xid) >>> } >>> } >>> } >>> - >>> - if (!c->idExists) { >>> - __glXFreeContext(c); >>> - } >>> } >>> if (c->drawPriv == glxPriv) >>> c->drawPriv = NULL; >>> if (c->readPriv == glxPriv) >>> c->readPriv = NULL; >>> + if (!c->idExists && !c->isCurrent) >>> + __glXFreeContext(c); >>> } >>> >>> glxPriv->destroy(glxPriv); >>> -- >>> 1.7.3 >>> >>> _______________________________________________ >>> xorg-devel@lists.x.org: X.Org development >>> Archives: http://lists.x.org/archives/xorg-devel >>> Info: http://lists.x.org/mailman/listinfo/xorg-devel >> >> > _______________________________________________ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel