Revision: 326 http://trac.macosforge.org/projects/xquartz/changeset/326 Author: gstaplin@apple.com Date: 2009-03-27 12:02:55 -0700 (Fri, 27 Mar 2009) Log Message: ----------- Fix the surface destruction issue more cleanly with a slightly better design. The test case still passes, and this should prevent a memory leak pattern that the previous fix would have suffered from. Tested with a few apps, and triangle_glx_destroy_relation (the specific test case for this). Modified Paths: -------------- AppleSGLX/trunk/apple_glx_context.c AppleSGLX/trunk/apple_glx_surface.c Modified: AppleSGLX/trunk/apple_glx_context.c =================================================================== --- AppleSGLX/trunk/apple_glx_context.c 2009-03-26 07:53:24 UTC (rev 325) +++ AppleSGLX/trunk/apple_glx_context.c 2009-03-27 19:02:55 UTC (rev 326) @@ -572,14 +572,7 @@ * If there are references to it, then it's probably made * current in another context. */ - d->destroy(d); - - /* - * FIXME: We retain 2 references to surfaces to prevent - * premature destruction. See also: the comment in the - * make current path. - */ - d->destroy(d); + d->destroy(d); } } } Modified: AppleSGLX/trunk/apple_glx_surface.c =================================================================== --- AppleSGLX/trunk/apple_glx_surface.c 2009-03-26 07:53:24 UTC (rev 325) +++ AppleSGLX/trunk/apple_glx_surface.c 2009-03-27 19:02:55 UTC (rev 326) @@ -101,16 +101,23 @@ fprintf(stderr, "xp_destroy_surface error: %d\n", (int)error); } - /* - * Warning: this causes other routines to be called (potentially) - * from surface_notify_handler. It's probably best to not have - * any locks at this point locked. + /* + * Check if this surface destroy came from the surface being destroyed + * on the server. If s->pending_destroy is true, then it did, and + * we don't want to try to destroy the surface on the server. */ - XAppleDRIDestroySurface(d->display, DefaultScreen(d->display), - d->drawable); - - apple_glx_diagnostic("%s: destroyed a surface for drawable 0x%lx uid %u\n", - __func__, d->drawable, s->uid); + if(!s->pending_destroy) { + /* + * Warning: this causes other routines to be called (potentially) + * from surface_notify_handler. It's probably best to not have + * any locks at this point locked. + */ + XAppleDRIDestroySurface(d->display, DefaultScreen(d->display), + d->drawable); + + apple_glx_diagnostic("%s: destroyed a surface for drawable 0x%lx uid %u\n", + __func__, d->drawable, s->uid); + } } /* Return true if an error occured. */ @@ -193,6 +200,18 @@ if(d) { d->types.surface.pending_destroy = true; d->release(d); + /* + * We release 2 references to the surface. One was acquired by + * the find, and the other was leftover from a context, or + * the surface being displayed, so the destroy() will decrease it + * once more. + * + * If the surface is in a context, it will take one d->destroy(d); + * to actually destroy it when the pending_destroy is processed + * by a glViewport callback (see apple_glx_context_update()). + */ + d->destroy(d); + d->unlock(d); } }