[icedtea-web] RFC: Fix for PR852: Classloader not being flushed after close

[Deepak Bhole]

* Deepak Bhole <dbhole at redhat.com> [2012-01-27 16:15]:
> * Omair Majid <omajid at redhat.com> [2012-01-27 15:56]:
> > On 01/27/2012 03:29 PM, Deepak Bhole wrote:
> > >* Omair Majid<omajid at redhat.com>  [2012-01-26 21:55]:
> > >>Hi Deepak,
> > >>
> > >>On 01/26/2012 01:18 PM, Deepak Bhole wrote:
> > >>>This patch fixes PR852. I would like to put it in 1.2 in addition to
> > >>>HEAD.
> > >>
> > >>I have a few on the patch comments below.
> > >>
> > >>>diff -r 41f03d932cdf netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
> > >>>--- a/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java	Mon Jan 09 18:45:31 2012 -0500
> > >>>+++ b/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java	Thu Jan 26 13:15:59 2012 -0500
> > >>
> > >>>+    public static void incrementLoaderCount(JNLPClassLoader loader) {
> > >>
> > >>>+    public static void decrementLoaderCount(JNLPClassLoader loader) {
> > >>
> > >>Just a thought, but it might be cleaner to make these instance
> > >>methods, since they always operate on a loader object. With that
> > >>changes, this becomes:
> > >>
> > >>public void incrementLoaderCount()
> > >>public void decrementLoaderCount()
> > >>
> > >>In fact, incrementLoaderCount does not even have to be public. If we
> > >>do go down that route, perhaps we can also go ahead and rename
> > >>decrementLoaderCount to dispose() or release()? (I am also thinking
> > >>close() but that would clash with the URLClassLoader's close()
> > >>method in OpenJDK7.) I think it makes the purpose clearer in that
> > >>this method releases resources associated with the classloader. It
> > >>also hides the fact that we are caching something from the users of
> > >>the JNLPClassLaoder api.
> > >>
> > >
> > >Sure. I think making it non-static should be perfectly fine.
> > >
> > 
> > What about private? ;)
> > 
> 
> Oops. New patch attached. I also renamed the methods to make them more
> clearer ([increment|decrement]LoaderCount ->
> [increment|decrement]LoaderUseCount)
> 

Gah. Sent too early. Patch attached this time.

> Okay for 1.2 and HEAD?
> 
> Cheers,
> Deepak
-------------- next part --------------
diff -r b901442e9ba4 NEWS
--- a/NEWS	Wed Jan 25 16:42:27 2012 +0100
+++ b/NEWS	Fri Jan 27 16:13:45 2012 -0500
@@ -22,6 +22,7 @@
   - PR749: sun.applet.PluginStreamHandler#handleMessage(String) really slow
   - PR782: Support building against npapi-sdk as well
   - PR838: IcedTea plugin crashes with chrome browser when javascript is executed
+  - PR852: Classloader not being flushed after last applet from a site is closed
   - RH586194: Unable to connect to connect with Juniper VPN client
   - RH718693: MindTerm SSH Applet doesn't work
 Common
diff -r b901442e9ba4 netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
--- a/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java	Wed Jan 25 16:42:27 2012 +0100
+++ b/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java	Fri Jan 27 16:13:45 2012 -0500
@@ -173,6 +173,11 @@
     private boolean foundMainJar= false;
 
     /**
+     * Variable to track how many times this loader is in use
+     */
+    private int useCount = 0;
+
+    /**
      * Create a new JNLPClassLoader from the specified file.
      *
      * @param file the JNLP file
@@ -321,6 +326,7 @@
                             throw new LaunchException(file, null, R("LSFatal"), R("LCClient"), R("LSignedAppJarUsingUnsignedJar"), R("LSignedAppJarUsingUnsignedJarInfo"));
 
                     loader.merge(extLoader);
+                    extLoader.decrementLoaderUseCount(); // loader urls have been merged, ext loader is no longer used
                 }
 
                 // loader is now current + ext. But we also need to think of
@@ -347,7 +353,11 @@
 
         // loaders are mapped to a unique key. Only extensions and parent
         // share a key, so it is safe to always share based on it
-        urlToLoader.put(uniqueKey, loader);
+        
+        loader.incrementLoaderUseCount();
+        synchronized(urlToLoader) {
+            urlToLoader.put(uniqueKey, loader);
+        }
 
         return loader;
     }
@@ -1762,6 +1772,42 @@
         }
         return result;
     }
+    
+    /**
+     * Increments loader use count by 1
+     * 
+     * @throws SecurityException if caller is not trusted
+     */
+    private synchronized void incrementLoaderUseCount() {
+        
+        // For use by trusted code only
+        if (System.getSecurityManager() != null)
+            System.getSecurityManager().checkPermission(new AllPermission());
+        
+        useCount++;
+    }
+
+    /**
+     * Decrements loader use count by 1
+     * 
+     * If count reaches 0, loader is removed from list of available loaders
+     * 
+     * @throws SecurityException if caller is not trusted
+     */
+    public synchronized void decrementLoaderUseCount() {
+
+        // For use by trusted code only
+        if (System.getSecurityManager() != null)
+            System.getSecurityManager().checkPermission(new AllPermission());
+
+        useCount--;
+
+        if (useCount <= 0) {
+            synchronized(urlToLoader) {
+                urlToLoader.remove(file.getUniqueKey());
+            }
+        }
+    }
 
     /*
      * Helper class to expose protected URLClassLoader methods.
diff -r b901442e9ba4 plugin/icedteanp/java/sun/applet/PluginAppletViewer.java
--- a/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java	Wed Jan 25 16:42:27 2012 +0100
+++ b/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java	Fri Jan 27 16:13:45 2012 -0500
@@ -1625,6 +1625,9 @@
 
                 appletShutdown(p);
                 appletPanels.removeElement(p);
+                
+                // Mark classloader unusable
+                ((JNLPClassLoader) cl).decrementLoaderUseCount();
 
                 try {
                     SwingUtilities.invokeAndWait(new Runnable() {