# HG changeset patch # User Andrew Azores # Date 1388155884 18000 # Node ID 28ac9a016c80327a360fc1de12214188a017a8f0 # Parent c7bd4f6cf1b7e78173fb8f7b18931847e503874e Fix classloading deadlock regression Resolve deadlock issue in JNLPClassLoader. See http://mail.openjdk.java.net/pipermail/distro-pkg-dev/2013-December/025546.html * netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java: (loadClassLock) removed. (available, jarIndexes, classpaths, jarEntries, jarLocationSecurityMap) fields wrapped in Collections.synchronized*() to provide atomic read/write. Synchronized on while iterating over these collections. (loadClass) no longer uses implicit JNLPClassLoader instance lock nor dedicated loadClassLock object. diff -r c7bd4f6cf1b7 -r 28ac9a016c80 ChangeLog --- a/ChangeLog Fri Dec 20 13:25:38 2013 +0100 +++ b/ChangeLog Fri Dec 27 09:51:24 2013 -0500 @@ -1,3 +1,13 @@ +2013-12-27 Andrew Azores + + Resolve deadlock issue in JNLPClassLoader. See + http://mail.openjdk.java.net/pipermail/distro-pkg-dev/2013-December/025546.html + * netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java: (loadClassLock) + removed. (available, jarIndexes, classpaths, jarEntries, jarLocationSecurityMap) + fields wrapped in Collections.synchronized*() to provide atomic read/write. + Synchronized on while iterating over these collections. (loadClass) no longer + uses implicit JNLPClassLoader instance lock nor dedicated loadClassLock object. + 2013-12-20 Jiri Vanek backported singletons logic, logs and test cleanup/fixes diff -r c7bd4f6cf1b7 -r 28ac9a016c80 netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java --- a/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java Fri Dec 20 13:25:38 2013 +0100 +++ b/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java Fri Dec 27 09:51:24 2013 -0500 @@ -167,26 +167,41 @@ /** Permissions granted by the user during runtime. */ private ArrayList runtimePermissions = new ArrayList(); - /** all jars not yet part of classloader or active */ - private List available = new ArrayList(); + /** all jars not yet part of classloader or active + * Synchronized since this field may become shared data between multiple classloading threads. + * See loadClass(String) and CodebaseClassLoader.findClassNonRecursive(String). + */ + private List available = Collections.synchronizedList(new ArrayList()); /** the jar cert verifier tool to verify our jars */ private final JarCertVerifier jcv; private boolean signing = false; - /** ArrayList containing jar indexes for various jars available to this classloader */ - private ArrayList jarIndexes = new ArrayList(); + /** ArrayList containing jar indexes for various jars available to this classloader + * Synchronized since this field may become shared data between multiple classloading threads. + * See loadClass(String) and CodebaseClassLoader.findClassNonRecursive(String). + */ + private List jarIndexes = Collections.synchronizedList(new ArrayList()); - /** Set of classpath strings declared in the manifest.mf files */ - private Set classpaths = new HashSet(); + /** Set of classpath strings declared in the manifest.mf files + * Synchronized since this field may become shared data between multiple classloading threads. + * See loadClass(String) and CodebaseClassLoader.findClassNonRecursive(String). + */ + private Set classpaths = Collections.synchronizedSet(new HashSet()); - /** File entries in the jar files available to this classloader */ - private TreeSet jarEntries = new TreeSet(); + /** File entries in the jar files available to this classloader + * Synchronized since this field may become shared data between multiple classloading threads. + * See loadClass(String) and CodebaseClassLoader.findClassNonRecursive(String). + */ + private Set jarEntries = Collections.synchronizedSet(new TreeSet()); - /** Map of specific original (remote) CodeSource Urls to securitydesc */ - private HashMap jarLocationSecurityMap = - new HashMap(); + /** Map of specific original (remote) CodeSource Urls to securitydesc + * Synchronized since this field may become shared data between multiple classloading threads. + * See loadClass(String) and CodebaseClassLoader.findClassNonRecursive(String). + */ + private Map jarLocationSecurityMap = + Collections.synchronizedMap(new HashMap()); /*Set to prevent once tried-to-get resources to be tried again*/ private Set alreadyTried = Collections.synchronizedSet(new HashSet()); @@ -206,16 +221,6 @@ */ private int useCount = 0; - /* This Object is used as a lock on the loadClass(String) method. This method should not - * be entered by multiple threads simultaneously. This Object should be used for no other - * purpose than synchronizing the body of the loadClass(String) method. The intrinsic instance - * lock is not suitable for this purpose or else deadlock may occur. - * - * See bug RH976833 and the mailing list archive discussion: - * http://mail.openjdk.java.net/pipermail/distro-pkg-dev/2013-September/024536.html - */ - private final Object loadClassLock = new Object(); - /** * Create a new JNLPClassLoader from the specified file. * @@ -1222,10 +1227,14 @@ protected void fillInPartJars(List jars) { for (JARDesc desc : jars) { String part = desc.getPart(); - for (JARDesc jar : available) { - if (part != null && part.equals(jar.getPart())) - if (!jars.contains(jar)) - jars.add(jar); + // "available" field can be affected by two different threads + // working in loadClass(String) + synchronized (available) { + for (JARDesc jar : available) { + if (part != null && part.equals(jar.getPart())) + if (!jars.contains(jar)) + jars.add(jar); + } } } } @@ -1571,35 +1580,56 @@ * Find a JAR in the shared 'extension' classloaders, this * classloader, or one of the classloaders for the JNLP file's * extensions. + * This method used to be qualified "synchronized." This was done solely for the + * purpose of ensuring only one thread entered the method at a time. This was not + * strictly necessary - ensuring that all affected fields are thread-safe is + * sufficient. Locking on the JNLPClassLoader instance when this method is called + * can result in deadlock if another thread is dealing with the CodebaseClassLoader + * at the same time. This solution is very heavy-handed as the instance lock is not + * truly required, and taking the lock on the classloader instance when not needed is + * not in general a good idea because it can and will lead to deadlock when multithreaded + * classloading is in effect. The solution is to keep the fields thread safe on their own. + * This is accomplished by wrapping them in Collections.synchronized* to provide + * atomic add/remove operations, and synchronizing on them when iterating or performing + * multiple mutations. + * See bug report RH976833. On some systems this bug will manifest itself as deadlock on + * every webpage with more than one Java applet, potentially also causing the browser + * process to hang. + * More information in the mailing list archives: + * http://mail.openjdk.java.net/pipermail/distro-pkg-dev/2013-September/024536.html + * + * Affected fields: available, classpaths, jarIndexes, jarEntries, jarLocationSecurityMap */ public Class loadClass(String name) throws ClassNotFoundException { - synchronized (loadClassLock) { - Class result = findLoadedClassAll(name); + Class result = findLoadedClassAll(name); - // try parent classloader - if (result == null) { - try { - ClassLoader parent = getParent(); - if (parent == null) - parent = ClassLoader.getSystemClassLoader(); + // try parent classloader + if (result == null) { + try { + ClassLoader parent = getParent(); + if (parent == null) + parent = ClassLoader.getSystemClassLoader(); - return parent.loadClass(name); - } catch (ClassNotFoundException ex) { - } + return parent.loadClass(name); + } catch (ClassNotFoundException ex) { } + } - // filter out 'bad' package names like java, javax - // validPackage(name); + // filter out 'bad' package names like java, javax + // validPackage(name); - // search this and the extension loaders - if (result == null) { + // search this and the extension loaders + if (result == null) { + try { + result = loadClassExt(name); + } catch (ClassNotFoundException cnfe) { + // Not found in external loader either + + // Look in 'Class-Path' as specified in the manifest file try { - result = loadClassExt(name); - } catch (ClassNotFoundException cnfe) { - // Not found in external loader either - - // Look in 'Class-Path' as specified in the manifest file - try { + // This field synchronized before iterating over it since it may + // be shared data between threads + synchronized (classpaths) { for (String classpath: classpaths) { JARDesc desc; try { @@ -1610,19 +1640,22 @@ } addNewJar(desc); } - - result = loadClassExt(name); - return result; - } catch (ClassNotFoundException cnfe1) { - if (JNLPRuntime.isDebug()) { - cnfe1.printStackTrace(); - } } - // As a last resort, look in any available indexes + result = loadClassExt(name); + return result; + } catch (ClassNotFoundException cnfe1) { + if (JNLPRuntime.isDebug()) { + cnfe1.printStackTrace(); + } + } - // Currently this loads jars directly from the site. We cannot cache it because this - // call is initiated from within the applet, which does not have disk read/write permissions + // As a last resort, look in any available indexes + // Currently this loads jars directly from the site. We cannot cache it because this + // call is initiated from within the applet, which does not have disk read/write permissions + // This field synchronized before iterating over it since it may + // be shared data between threads + synchronized (jarIndexes) { for (JarIndex index : jarIndexes) { // Non-generic code in sun.misc.JarIndex @SuppressWarnings("unchecked") @@ -1652,13 +1685,13 @@ } } } + } - if (result == null) { - throw new ClassNotFoundException(name); - } + if (result == null) { + throw new ClassNotFoundException(name); + } - return result; - } + return result; } /** @@ -2030,21 +2063,23 @@ protected SecurityDesc getCodeSourceSecurity(URL source) { SecurityDesc sec=jarLocationSecurityMap.get(source); - if (sec == null && !alreadyTried.contains(source)) { - alreadyTried.add(source); - //try to load the jar which is requesting the permissions, but was NOT downloaded by standard way - if (JNLPRuntime.isDebug()) { - System.out.println("Application is trying to get permissions for " + source.toString() + ", which was not added by standard way. Trying to download and verify!"); - } - try { - JARDesc des = new JARDesc(source, null, null, false, false, false, false); - addNewJar(des); - sec = jarLocationSecurityMap.get(source); - } catch (Throwable t) { + synchronized (alreadyTried) { + if (sec == null && !alreadyTried.contains(source)) { + alreadyTried.add(source); + //try to load the jar which is requesting the permissions, but was NOT downloaded by standard way if (JNLPRuntime.isDebug()) { - t.printStackTrace(); + System.out.println("Application is trying to get permissions for " + source.toString() + ", which was not added by standard way. Trying to download and verify!"); } - sec = null; + try { + JARDesc des = new JARDesc(source, null, null, false, false, false, false); + addNewJar(des); + sec = jarLocationSecurityMap.get(source); + } catch (Throwable t) { + if (JNLPRuntime.isDebug()) { + t.printStackTrace(); + } + sec = null; + } } } if (sec == null){ @@ -2079,8 +2114,10 @@ addNativeDirectory(nativeDirectory); // security descriptors - for (URL key : extLoader.jarLocationSecurityMap.keySet()) { - jarLocationSecurityMap.put(key, extLoader.jarLocationSecurityMap.get(key)); + synchronized (jarLocationSecurityMap) { + for (URL key : extLoader.jarLocationSecurityMap.keySet()) { + jarLocationSecurityMap.put(key, extLoader.jarLocationSecurityMap.get(key)); + } } } @@ -2325,9 +2362,11 @@ } // Permissions for all remote hosting urls - for (URL u: jarLocationSecurityMap.keySet()) { - permissions.add(new SocketPermission(u.getHost(), - "connect, accept")); + synchronized (jarLocationSecurityMap) { + for (URL u: jarLocationSecurityMap.keySet()) { + permissions.add(new SocketPermission(u.getHost(), + "connect, accept")); + } } // Permissions for codebase urls (if there is a loader)