8280468: Crashes in getConfigColormap, getConfigVisualId, XVisualIDFromVisual on Linux

Reviewed-by: serb, prr

src/java.desktop/unix/classes/sun/awt/X11/XCanvasPeer.java

@@ -62,36 +62,33 @@ class XCanvasPeer extends XComponentPeer implements CanvasPeer {
         if (graphicsConfig == null || gc == null) {
             return gc;
         }
-        // Opt: Only need to do if we're not using the default GC
 
-        int screenNum = ((X11GraphicsDevice)gc.getDevice()).getScreen();
+        final X11GraphicsDevice newDev = getSameScreenDevice(gc);
+        final int visualToLookFor = graphicsConfig.getVisual();
 
-        X11GraphicsConfig parentgc;
+        final GraphicsConfiguration[] configurations = newDev.getConfigurations();
-        // save vis id of current gc
+        for (final GraphicsConfiguration config : configurations) {
-        int visual = graphicsConfig.getVisual();
+            final X11GraphicsConfig x11gc = (X11GraphicsConfig) config;
-
+            if (visualToLookFor == x11gc.getVisual()) {
-        X11GraphicsDevice newDev = (X11GraphicsDevice) GraphicsEnvironment.
+                graphicsConfig = x11gc;
-            getLocalGraphicsEnvironment().
-            getScreenDevices()[screenNum];
-
-        for (int i = 0; i < newDev.getNumConfigs(screenNum); i++) {
-            if (visual == newDev.getConfigVisualId(i, screenNum)) {
-                // use that
-                graphicsConfig = (X11GraphicsConfig)newDev.getConfigurations()[i];
-                break;
             }
         }
-        // just in case...
-        if (graphicsConfig == null) {
-            graphicsConfig = (X11GraphicsConfig) GraphicsEnvironment.
-                getLocalGraphicsEnvironment().
-                getScreenDevices()[screenNum].
-                getDefaultConfiguration();
-        }
 
         return graphicsConfig;
     }
 
+    private X11GraphicsDevice getSameScreenDevice(GraphicsConfiguration gc) {
+        XToolkit.awtLock(); // so that the number of screens doesn't change during
+        try {
+            final int screenNum = ((X11GraphicsDevice) gc.getDevice()).getScreen();
+            return (X11GraphicsDevice) GraphicsEnvironment.
+                    getLocalGraphicsEnvironment().
+                    getScreenDevices()[screenNum];
+        } finally {
+            XToolkit.awtUnlock();
+        }
+    }
+
     protected boolean shouldFocusOnClick() {
         // Canvas should always be able to be focused by mouse clicks.
         return true;

src/java.desktop/unix/classes/sun/awt/X11GraphicsDevice.java

@@ -41,6 +41,7 @@ import java.util.HashSet;
 import sun.awt.util.ThreadGroupUtils;
 import sun.java2d.SunGraphicsEnvironment;
 import sun.java2d.loops.SurfaceType;
+import sun.awt.X11.XToolkit;
 import sun.java2d.opengl.GLXGraphicsConfig;
 import sun.java2d.pipe.Region;
 import sun.java2d.xr.XRGraphicsConfig;
@@ -63,7 +64,6 @@ public final class X11GraphicsDevice extends GraphicsDevice
 
     private static AWTPermission fullScreenExclusivePermission;
     private static Boolean xrandrExtSupported;
-    private final Object configLock = new Object();
     private SunDisplayChanger topLevels = new SunDisplayChanger();
     private DisplayMode origDisplayMode;
     private boolean shutdownHookRegistered;
@@ -150,8 +150,11 @@ public final class X11GraphicsDevice extends GraphicsDevice
     @Override
     public GraphicsConfiguration[] getConfigurations() {
         if (configs == null) {
-            synchronized (configLock) {
+            XToolkit.awtLock();
+            try {
                 makeConfigurations();
+            } finally {
+                XToolkit.awtUnlock();
             }
         }
         return configs.clone();
@@ -238,8 +241,11 @@ public final class X11GraphicsDevice extends GraphicsDevice
     @Override
     public GraphicsConfiguration getDefaultConfiguration() {
         if (defaultConfig == null) {
-            synchronized (configLock) {
+            XToolkit.awtLock();
+            try {
                 makeDefaultConfiguration();
+            } finally {
+                XToolkit.awtUnlock();
             }
         }
         return defaultConfig;
@@ -571,6 +577,8 @@ public final class X11GraphicsDevice extends GraphicsDevice
     }
 
     public void invalidate(X11GraphicsDevice device) {
+        assert XToolkit.isAWTLockHeldByCurrentThread();
+
         screen = device.screen;
     }
 }

src/java.desktop/unix/native/libawt_xawt/awt/awt_GraphicsEnv.c

@@ -292,6 +292,8 @@ makeDefaultConfig(JNIEnv *env, int screen) {
 
 static void
 getAllConfigs (JNIEnv *env, int screen, AwtScreenDataPtr screenDataPtr) {
+    // NB: should be invoked only while holding the AWT lock
+    DASSERT(screen >= 0 && screen < awt_numScreens);
 
     int i;
     int n8p=0, n12p=0, n8s=0, n8gs=0, n8sg=0, n1sg=0, nTrue=0;
@@ -314,8 +316,6 @@ getAllConfigs (JNIEnv *env, int screen, AwtScreenDataPtr screenDataPtr) {
         xinawareScreen = screen;
     }
 
-    AWT_LOCK ();
-
     viTmp.screen = xinawareScreen;
 
     viTmp.depth = 8;
@@ -372,7 +372,6 @@ getAllConfigs (JNIEnv *env, int screen, AwtScreenDataPtr screenDataPtr) {
     if (graphicsConfigs == NULL) {
         JNU_ThrowOutOfMemoryError((JNIEnv *)JNU_GetEnv(jvm, JNI_VERSION_1_2),
                                   NULL);
-        AWT_UNLOCK();
         return;
     }
 
@@ -382,6 +381,9 @@ getAllConfigs (JNIEnv *env, int screen, AwtScreenDataPtr screenDataPtr) {
          * been reset, so we need to recreate the default config here.
          */
         screenDataPtr->defaultConfig = makeDefaultConfig(env, screen);
+        if (screenDataPtr->defaultConfig == NULL) {
+            return;
+        }
     }
 
     defaultConfig = screenDataPtr->defaultConfig;
@@ -581,7 +583,6 @@ cleanup:
        XFree (pVI1sg);
     if (nTrue != 0)
        XFree (pVITrue);
-    AWT_UNLOCK ();
 }
 
 /*
@@ -770,6 +771,7 @@ JNIEnv *env, jobject this)
 }
 
 static void ensureConfigsInited(JNIEnv* env, int screen) {
+    // NB: should be invoked only while holding the AWT lock
    if (x11Screens[screen].numConfigs == 0) {
        if (env == NULL) {
            env = (JNIEnv *)JNU_GetEnv(jvm, JNI_VERSION_1_2);
@@ -780,6 +782,8 @@ static void ensureConfigsInited(JNIEnv* env, int screen) {
 
 AwtGraphicsConfigDataPtr
 getDefaultConfig(int screen) {
+    // NB: should be invoked only while holding the AWT lock
+    DASSERT(screen >= 0 && screen < awt_numScreens);
     ensureConfigsInited(NULL, screen);
     return x11Screens[screen].defaultConfig;
 }
@@ -973,11 +977,11 @@ JNIEXPORT jint JNICALL
 Java_sun_awt_X11GraphicsDevice_getNumConfigs(
 JNIEnv *env, jobject this, jint screen)
 {
-    AWT_LOCK();
+    // NB: should be invoked only while holding the AWT lock
+    DASSERT(screen >= 0 && screen < awt_numScreens);
     ensureConfigsInited(env, screen);
-    int configs = x11Screens[screen].numConfigs;
+    return x11Screens[screen].numConfigs;
-    AWT_UNLOCK();
+
-    return configs;
 }
 
 /*
@@ -989,12 +993,11 @@ JNIEXPORT jint JNICALL
 Java_sun_awt_X11GraphicsDevice_getConfigVisualId(
 JNIEnv *env, jobject this, jint index, jint screen)
 {
-    int visNum;
+    // NB: should be invoked only while holding the AWT lock
-    AWT_LOCK();
+    DASSERT(screen >= 0 && screen < awt_numScreens);
     ensureConfigsInited(env, screen);
     jint id = (jint) (index == 0 ? x11Screens[screen].defaultConfig
                                  : x11Screens[screen].configs[index])->awt_visInfo.visualid;
-    AWT_UNLOCK();
     return id;
 }
 
@@ -1007,12 +1010,11 @@ JNIEXPORT jint JNICALL
 Java_sun_awt_X11GraphicsDevice_getConfigDepth(
 JNIEnv *env, jobject this, jint index, jint screen)
 {
-    int visNum;
+    // NB: should be invoked only while holding the AWT lock
-    AWT_LOCK();
+    DASSERT(screen >= 0 && screen < awt_numScreens);
     ensureConfigsInited(env, screen);
     jint depth = (jint) (index == 0 ? x11Screens[screen].defaultConfig
                                     : x11Screens[screen].configs[index])->awt_visInfo.depth;
-    AWT_UNLOCK();
     return depth;
 }
 
@@ -1025,12 +1027,11 @@ JNIEXPORT jint JNICALL
 Java_sun_awt_X11GraphicsDevice_getConfigColormap(
 JNIEnv *env, jobject this, jint index, jint screen)
 {
-    int visNum;
+    // NB: should be invoked only while holding the AWT lock
-    AWT_LOCK();
+    DASSERT(screen >= 0 && screen < awt_numScreens);
     ensureConfigsInited(env, screen);
     jint colormap = (jint) (index == 0 ? x11Screens[screen].defaultConfig
                                        : x11Screens[screen].configs[index])->awt_cmap;
-    AWT_UNLOCK();
     return colormap;
 }
 
@@ -1140,8 +1141,10 @@ JNIEXPORT void JNICALL
 Java_sun_awt_X11GraphicsConfig_init(
 JNIEnv *env, jobject this, jint visualNum, jint screen)
 {
+    // NB: should be invoked only while holding the AWT lock
+    DASSERT(screen >= 0 && screen < awt_numScreens);
+
     AwtGraphicsConfigData *adata = NULL;
-    AWT_LOCK();
     AwtScreenData asd = x11Screens[screen];
     int i, n;
     int depth;
@@ -1163,7 +1166,6 @@ JNIEnv *env, jobject this, jint visualNum, jint screen)
 
     /* If didn't find the visual, throw an exception... */
     if (adata == (AwtGraphicsConfigData *) NULL) {
-        AWT_UNLOCK();
         JNU_ThrowIllegalArgumentException(env, "Unknown Visual Specified");
         return;
     }
@@ -1182,7 +1184,6 @@ JNIEnv *env, jobject this, jint visualNum, jint screen)
     (*env)->SetIntField(env, this, x11GraphicsConfigIDs.bitsPerPixel,
                         (jint)tempImage->bits_per_pixel);
     XDestroyImage(tempImage);
-    AWT_UNLOCK();
 }
 
 /*