Fix X11 Display Connection leak w/ new GLAutoDrawableBase code when used w/ offscreen drawables, reported by Mark Raynsfordv2.0-rc10

New common GLAutoDrawableBase missed to close the AbstractGraphicsDevice
in case it has been created and dedicated for the passed GLDrawable.
This detailed knowledge is only known to the creator, hence it is passed
in the constructor and is being passed through all specializations.

Further more the new X11/GLX impl. of GLDrawableFactory's
'createMutableSurfaceImpl' always creates it's own private X11 display connection
to avoid locking / threading issues. Since the old implementation reused the
shared display connection which is prone to threading issues, this bug was not visible before.

Also fixed the unit test TestNEWTCloseX11DisplayBug565,
now correctly validating that no display connection is left over
after a new cycle of create/destroy of onscreen and offscreen drawables.

11 files changed, 61 insertions, 29 deletions

diff --git a/src/jogl/classes/com/jogamp/opengl/OffscreenAutoDrawable.java b/src/jogl/classes/com/jogamp/opengl/OffscreenAutoDrawable.java
index 1ea8595c6..8450ffdb0 100644
--- a/src/jogl/classes/com/jogamp/opengl/OffscreenAutoDrawable.java
+++ b/src/jogl/classes/com/jogamp/opengl/OffscreenAutoDrawable.java
@@ -28,6 +28,7 @@
 
 package com.jogamp.opengl;
 
+import javax.media.nativewindow.AbstractGraphicsDevice;
 import javax.media.opengl.GLAutoDrawableDelegate;
 import javax.media.opengl.GLContext;
 import javax.media.opengl.GLDrawable;
