Analysis of glXMakeCurrent freeze on ATI fglrx 8.78.6; Misc ..

Analysis of glXMakeCurrent freeze on ATI fglrx 8.78.6
    - Workaround in TestGLWindows01NEWT: same create/destroy order
    - Prove bug with simple native test app: jogl/test/native/displayMultiple02.c

Misc:
    - Reverted d52181032830acdd5e4069a41ccd0daff5922d8a, ie reenable x11IOErrorHandler (nativewindow)
    - GLDrawableHelper: methods -> final
    - X11Util.NamedDisplay: remove unused RecursiveLock, Cloneable

8 files changed, 247 insertions, 30 deletions

diff --git a/src/jogl/classes/com/jogamp/opengl/impl/GLDrawableHelper.java b/src/jogl/classes/com/jogamp/opengl/impl/GLDrawableHelper.java
index 4ad0dd4c3..c8cfd9a01 100644
--- a/src/jogl/classes/com/jogamp/opengl/impl/GLDrawableHelper.java
+++ b/src/jogl/classes/com/jogamp/opengl/impl/GLDrawableHelper.java
@@ -62,7 +62,7 @@ public class GLDrawableHelper {
     reset();
   }
 
-  public void reset() {
+  public final void reset() {
     synchronized(listenersLock) {
         listeners = new ArrayList();
         listenersIter = false;
@@ -75,7 +75,7 @@ public class GLDrawableHelper {
     animatorCtrl = null;
   }
 
-  public String toString() {
+  public final String toString() {
     StringBuffer sb = new StringBuffer();
     sb.append("GLAnimatorControl: "+animatorCtrl+", ");
     synchronized(listenersLock) {
@@ -94,11 +94,11 @@ public class GLDrawableHelper {
     return sb.toString();
   }
 
-  public void addGLEventListener(GLEventListener listener) {
+  public final void addGLEventListener(GLEventListener listener) {
     addGLEventListener(-1, listener);
   }
 
-  public void addGLEventListener(int index, GLEventListener listener) {
+  public final void addGLEventListener(int index, GLEventListener listener) {
     synchronized(listenersLock) {
         if(0>index) {
             index = listeners.size();
@@ -116,7 +116,7 @@ public class GLDrawableHelper {
     }
   }
   
-  public void removeGLEventListener(GLEventListener listener) {
+  public final void removeGLEventListener(GLEventListener listener) {
     synchronized(listenersLock) {
         if(!listenersIter) {
             // fast path
@@ -131,7 +131,7 @@ public class GLDrawableHelper {
     }
   }
 
-  public void dispose(GLAutoDrawable drawable) {
+  public final void dispose(GLAutoDrawable drawable) {
     synchronized(listenersLock) {
         listenersIter = true;
         for (int i=0; i < listeners.size(); i++) {
@@ -154,7 +154,7 @@ public class GLDrawableHelper {
       return false;
   }
 
-  public void init(GLAutoDrawable drawable) {
+  public final void init(GLAutoDrawable drawable) {
     synchronized(listenersLock) {
         listenersIter = true;
         for (int i=0; i < listeners.size(); i++) {
@@ -167,7 +167,7 @@ public class GLDrawableHelper {
     }
   }
 
-  public void display(GLAutoDrawable drawable) {
+  public final void display(GLAutoDrawable drawable) {
     synchronized(listenersLock) {
         listenersIter = true;
         for (int i=0; i < listeners.size(); i++) {
@@ -190,7 +190,7 @@ public class GLDrawableHelper {
     listener.reshape(drawable, x, y, width, height);
   }
 
-  public void reshape(GLAutoDrawable drawable, int x, int y, int width, int height) {
+  public final void reshape(GLAutoDrawable drawable, int x, int y, int width, int height) {
     synchronized(listenersLock) {
         listenersIter = true;
         for (int i=0; i < listeners.size(); i++) {
@@ -200,7 +200,7 @@ public class GLDrawableHelper {
     }
   }
 
-  private void execGLRunnables(GLAutoDrawable drawable) {
+  private final void execGLRunnables(GLAutoDrawable drawable) {
     if(glRunnables.size()>0) {
         // swap one-shot list asap
         ArrayList _glRunnables = null;
@@ -218,7 +218,7 @@ public class GLDrawableHelper {
     }
   }
 
-  public void setAnimator(GLAnimatorControl animator) throws GLException {
+  public final void setAnimator(GLAnimatorControl animator) throws GLException {
     synchronized(glRunnablesLock) {
         if(animatorCtrl!=animator && null!=animator && null!=animatorCtrl) {
             throw new GLException("Trying to register GLAnimatorControl "+animator+", where "+animatorCtrl+" is already registered. Unregister first.");
@@ -227,7 +227,7 @@ public class GLDrawableHelper {
     }
   }
 
-  public GLAnimatorControl getAnimator() {
+  public final GLAnimatorControl getAnimator() {
     synchronized(glRunnablesLock) {
         return animatorCtrl;
     }
@@ -237,7 +237,7 @@ public class GLDrawableHelper {
     return ( null != animatorCtrl ) ? animatorCtrl.isAnimating() && animatorCtrl.getThread() != Thread.currentThread() : false ;
   }
 
-  public void invoke(GLAutoDrawable drawable, boolean wait, GLRunnable glRunnable) {
+  public final void invoke(GLAutoDrawable drawable, boolean wait, GLRunnable glRunnable) {
     if( null == drawable || null == glRunnable ) {
         return;
     }
@@ -274,11 +274,11 @@ public class GLDrawableHelper {
     }
   }
 
-  public void setAutoSwapBufferMode(boolean onOrOff) {
+  public final void setAutoSwapBufferMode(boolean onOrOff) {
     autoSwapBufferMode = onOrOff;
   }
 
-  public boolean getAutoSwapBufferMode() {
+  public final boolean getAutoSwapBufferMode() {
     return autoSwapBufferMode;
   }
 
@@ -289,10 +289,10 @@ public class GLDrawableHelper {
       the GLContext's public APIs, and putting it into a separate
       class helps ensure that we don't inadvertently use private
       methods of the GLContext or its implementing classes. */
-  public void invokeGL(GLDrawable drawable,
-                       GLContext context,
-                       Runnable  runnable,
-                       Runnable  initAction) {
+  public final void invokeGL(GLDrawable drawable,
+                             GLContext context,
+                             Runnable  runnable,
+                             Runnable  initAction) {
     if(null==context) {
         if (DEBUG) {
             Exception e = new GLException(Thread.currentThread().getName()+"Info: GLDrawableHelper " + this + ".invokeGL(): NULL GLContext");
diff --git a/src/jogl/classes/com/jogamp/opengl/impl/x11/glx/X11GLXDrawableFactory.java b/src/jogl/classes/com/jogamp/opengl/impl/x11/glx/X11GLXDrawableFactory.java
index 5915ad55c..2828a90f1 100644
--- a/src/jogl/classes/com/jogamp/opengl/impl/x11/glx/X11GLXDrawableFactory.java
+++ b/src/jogl/classes/com/jogamp/opengl/impl/x11/glx/X11GLXDrawableFactory.java
@@ -162,8 +162,8 @@ public class X11GLXDrawableFactory extends GLDrawableFactoryImpl {
         sharedDrawable=null;
     }
     if(null!=sharedScreen) {
-         // may cause deadlock: X11Util.closeThreadLocalDisplay(null);
-         sharedScreen = null;
+        // may cause deadlock: X11Util.closeThreadLocalDisplay(sharedScreen.getDevice().getHandle());
+        sharedScreen = null;
     }
     // don't close pending XDisplay, since this might be a different thread as the opener
     X11Util.shutdown( false, DEBUG );
diff --git a/src/junit/com/jogamp/test/junit/newt/TestGLWindows01NEWT.java b/src/junit/com/jogamp/test/junit/newt/TestGLWindows01NEWT.java
index 8d0561a81..73d962dfa 100644
--- a/src/junit/com/jogamp/test/junit/newt/TestGLWindows01NEWT.java
+++ b/src/junit/com/jogamp/test/junit/newt/TestGLWindows01NEWT.java
@@ -72,6 +72,7 @@ public class TestGLWindows01NEWT extends UITestCase {
         GLWindow glWindow;
         if(null!=screen) {
             boolean destroyWhenUnused = screen.getDestroyWhenUnused();
+            Assert.assertEquals(destroyWhenUnused, screen.getDisplay().getDestroyWhenUnused());
             glWindow = GLWindow.create(screen, caps);
             Assert.assertNotNull(glWindow);
             Assert.assertEquals(destroyWhenUnused, glWindow.getScreen().getDestroyWhenUnused());
@@ -244,14 +245,76 @@ public class TestGLWindows01NEWT extends UITestCase {
     }
 
     @Test
-    public void testWindowDecor03TwoWin() throws InterruptedException {
+    public void testWindowDecor03TwoWinOneDisplay() throws InterruptedException {
+        GLCapabilities caps = new GLCapabilities(glp);
+        Assert.assertNotNull(caps);
+
+        Display display = NewtFactory.createDisplay(null); // local display
+        Assert.assertNotNull(display);
+        display.setDestroyWhenUnused(true);
+
+        Screen screen1  = NewtFactory.createScreen(display, 0); // screen 0
+        Assert.assertNotNull(screen1);
+        GLWindow window1 = createWindow(screen1, caps, width, height,
+                                       true /* onscreen */, false /* undecorated */, 
+                                       false /*addGLEventListenerAfterVisible*/);
+        Assert.assertNotNull(window1);
+
+        Screen screen2  = NewtFactory.createScreen(display, 0); // screen 0
+        Assert.assertNotNull(screen2);
+        GLWindow window2 = createWindow(screen2, caps, width, height, 
+                                       true /* onscreen */, false /* undecorated */, 
+                                       false /*addGLEventListenerAfterVisible*/);
+        Assert.assertNotNull(window2);
+
+        Assert.assertEquals(1,Display.getActiveDisplayNumber());
+
+        Assert.assertEquals(2,display.getReferenceCount());
+        Assert.assertEquals(true,display.isNativeValid());
+        Assert.assertNotNull(display.getEDTUtil());
+        Assert.assertEquals(true,display.getEDTUtil().isRunning());
+
+        Assert.assertEquals(1,screen1.getReferenceCount());
+        Assert.assertEquals(true,screen1.isNativeValid());
+
+        Assert.assertEquals(1,screen2.getReferenceCount());
+        Assert.assertEquals(true,screen2.isNativeValid());
+
+        int state;
+        for(state=0; state*100<durationPerTest; state++) {
+            Thread.sleep(100);
+        }
+        System.out.println("duration1: "+window1.getDuration());
+        System.out.println("duration2: "+window2.getDuration());
+
+        destroyWindow(window2, null, null, true);
+        destroyWindow(window1, null, null, true);
+
+        Assert.assertEquals(0,Display.getActiveDisplayNumber());
+
+        Assert.assertEquals(0,display.getReferenceCount());
+        Assert.assertEquals(false,display.isNativeValid());
+        Assert.assertNotNull(display.getEDTUtil());
+        Assert.assertEquals(false,display.getEDTUtil().isRunning());
+
+        Assert.assertEquals(0,screen1.getReferenceCount());
+        Assert.assertEquals(false,screen1.isNativeValid());
+
+        Assert.assertEquals(0,screen2.getReferenceCount());
+        Assert.assertEquals(false,screen2.isNativeValid());
+    }
+
+    @Test
+    public void testWindowDecor03TwoWinTwoDisplays() throws InterruptedException {
         GLCapabilities caps = new GLCapabilities(glp);
         Assert.assertNotNull(caps);
 
         Display display1 = NewtFactory.createDisplay(null); // local display
         Assert.assertNotNull(display1);
+        display1.setDestroyWhenUnused(true);
         Display display2 = NewtFactory.createDisplay(null); // local display
         Assert.assertNotNull(display2);
+        display2.setDestroyWhenUnused(true);
         Assert.assertNotSame(display1, display2);
 
         Screen screen1  = NewtFactory.createScreen(display1, 0); // screen 0
@@ -291,8 +354,14 @@ public class TestGLWindows01NEWT extends UITestCase {
         System.out.println("duration1: "+window1.getDuration());
         System.out.println("duration2: "+window2.getDuration());
 
-        destroyWindow(window2, screen2, display2, true);
-        destroyWindow(window1, screen1, display1, true);
+        // It is observed that some X11 drivers, eg ATI, fglrx 8.78.6,
+        // are quite sensitive to multiple Display connections (NEWT Display -> X11 Display).
+        // In such cases, closing displays shall happen in the same order as
+        // opening them, otherwise some driver related bug appears.
+        // You may test this, ie just reverse the destroy order below.
+        // See also native test: jogl/test/native/displayMultiple02.c
+        destroyWindow(window1, null, null, true);
+        destroyWindow(window2, null, null, true);
 
         Assert.assertEquals(0,Display.getActiveDisplayNumber());
 
@@ -308,7 +377,7 @@ public class TestGLWindows01NEWT extends UITestCase {
         Assert.assertNotNull(display2.getEDTUtil());
         Assert.assertEquals(false,display2.getEDTUtil().isRunning());
         Assert.assertEquals(0,screen2.getReferenceCount());
-        Assert.assertEquals(false,screen2.isNativeValid());
+        Assert.assertEquals(false,screen2.isNativeValid()); 
     }
 
     public static void setDemoFields(GLEventListener demo, GLWindow glWindow) {
diff --git a/src/nativewindow/classes/com/jogamp/nativewindow/impl/x11/X11Util.java b/src/nativewindow/classes/com/jogamp/nativewindow/impl/x11/X11Util.java
index 5e89e9f50..14e285187 100644
--- a/src/nativewindow/classes/com/jogamp/nativewindow/impl/x11/X11Util.java
+++ b/src/nativewindow/classes/com/jogamp/nativewindow/impl/x11/X11Util.java
@@ -126,7 +126,7 @@ public class X11Util {
 
     private static ThreadLocal currentDisplayMap = new ThreadLocal();
 
-    public static class NamedDisplay extends RecursiveLock implements Cloneable {
+    public static class NamedDisplay {
         String name;
         long   handle;
         int    refCount;
@@ -142,6 +142,8 @@ public class X11Util {
         public final String getName() { return name; }
         public final long   getHandle() { return handle; }
         public final int    getRefCount() { return refCount; }
+
+        public void setUncloseable(boolean v) { unCloseable = v; }
         public final boolean isUncloseable() { return unCloseable; }
 
         public Object clone() throws CloneNotSupportedException {
@@ -158,9 +160,9 @@ public class X11Util {
       */
     public static int shutdown(boolean realXClosePendingDisplays, boolean verbose) {
         int num=0;
-        String msg = null;
         if(DEBUG||verbose) {
-            msg = "X11Util.Display: Shutdown (global: "+globalNamedDisplayMap.size()+ ")" ;
+            String msg = "X11Util.Display: Shutdown (closePendingDisplays: "+realXClosePendingDisplays+
+                         ", global: "+globalNamedDisplayMap.size()+ ")" ;
             if(DEBUG) {
                 Exception e = new Exception(msg);
                 e.printStackTrace();
@@ -269,7 +271,7 @@ public class X11Util {
             ndpy = (NamedDisplay) globalNamedDisplayMap.get(handle);
         }
         if( null != ndpy ) {
-            ndpy.unCloseable=true;
+            ndpy.setUncloseable(true);
             return true;
         }
         return false;
@@ -355,11 +357,22 @@ public class X11Util {
             throw new RuntimeException("X11Util.Display: Display(0x"+Long.toHexString(handle)+") Mapping error: "+namedDpy+". Thread "+Thread.currentThread().getName());
         }
 
+        if(DEBUG) {
+            Exception e = new Exception("X11Util.Display: Closing new "+namedDpy+". Thread "+Thread.currentThread().getName());
+            e.printStackTrace();
+        }
+
         if(!namedDpy.isUncloseable()) {
             XCloseDisplay(namedDpy.getHandle());
         }
     }
 
+    public static NamedDisplay getNamedDisplay(long handle) {
+        synchronized(globalLock) {
+            return (NamedDisplay) globalNamedDisplayMap.get(handle);
+        }
+    }
+
     /** 
      * @return If name is null, it returns the previous queried NULL display name,
      * otherwise the name. */
diff --git a/src/nativewindow/native/x11/Xmisc.c b/src/nativewindow/native/x11/Xmisc.c
index 57bc6fc59..31d57240f 100644
--- a/src/nativewindow/native/x11/Xmisc.c
+++ b/src/nativewindow/native/x11/Xmisc.c
@@ -253,7 +253,7 @@ Java_com_jogamp_nativewindow_impl_x11_X11Util_initialize(JNIEnv *env, jclass _un
         }
 
         _initClazzAccess(env);
-        // x11IOErrorHandlerEnable(1, env); // JAU FIXME: can't enable without disable
+        x11IOErrorHandlerEnable(1, env);
         _initialized=1;
     }
 }
diff --git a/test/native/displayMultiple01.c b/test/native/displayMultiple01.c
new file mode 100644
index 000000000..d51453687
--- /dev/null
+++ b/test/native/displayMultiple01.c
@@ -0,0 +1,18 @@
+#include <X11/X.h>
+#include <X11/Xlib.h>
+#include <X11/Xutil.h>
+#include <GL/glx.h>
+#include <stdio.h>
+
+int main(int nargs, char **vargs) {
+    int major, minor;
+    Display *disp = XOpenDisplay(NULL);
+    glXQueryVersion(disp, &major, &minor);
+    fprintf(stderr, "%p: %d.%d\n", disp, major, minor);
+    XCloseDisplay(disp);
+    disp = XOpenDisplay(NULL);
+    glXQueryVersion(disp, &major, &minor);
+    fprintf(stderr, "%p: %d.%d\n", disp, major, minor);
+    XCloseDisplay(disp);
+    return 0;
+}
diff --git a/test/native/displayMultiple02.c b/test/native/displayMultiple02.c
new file mode 100644
index 000000000..1bfe95b95
--- /dev/null
+++ b/test/native/displayMultiple02.c
@@ -0,0 +1,113 @@
+/**
+ * compile with: gcc -o displayMultiple02 displayMultiple02.c -lX11 -lGL
+ */
+
+#include <stdio.h>
+#include <X11/X.h>
+#include <X11/Xlib.h>
+#include <X11/Xutil.h>
+#include <GL/glx.h>
+#include <GL/gl.h>
+
+static void testOrder(int reverseDestroyOrder, const char * msg);
+
+int main(int nargs, char **vargs) {
+    testOrder(0, "Normal order");
+    testOrder(1, "Reverse order");
+    return 0;
+}
+
+static void createGLWin(Display *dpy, int width, int height, Window *rWin, GLXContext *rCtx);
+static void useGL(Display *dpy, Window win, GLXContext ctx, int width, int height);
+
+void testOrder(int reverseDestroyOrder, const char * msg) {
+    int major, minor;
+    Display *disp1;
+    Window win1;
+    GLXContext ctx1;
+
+    Display *disp2;
+    Window win2;
+    GLXContext ctx2;
+
+    fprintf(stderr, "%s: Create #1\n", msg);
+    disp1 = XOpenDisplay(NULL);
+    createGLWin(disp1, 200, 200, &win1, &ctx1);
+    useGL(disp1, win1, ctx1, 200, 200);
+
+    fprintf(stderr, "%s: Create #2\n", msg);
+    disp2 = XOpenDisplay(NULL);
+    createGLWin(disp2, 300, 300, &win2, &ctx2);
+    useGL(disp2, win2, ctx2, 300, 300);
+
+    if(reverseDestroyOrder) {
+        fprintf(stderr, "%s: Destroy #2\n", msg);
+        glXMakeCurrent(disp2, 0, 0);
+        glXDestroyContext(disp2, ctx2);
+        XCloseDisplay(disp2);
+
+        fprintf(stderr, "%s: Destroy #1\n", msg);
+        glXMakeCurrent(disp1, 0, 0);
+        glXDestroyContext(disp1, ctx1);
+        XCloseDisplay(disp1);
+    } else {
+        fprintf(stderr, "%s: Destroy #1\n", msg);
+        glXMakeCurrent(disp1, 0, 0);
+        glXDestroyContext(disp1, ctx1);
+        XCloseDisplay(disp1);
+
+        fprintf(stderr, "%s: Destroy #2\n", msg);
+        glXMakeCurrent(disp2, 0, 0);
+        glXDestroyContext(disp2, ctx2);
+        XCloseDisplay(disp2);
+
+    }
+
+    fprintf(stderr, "%s: Success - no bug\n", msg);
+}
+
+/* attributes for a double buffered visual in RGBA format with at least
+ * 4 bits per color and a 16 bit depth buffer */
+static int attrListDbl[] = { GLX_RGBA, GLX_DOUBLEBUFFER, 
+    GLX_RED_SIZE, 4, 
+    GLX_GREEN_SIZE, 4, 
+    GLX_BLUE_SIZE, 4, 
+    GLX_DEPTH_SIZE, 16,
+    None };
+
+void createGLWin(Display *dpy, int width, int height, Window *rWin, GLXContext *rCtx)
+{
+    int screen = DefaultScreen(dpy);
+    XVisualInfo *vi = glXChooseVisual(dpy, screen, attrListDbl);
+    Colormap cmap;
+    XSetWindowAttributes attr;
+
+    /* create a GLX context */
+    *rCtx = glXCreateContext(dpy, vi, 0, GL_TRUE);
+    /* create a color map */
+    cmap = XCreateColormap(dpy, RootWindow(dpy, vi->screen), vi->visual, AllocNone);
+    attr.colormap = cmap;
+    attr.border_pixel = 0;
+
+    /* create a window in window mode*/
+    attr.event_mask = ExposureMask | KeyPressMask | ButtonPressMask |
+        StructureNotifyMask;
+    *rWin = XCreateWindow(dpy, RootWindow(dpy, vi->screen),
+        0, 0, width, height, 0, vi->depth, InputOutput, vi->visual,
+        CWBorderPixel | CWColormap | CWEventMask, &attr);
+
+    XMapRaised(dpy, *rWin);
+}
+
+void useGL(Display *dpy, Window win, GLXContext ctx, int width, int height)
+{
+    glXMakeCurrent(dpy, win, ctx);
+    glShadeModel(GL_SMOOTH);
+    glClearColor(0.0f, 0.0f, 0.0f, 0.0f);
+    glClearDepth(1.0f);
+    glViewport(0, 0, width, height);
+    glClear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT);
+    glXSwapBuffers(dpy, win);
+    glXMakeCurrent(dpy, 0, 0);
+}
+
diff --git a/test/native/make.sh b/test/native/make.sh
new file mode 100755
index 000000000..2552eddb0
--- /dev/null
+++ b/test/native/make.sh
@@ -0,0 +1,4 @@
+#! /bin/bash
+
+gcc -o displayMultiple01 displayMultiple01.c -lX11 -lGL
+gcc -o displayMultiple02 displayMultiple02.c -lX11 -lGL