[icedtea-web] RFC: Fix for PR852: Classloader not being flushed after close
Deepak Bhole
dbhole at redhat.com
Fri Jan 27 12:29:36 PST 2012
* 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.
As for renaming decrement... to dispose, the loader is no always
disposed after the call. If multiple applets are opened, we only want
the count to be reduced, not the loader disposed, so I think the
existing name is more appropriate.
> >+ synchronized(loader.useCount) {
> >+ loader.useCount--;
> >+
> >+ if (loader.useCount<= 0) {
> >+ synchronized(urlToLoader) {
> >+ urlToLoader.remove(loader.file.getUniqueKey());
> >+ }
> >+ }
> >+ }
> >+ }
> >
>
> Synchronizing over Integer (or Long) has always looked scary to me.
> I am not sure if this is a problem in this case, but as soon as you
> do the post increment, the object (useCount) has changed. If another
> thread comes along at this point, it can now synchronize over the
> new object and it would be possible to have two threads enter the
> same synchronized block. I am attaching a program that shows that
> the Integer has indeed changed.
>
Doh! I hadn't even thought of that. You're right, it is unsafe to use
that. I will fix it to synchronize around the loader itself. It looks
like it should be safe, but please let me know if you prefer a lock
specific for the counter.
> >diff -r 41f03d932cdf plugin/icedteanp/java/sun/applet/PluginAppletViewer.java
> >--- a/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java Mon Jan 09 18:45:31 2012 -0500
> >+++ b/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java Thu Jan 26 13:15:59 2012 -0500
> >@@ -1625,6 +1625,9 @@
> >
> > appletShutdown(p);
> > appletPanels.removeElement(p);
> >+
> >+ // Mark classloader unusable
> >+ JNLPClassLoader.decrementLoaderCount((JNLPClassLoader) cl);
> >
> > try {
> > SwingUtilities.invokeAndWait(new Runnable() {
> >
>
> Not being an expert, I am not sure about the details of how this
> fixes the problem. If the _only_ problem was that we are maintaining
> a reference to the classloader, perhaps making urlToLoader a
> Map<String, WeakReference<JNLPClassLoader>> might be more
> appropriate? Or is there something I am missing?
>
> Sorry if this is not very helpful - I took a look at bugzilla but I
> could not find the actual issue described there.
>
Please see attached test case I created for the issue.
New patch with changes attached.
Thanks for reviewing this!
Cheers,
Deepak
-------------- next part --------------
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 15:26:47 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.decrementLoaderCount(); // 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.incrementLoaderCount();
+ 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
+ */
+ public synchronized void incrementLoaderCount() {
+
+ // 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 decrementLoaderCount() {
+
+ // 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 15:26:47 2012 -0500
@@ -1625,6 +1625,9 @@
appletShutdown(p);
appletPanels.removeElement(p);
+
+ // Mark classloader unusable
+ ((JNLPClassLoader) cl).decrementLoaderCount();
try {
SwingUtilities.invokeAndWait(new Runnable() {
-------------- next part --------------
A non-text attachment was scrubbed...
Name: PR852-test.zip
Type: application/zip
Size: 1438 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20120127/0be253e6/PR852-test.zip
More information about the distro-pkg-dev
mailing list