@@ -45,8 +46,16 @@ import jogamp.opengl.GLFBODrawableImpl;
  */
 public class OffscreenAutoDrawable extends GLAutoDrawableDelegate {
   
-    public OffscreenAutoDrawable(GLDrawable drawable, GLContext context, Object upstreamWidget) {
-        super(drawable, context, upstreamWidget);
+    /**
+     * @param drawable a valid {@link GLDrawable}, may not be realized yet.
+     * @param context a valid {@link GLContext}, may not be made current (created) yet.
+     * @param ownDevice pass <code>true</code> if {@link AbstractGraphicsDevice#close()} shall be issued,
+     *                  otherwise pass <code>false</code>. Closing the device is required in case
+     *                  the drawable is created w/ it's own new instance, e.g. offscreen drawables,
+     *                  and no further lifecycle handling is applied.
+     */
+    public OffscreenAutoDrawable(GLDrawable drawable, GLContext context, boolean ownDevice) {
+        super(drawable, context, null, ownDevice);
     }
 
     /**
diff --git a/src/jogl/classes/javax/media/opengl/GLAutoDrawableDelegate.java b/src/jogl/classes/javax/media/opengl/GLAutoDrawableDelegate.java
index 76959f3f4..67e81270d 100644
--- a/src/jogl/classes/javax/media/opengl/GLAutoDrawableDelegate.java
+++ b/src/jogl/classes/javax/media/opengl/GLAutoDrawableDelegate.java
@@ -28,6 +28,8 @@
  
 package javax.media.opengl;
 
+import javax.media.nativewindow.AbstractGraphicsDevice;
+
 import com.jogamp.common.util.locks.LockFactory;
 import com.jogamp.common.util.locks.RecursiveLock;
 
@@ -57,12 +59,16 @@ public class GLAutoDrawableDelegate extends GLAutoDrawableBase {
     public static final boolean DEBUG = Debug.debug("GLAutoDrawableDelegate");
     
     /**
-     * @param drawable
-     * @param context
+     * @param drawable a valid {@link GLDrawable}, may not be realized yet.
+     * @param context a valid {@link GLContext}, may not be made current (created) yet.
      * @param upstreamWidget optional UI element holding this instance, see {@link #getUpstreamWidget()}.
+     * @param ownDevice pass <code>true</code> if {@link AbstractGraphicsDevice#close()} shall be issued,
+     *                  otherwise pass <code>false</code>. Closing the device is required in case
+     *                  the drawable is created w/ it's own new instance, e.g. offscreen drawables,
+     *                  and no further lifecycle handling is applied.
      */
-    public GLAutoDrawableDelegate(GLDrawable drawable, GLContext context, Object upstreamWidget) {
-        super((GLDrawableImpl)drawable, (GLContextImpl)context);
+    public GLAutoDrawableDelegate(GLDrawable drawable, GLContext context, Object upstreamWidget, boolean ownDevice) {
+        super((GLDrawableImpl)drawable, (GLContextImpl)context, ownDevice);
         this.upstreamWidget = null;
     }
     
diff --git a/src/jogl/classes/jogamp/opengl/GLAutoDrawableBase.java b/src/jogl/classes/jogamp/opengl/GLAutoDrawableBase.java
index fe6d0fd76..cc4e1b434 100644
--- a/src/jogl/classes/jogamp/opengl/GLAutoDrawableBase.java
+++ b/src/jogl/classes/jogamp/opengl/GLAutoDrawableBase.java
@@ -30,6 +30,8 @@ package jogamp.opengl;
 
 import java.io.PrintStream;
 
+import javax.media.nativewindow.AbstractGraphicsConfiguration;
+import javax.media.nativewindow.AbstractGraphicsDevice;
 import javax.media.nativewindow.NativeSurface;
 import javax.media.nativewindow.WindowClosingProtocol;
 import javax.media.nativewindow.WindowClosingProtocol.WindowClosingMode;
@@ -66,13 +68,23 @@ public abstract class GLAutoDrawableBase implements GLAutoDrawable, FPSCounter {
     
     protected volatile GLDrawableImpl drawable; // volatile: avoid locking for read-only access
     protected GLContextImpl context;
+    protected final boolean ownDevice;
     protected int additionalCtxCreationFlags = 0;
     protected volatile boolean sendReshape = false; // volatile: maybe written by WindowManager thread w/o locking
     protected volatile boolean sendDestroy = false; // volatile: maybe written by WindowManager thread w/o locking
 
-    public GLAutoDrawableBase(GLDrawableImpl drawable, GLContextImpl context) {
+    /**
+     * @param drawable a valid {@link GLDrawableImpl}, may not be realized yet.
+     * @param context a valid {@link GLContextImpl}, may not be made current (created) yet.
+     * @param ownDevice pass <code>true</code> if {@link AbstractGraphicsDevice#close()} shall be issued,
+     *                  otherwise pass <code>false</code>. Closing the device is required in case
+     *                  the drawable is created w/ it's own new instance, e.g. offscreen drawables,
+     *                  and no further lifecycle handling is applied.
+     */
+    public GLAutoDrawableBase(GLDrawableImpl drawable, GLContextImpl context, boolean ownDevice) {
         this.drawable = drawable;
         this.context = context;
+        this.ownDevice = ownDevice;
         resetFPSCounter();        
     }
    
@@ -174,7 +186,7 @@ public abstract class GLAutoDrawableBase implements GLAutoDrawable, FPSCounter {
     }
     
     /**
-     * Calls {@link #destroyImplInLock()} while claiming the lock. 
+     * Calls {@link #destroyImplInLock()} while claiming the lock.
      */
     protected final void defaultDestroy() {
         final RecursiveLock lock = getLock();
@@ -200,17 +212,22 @@ public abstract class GLAutoDrawableBase implements GLAutoDrawable, FPSCounter {
     protected void destroyImplInLock() {
         final GLContext _context = context;
         final GLDrawable _drawable = drawable;
-        if( null != _drawable && _drawable.isRealized() ) {
-            if( null != _context && _context.isCreated() ) {
-                // Catch dispose GLExceptions by GLEventListener, just 'print' them
-                // so we can continue with the destruction.
-                try {
-                    helper.disposeGL(this, _drawable, _context, null);
-                } catch (GLException gle) {
-                    gle.printStackTrace();
+        if( null != _drawable ) {
+            if( _drawable.isRealized() ) {
+                if( null != _context && _context.isCreated() ) {
+                    // Catch dispose GLExceptions by GLEventListener, just 'print' them
+                    // so we can continue with the destruction.
+                    try {
+                        helper.disposeGL(this, _drawable, _context, null);
+                    } catch (GLException gle) {
+                        gle.printStackTrace();
+                    }
                 }
+                _drawable.setRealized(false);
+            }
+            if( ownDevice ) {
+                _drawable.getNativeSurface().getGraphicsConfiguration().getScreen().getDevice().close();
             }
-            _drawable.setRealized(false);
         }
         context = null;
         drawable = null;        
diff --git a/src/jogl/classes/jogamp/opengl/GLDrawableFactoryImpl.java b/src/jogl/classes/jogamp/opengl/GLDrawableFactoryImpl.java
index f092288fb..f7808294b 100644
--- a/src/jogl/classes/jogamp/opengl/GLDrawableFactoryImpl.java
+++ b/src/jogl/classes/jogamp/opengl/GLDrawableFactoryImpl.java
@@ -245,7 +245,7 @@ public abstract class GLDrawableFactoryImpl extends GLDrawableFactory {
     if(null==drawable) {
         throw new GLException("Could not create Pbuffer drawable for: "+device+", "+capsChosen+", "+width+"x"+height);
     }
-    return new GLPbufferImpl( drawable, shareWith);
+    return new GLPbufferImpl( drawable, shareWith, true);
   }
 
   //---------------------------------------------------------------------------
diff --git a/src/jogl/classes/jogamp/opengl/GLPbufferImpl.java b/src/jogl/classes/jogamp/opengl/GLPbufferImpl.java
index 6b64120fe..32f4cb696 100644
--- a/src/jogl/classes/jogamp/opengl/GLPbufferImpl.java
+++ b/src/jogl/classes/jogamp/opengl/GLPbufferImpl.java
@@ -58,8 +58,8 @@ import com.jogamp.common.util.locks.RecursiveLock;
 public class GLPbufferImpl extends GLAutoDrawableBase implements GLPbuffer {
   private int floatMode;
 
-  public GLPbufferImpl(GLDrawableImpl pbufferDrawable, GLContext sharedContext) {
-    super(pbufferDrawable, null); // drawable := pbufferDrawable 
+  public GLPbufferImpl(GLDrawableImpl pbufferDrawable, GLContext sharedContext, boolean ownDevice) {
+    super(pbufferDrawable, null, ownDevice); // drawable := pbufferDrawable 
     
     GLCapabilitiesImmutable caps = (GLCapabilitiesImmutable)
          drawable.getNativeSurface().getGraphicsConfiguration().getChosenCapabilities();
diff --git a/src/nativewindow/classes/jogamp/nativewindow/x11/X11Util.java b/src/nativewindow/classes/jogamp/nativewindow/x11/X11Util.java
index fcc374751..7b46a1df0 100644
--- a/src/nativewindow/classes/jogamp/nativewindow/x11/X11Util.java
+++ b/src/nativewindow/classes/jogamp/nativewindow/x11/X11Util.java
@@ -255,7 +255,7 @@ public class X11Util {
      */
     public static int shutdown(boolean realXCloseOpenAndPendingDisplays, boolean verbose) {
         int num=0;
-        if(DEBUG||verbose||pendingDisplayList.size() > 0) {
+        if(DEBUG || verbose || openDisplayMap.size() > 0 || pendingDisplayList.size() > 0) {
             System.err.println("X11Util.Display: Shutdown (close open / pending Displays: "+realXCloseOpenAndPendingDisplays+
                                ", open (no close attempt): "+openDisplayMap.size()+"/"+openDisplayList.size()+
                                ", pending (not closed, marked uncloseable): "+pendingDisplayList.size()+")");
diff --git a/src/newt/classes/com/jogamp/newt/opengl/GLWindow.java b/src/newt/classes/com/jogamp/newt/opengl/GLWindow.java
index d662a743a..0fc1b4e89 100644
--- a/src/newt/classes/com/jogamp/newt/opengl/GLWindow.java
+++ b/src/newt/classes/com/jogamp/newt/opengl/GLWindow.java
@@ -97,7 +97,7 @@ public class GLWindow extends GLAutoDrawableBase implements GLAutoDrawable, Wind
      * Constructor. Do not call this directly -- use {@link #create()} instead.
      */
     protected GLWindow(Window window) {
-        super(null, null);
+        super(null, null, false);
         this.window = (WindowImpl) window;
         this.window.setHandleDestroyNotify(false);
         window.addWindowListener(new WindowAdapter() {
diff --git a/src/test/com/jogamp/opengl/test/junit/jogl/acore/TestFBODrawableNEWT.java b/src/test/com/jogamp/opengl/test/junit/jogl/acore/TestFBODrawableNEWT.java
index 1a33845b3..7977347a7 100644
--- a/src/test/com/jogamp/opengl/test/junit/jogl/acore/TestFBODrawableNEWT.java
+++ b/src/test/com/jogamp/opengl/test/junit/jogl/acore/TestFBODrawableNEWT.java
@@ -167,7 +167,7 @@ public class TestFBODrawableNEWT extends UITestCase {
         final FBObject.RenderAttachment depthA = fbo.getDepthAttachment();
         Assert.assertNotNull(depthA);
         
-        final OffscreenAutoDrawable glad = new OffscreenAutoDrawable(fboDrawable, context, null);
+        final OffscreenAutoDrawable glad = new OffscreenAutoDrawable(fboDrawable, context, true);
 
         glad.addGLEventListener(demo);
         glad.addGLEventListener(new GLEventListener() {
diff --git a/src/test/com/jogamp/opengl/test/junit/jogl/acore/TestGLAutoDrawableDelegateNEWT.java b/src/test/com/jogamp/opengl/test/junit/jogl/acore/TestGLAutoDrawableDelegateNEWT.java
index 426b7734f..96d9b2e28 100644
--- a/src/test/com/jogamp/opengl/test/junit/jogl/acore/TestGLAutoDrawableDelegateNEWT.java
+++ b/src/test/com/jogamp/opengl/test/junit/jogl/acore/TestGLAutoDrawableDelegateNEWT.java
@@ -85,7 +85,7 @@ public class TestGLAutoDrawableDelegateNEWT extends UITestCase {
         Assert.assertTrue(GLContext.CONTEXT_CURRENT_NEW==res || GLContext.CONTEXT_CURRENT==res);
         context.release();
         
-        final GLAutoDrawableDelegate glad = new GLAutoDrawableDelegate(drawable, context, window) {
+        final GLAutoDrawableDelegate glad = new GLAutoDrawableDelegate(drawable, context, window, false) {
             @Override
             protected void destroyImplInLock() {
                 super.destroyImplInLock();  // destroys drawable/context
diff --git a/src/test/com/jogamp/opengl/test/junit/jogl/acore/TestGLContextDrawableSwitchNEWT.java b/src/test/com/jogamp/opengl/test/junit/jogl/acore/TestGLContextDrawableSwitchNEWT.java
index 92b4c5238..cece4c6d5 100644
--- a/src/test/com/jogamp/opengl/test/junit/jogl/acore/TestGLContextDrawableSwitchNEWT.java
+++ b/src/test/com/jogamp/opengl/test/junit/jogl/acore/TestGLContextDrawableSwitchNEWT.java
@@ -88,7 +88,7 @@ public class TestGLContextDrawableSwitchNEWT extends UITestCase {
         drawable.setRealized(true);
         Assert.assertTrue(drawable.isRealized());
         
-        final GLAutoDrawableDelegate glad = new GLAutoDrawableDelegate(drawable, null, window) {
+        final GLAutoDrawableDelegate glad = new GLAutoDrawableDelegate(drawable, null, window, false) {
             @Override
             protected void destroyImplInLock() {
                 super.destroyImplInLock();
diff --git a/src/test/com/jogamp/opengl/test/junit/jogl/acore/TestNEWTCloseX11DisplayBug565.java b/src/test/com/jogamp/opengl/test/junit/jogl/acore/TestNEWTCloseX11DisplayBug565.java
index e14d5b800..33a9b7799 100644
--- a/src/test/com/jogamp/opengl/test/junit/jogl/acore/TestNEWTCloseX11DisplayBug565.java
+++ b/src/test/com/jogamp/opengl/test/junit/jogl/acore/TestNEWTCloseX11DisplayBug565.java
@@ -43,10 +43,10 @@ public class TestNEWTCloseX11DisplayBug565 {
 
         if(NativeWindowFactory.TYPE_X11 == NativeWindowFactory.getNativeWindowType(false)) {
             final int openD = X11Util.getOpenDisplayConnectionNumber() - open0;
-            if(openD>1) {
+            if( openD > 0) {
                 X11Util.dumpOpenDisplayConnections();
                 X11Util.dumpPendingDisplayConnections();
-                Assert.assertTrue("More than 1 new open display connections", false);
+                Assert.assertEquals("New display connection didn't close", 0, openD);
             }
         }
       }
@@ -86,10 +86,10 @@ public class TestNEWTCloseX11DisplayBug565 {
 
         if(NativeWindowFactory.TYPE_X11 == NativeWindowFactory.getNativeWindowType(false)) {
             final int openD = X11Util.getOpenDisplayConnectionNumber() - open0;
-            if(openD>1) {
+            if(openD > 0) {
                 X11Util.dumpOpenDisplayConnections();
                 X11Util.dumpPendingDisplayConnections();
-                Assert.assertTrue("More than 1 new open display connections", false);
+                Assert.assertEquals("New display connection didn't close", 0, openD);
             }
         }
       }