XQuartz: Fix a memory leak with surfaces that a new test found.

xp_destroy_surface was called with a surface id of 0, due to some
premature cleanup that set it to 0.  This means the surfaces
weren't being destroyed until the window was.

The code that did that was: pDRIDrawablePriv->sid = 0;

In long running applications this leak may or may not have been
harmful.  With the old libGL the surfaces weren't destroyed until
the context was destroyed or a new context created.  In the new
libGL they are reference counted, and released much sooner, so we
ran into a resource leak more noticeably with some tests.

Make the Apple DRI code dispatch events to the client(s) for
destroyed surfaces, when a resource is destroyed.  This seems to
work in my tests, however this clearly wasn't working for a while,
so bugs may result in the future if it enables some new (unexpected)
side effects.

Also add a few helpful comments to aid in understanding the code
in the future.

Tested with the test suite, Pymol, and various Mesa demos.
(cherry picked from commit bede83eb19a1629396fcd5a46441f8476a8fcd1b)
(cherry picked from commit 4fe7df265324f63025686efe9d32342e3cef40d3)

1 files changed, 45 insertions, 11 deletions

diff --git a/hw/xquartz/xpr/dri.c b/hw/xquartz/xpr/dri.c
index 3474d27a7..edc1d248e 100644
--- a/hw/xquartz/xpr/dri.c
+++ b/hw/xquartz/xpr/dri.c
@@ -395,8 +395,8 @@ DRICreateSurface(ScreenPtr pScreen, Drawable id,
 	    return FALSE; /*error*/
     }
 #endif
-    else { /* for GLX 1.3, a PBuffer */
-        /* NOT_DONE */
+    else {
+	/* We handle GLXPbuffers in a different way (via CGL). */
         return FALSE;
     }
     
@@ -486,13 +486,27 @@ DRIDestroySurface(ScreenPtr pScreen, Drawable id, DrawablePtr pDrawable,
     }
 
     if (pDRIDrawablePriv != NULL) {
+	/*
+	 * This doesn't seem to be used, because notify is NULL in all callers.
+	 */
+
         if (notify != NULL) {
             pDRIDrawablePriv->notifiers = x_hook_remove(pDRIDrawablePriv->notifiers,
                                                         notify, notify_data);
         }
-        if (--pDRIDrawablePriv->refCount <= 0) {
-            /* This calls back to DRIDrawablePrivDelete
-               which frees the private area */
+
+	--pDRIDrawablePriv->refCount;
+
+	/* 
+	 * Check if the drawable privates still have a reference to the
+	 * surface.
+	 */
+
+        if (pDRIDrawablePriv->refCount <= 0) {
+            /*
+	     * This calls back to DRIDrawablePrivDelete which
+	     * frees the private area and dispatches events, if needed. 
+	     */
             FreeResourceByType(id, DRIDrawablePrivResType, FALSE);
         }
     }
@@ -500,6 +514,10 @@ DRIDestroySurface(ScreenPtr pScreen, Drawable id, DrawablePtr pDrawable,
     return TRUE;
 }
 
+/* 
+ * The assumption is that this is called when the refCount of a surface
+ * drops to <= 0, or the window/pixmap is destroyed.  
+ */
 Bool
 DRIDrawablePrivDelete(pointer pResource, XID id)
 {
@@ -518,18 +536,23 @@ DRIDrawablePrivDelete(pointer pResource, XID id)
     }
 
     if (pDRIDrawablePriv == NULL) {
+	/* 
+	 * We reuse __func__ and the resource type for the GLXPixmap code.
+	 * Attempt to free a pixmap buffer associated with the resource
+	 * if possible.
+	 */
 	return DRIFreePixmapImp(pDrawable);
     }
-
+    
     if (pDRIDrawablePriv->drawableIndex != -1) {
         /* release drawable table entry */
         pDRIPriv->DRIDrawables[pDRIDrawablePriv->drawableIndex] = NULL;
     }
 
     if (pDRIDrawablePriv->sid != 0) {
-        xp_destroy_surface(pDRIDrawablePriv->sid);
-        x_hash_table_remove(surface_hash, x_cvt_uint_to_vptr(pDRIDrawablePriv->sid));
+	DRISurfaceNotify(pDRIDrawablePriv->sid, AppleDRISurfaceNotifyDestroyed);
     }
+  
 
     if (pDRIDrawablePriv->notifiers != NULL)
         x_hook_free(pDRIDrawablePriv->notifiers);
@@ -678,6 +701,11 @@ DRIQueryVersion(int *majorVersion,
     *patchVersion = APPLE_DRI_PATCH_VERSION;
 }
 
+/* 
+ * Note: this also cleans up the hash table in addition to notifying clients.
+ * The sid/surface-id should not be used after this, because it will be
+ * invalid.
+ */ 
 void
 DRISurfaceNotify(xp_surface_id id, int kind)
 {
@@ -698,21 +726,27 @@ DRISurfaceNotify(xp_surface_id id, int kind)
 
     if (kind == AppleDRISurfaceNotifyDestroyed)
     {
-        pDRIDrawablePriv->sid = 0;
-        x_hash_table_remove(surface_hash, x_cvt_uint_to_vptr(id));
+	x_hash_table_remove(surface_hash, x_cvt_uint_to_vptr(id));
     }
 
     x_hook_run(pDRIDrawablePriv->notifiers, &arg);
 
     if (kind == AppleDRISurfaceNotifyDestroyed)
     {
-        /* Kill off the handle. */
+	xp_destroy_surface(pDRIDrawablePriv->sid);
+	
+	/* Guard against reuse, even though we are freeing after this. */
+	pDRIDrawablePriv->sid = 0;
 
         FreeResourceByType(pDRIDrawablePriv->pDraw->id,
                            DRIDrawablePrivResType, FALSE);
     }
 }
 
+/*
+ * This creates a shared memory buffer for use with GLXPixmaps
+ * and AppleSGLX.
+ */
 Bool DRICreatePixmap(ScreenPtr pScreen, Drawable id,
 		     DrawablePtr pDrawable, char *path,
 		     size_t pathmax)