Revision

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

Diff

--- 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);	    
  	}
     }
 }

--- 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);
     }
 }