[icedtea-web] RFC: Patch to support plugin classloader sharing
Omair Majid
omajid at redhat.com
Fri Mar 4 13:56:54 PST 2011
On 03/04/2011 03:55 PM, Deepak Bhole wrote:
> * Dr Andrew John Hughes<ahughes at redhat.com> [2011-03-03 07:41]:
>> > On 21:08 Wed 02 Mar , Deepak Bhole wrote:
>>>>>> > > > > >
> <snip>
>>>>> > > > >
>>>>> > > > > This is horrible. Not only is it going to break if the implementation details of
>>>>> > > > > URLClassLoader change (have you checked OpenJDK7?) but it will fail with any
>>>>> > > > > different implementation of URLClassLoader. Can you not override a method or
>>>>> > > > > use our own classloader instead of this awful hack?
>>>>> > > > >
>>>> > > >
>>>> > > > Yep, I agree 100% .. (well 99.9 :)) that it is a terrible hack.
>>>> > > >
> <snip>
>>>> > > >
>>> > >
>>> > > Hold the reply please. I just thought of another solution that might
>>> > > work, but it is more disruptive (from code perspective). I will
>>> > > implement it and post it tomorrow and we can discuss which one to adopt
>>> > > on the list.
>>> > >
>> >
>> > Ok. I'd pretty much prefer most things to this solution.
>> >
> Fair enough:) I realized after sending the original patch that the
> ideas of the new design could be implemented (at a higher level) into
> the existing one. So here is a patch that does not use any reflection,
> nor does it add any additional sun.misc dependencies.
>
> Though this may appear like a huge change, I have tried hard to keep it
> very simple, and have tested it as much as possible. I think it is safe
> for 1.0 as well as HEAD though I will forgo 1.0 if the reviewer thinks
> it is unsafe.
>
Just to summarize (and to make sure I understand this): the key here is
to create another ClassLoader. This ClassLoader only contains the
codebase (can there be more than one?) and is only used as a last
resort. This avoids contacting the server needlessly.
> ChangeLog:
> 2011-03-04 Deepak Bhole<dbhole at redhat.com>
>
> * NEWS: Updated.
> * netx/net/sourceforge/jnlp/PluginBridge.java (PluginBridge): Use
> documentbase as a uniquekey so that the classloader may be shared by
> applets from the same page.
> * netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java: Added new
> CodeBaseClassLoader class to load codebase (from path instead of a file)
> classes.
> (getInstance): Try to match file locations only for Web Start apps. For
> plugin, merge the new loader into current one.
> (enableCodeBase): Use the new addToCodeBaseLoader method.
> (findLoadedClassAll): Search the codebase loader if the class was not
> found in the file loaders.
> (findClass): Likewise.
> (getResource): Likewise.
> (findResources): Likewise.
> (merge): Merge codebase loaders.
> (addToCodeBaseLoader): New method. Adds a given url to the codebase loader
> if it is a path.
> (CodeBaseClassLoader): New inner class. Extends URLClassLoader to expose
> its protected methods like addURL.
> * netx/net/sourceforge/jnlp/runtime/JNLPSecurityManager.java
> (getApplication): Accomodate the fact that the classloader for a class may
> be a CodeBaseClassLoader.
> * plugin/icedteanp/java/sun/applet/PluginAppletViewer.java (run):
> Likewise.
>
> Cheers,
> Deepak
>
>
> plugin-shared-cl-2.patch
>
>
> diff -r 64b9d3a8239c NEWS
> --- a/NEWS Thu Mar 03 17:56:00 2011 -0500
> +++ b/NEWS Fri Mar 04 15:46:36 2011 -0500
> @@ -21,6 +21,7 @@
> - Use Firefox's proxy settings if possible
> - RH669942: javaws fails to download version/packed files (missing support for jnlp.packEnabled and jnlp.versionEnabled)
> * Plugin
> + - PR475, RH604061: Allow applets from the same page to use the same classloader
> - PR612: NetDania application ends on java.security.AccessControlException: access denied (java.util.PropertyPermission browser read)
>
> New in release 1.0 (2010-XX-XX):
> diff -r 64b9d3a8239c netx/net/sourceforge/jnlp/PluginBridge.java
> --- a/netx/net/sourceforge/jnlp/PluginBridge.java Thu Mar 03 17:56:00 2011 -0500
> +++ b/netx/net/sourceforge/jnlp/PluginBridge.java Fri Mar 04 15:46:36 2011 -0500
> @@ -130,9 +130,10 @@
> else
> security = null;
>
> - this.uniqueKey = Calendar.getInstance().getTimeInMillis() + "-" +
> - Math.abs(((new java.util.Random()).nextInt())) + "-" +
> - documentBase;
> + // Plugin needs to share classloaders so that applet instances from
> + // same page can communicate (there are applets known to require
> + // such communication for proper functionality)
> + this.uniqueKey = documentBase.toString();
> }
>
> public String getTitle() {
> diff -r 64b9d3a8239c netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
> --- a/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java Thu Mar 03 17:56:00 2011 -0500
> +++ b/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java Fri Mar 04 15:46:36 2011 -0500
> @@ -145,6 +145,9 @@
> /** Map of specific codesources to securitydesc */
> private HashMap<URL, SecurityDesc> jarLocationSecurityMap =
> new HashMap<URL, SecurityDesc>();
> +
> + /** Loader for codebase (which is a path, rather than a file) */
> + private CodeBaseClassLoader codeBaseLoader;
>
> /**
> * Create a new JNLPClassLoader from the specified file.
> @@ -276,10 +279,12 @@
>
> try {
>
> - // If base loader is null, or the baseloader's file and this
> - // file is different, initialize a new loader
> +
> + // A null baseloader implies that no loader has been created
> + // for this codebase/jnlp yet. Create one.
> if (baseLoader == null ||
> - !baseLoader.getJNLPFile().getFileLocation().equals(file.getFileLocation())) {
> + (file.isApplication()&&
> + !baseLoader.getJNLPFile().getFileLocation().equals(file.getFileLocation()))) {
>
> loader = new JNLPClassLoader(file, policy);
>
> @@ -303,6 +308,13 @@
>
> } else {
> // if key is same and locations match, this is the loader we want
> + if (!file.isApplication()) {
> + // If this is an applet, we do need to consider its loader
> + loader = new JNLPClassLoader(file, policy);
> +
> + if (baseLoader != null)
> + baseLoader.merge(loader);
> + }
> loader = baseLoader;
> }
>
This logic is getting quite convoluted. What's the purpose of this hunk
(and the one before this)?
> @@ -529,7 +541,7 @@
> * loaded from the codebase are not cached.
> */
> public void enableCodeBase() {
> - addURL(file.getCodeBase()); // nothing happens if called more that once?
> + addToCodeBaseLoader(file.getCodeBase());
> }
>
> /**
> @@ -958,8 +970,13 @@
> if (result != null)
> return result;
> }
> -
> - return null;
> +
> + // Result is still null. Return what the codebaseloader
> + // has (which returns null if it is not loaded there either)
> + if (codeBaseLoader != null)
> + return codeBaseLoader.findLoadedClassFromParent(name);
> + else
> + return null;
> }
>
> /**
> @@ -1067,6 +1084,11 @@
> }
> }
>
> + // Try codebase loader
> + if (codeBaseLoader != null)
> + return codeBaseLoader.findClass(name);
> +
> + // All else failed. Throw CNFE
> throw new ClassNotFoundException(name);
> }
>
> @@ -1109,6 +1131,10 @@
> for (int i = 1; i< loaders.length; i++)
> if (result == null)
> result = loaders[i].getResource(name);
> +
> + // If result is still null, look in the codebase loader
> + if (result == null&& codeBaseLoader != null)
> + result = codeBaseLoader.getResource(name);
>
> return result;
> }
> @@ -1120,9 +1146,9 @@
> @Override
> public Enumeration<URL> findResources(String name) throws IOException {
> Vector<URL> resources = new Vector<URL>();
> + Enumeration<URL> e;
>
> for (int i = 0; i< loaders.length; i++) {
> - Enumeration<URL> e;
>
> if (loaders[i] == this)
> e = super.findResources(name);
> @@ -1133,6 +1159,14 @@
> resources.add(e.nextElement());
> }
>
> + // Add resources from codebase (only if nothing was found above,
> + // otherwise the server will get hammered)
> + if (resources.isEmpty()&& codeBaseLoader != null) {
> + e = codeBaseLoader.findResources(name);
> + while (e.hasMoreElements())
> + resources.add(e.nextElement());
> + }
> +
> return resources.elements();
> }
>
> @@ -1250,6 +1284,9 @@
> // jars
> for (URL u : extLoader.getURLs())
> addURL(u);
> +
> + // Codebase
> + addToCodeBaseLoader(extLoader.file.getCodeBase());
>
> // native search paths
> for (File nativeDirectory : extLoader.getNativeDirectories())
> @@ -1261,6 +1298,28 @@
> }
> }
>
> + /**
> + * Adds the given path to the path loader
> + *
> + * @param URL the path to add
> + * @throws IllegalArgumentException If the given url is not a path
> + */
> + private void addToCodeBaseLoader(URL u) {
> +
> + // Only paths may be added
> + if (!u.getFile().endsWith("/")) {
> + throw new IllegalArgumentException("addToPathLoader only accepts path based URLs");
> + }
> +
> + // If there is no loader yet, create one, else add it to the
> + // existing one (happens when called from merge())
> + if (codeBaseLoader == null) {
> + codeBaseLoader = new CodeBaseClassLoader(new URL[] { u }, this);
> + } else {
> + codeBaseLoader.addURL(u);
> + }
> + }
> +
> private DownloadOptions getDownloadOptionsForJar(JARDesc jar) {
> boolean usePack = false;
> boolean useVersion = false;
> @@ -1283,4 +1342,49 @@
> return new DownloadOptions(usePack, useVersion);
> }
>
> + /*
> + * Helper class to expose protected URLClassLoader methods.
> + */
> +
> + public class CodeBaseClassLoader extends URLClassLoader {
> +
> + JNLPClassLoader parentJNLPClassLoader;
> +
> + public CodeBaseClassLoader(URL[] urls, JNLPClassLoader cl) {
> + super(urls);
> + parentJNLPClassLoader = cl;
> + }
> +
> + @Override
> + public void addURL(URL url) {
> + super.addURL(url);
> + }
> +
> + @Override
> + public Class<?> findClass(String name) throws ClassNotFoundException {
> + return super.findClass(name);
> + }
> +
> + /**
> + * Returns the output of super.findLoadedClass().
> + *
> + * The method is renamed because ClassLoader.findLoadedClass() is final
> + *
> + * @param name The name of the class to find
> + * @return Output of ClassLoader.findLoadedClass() which is the class if found, null otherwise
> + * @see java.lang.ClassLoader#findLoadedClass(String)
> + */
> + public Class<?> findLoadedClassFromParent(String name) {
> + return findLoadedClass(name);
> + }
> +
> + /**
> + * Returns JNLPClassLoader that encompasses this loader
> + *
> + * @return parent JNLPClassLoader
> + */
> + public JNLPClassLoader getParentJNLPClassLoader() {
> + return parentJNLPClassLoader;
> + }
> + }
Much better than those crazy reflection calls :)
> }
> diff -r 64b9d3a8239c netx/net/sourceforge/jnlp/runtime/JNLPSecurityManager.java
> --- a/netx/net/sourceforge/jnlp/runtime/JNLPSecurityManager.java Thu Mar 03 17:56:00 2011 -0500
> +++ b/netx/net/sourceforge/jnlp/runtime/JNLPSecurityManager.java Fri Mar 04 15:46:36 2011 -0500
> @@ -199,8 +199,16 @@
>
> // this needs to be tightened up
> for (int i = 0; i< stack.length&& i< maxDepth; i++) {
> - if (stack[i].getClassLoader() instanceof JNLPClassLoader) {
> - JNLPClassLoader loader = (JNLPClassLoader) stack[i].getClassLoader();
> + ClassLoader cl = stack[i].getClassLoader();
> +
> + // Since we want to deal with JNLPClassLoader, extract it if this
> + // is a codebase loader
> + if (cl instanceof JNLPClassLoader.CodeBaseClassLoader)
> + cl = ((JNLPClassLoader.CodeBaseClassLoader) cl).getParentJNLPClassLoader();
> +
> + if (cl instanceof JNLPClassLoader) {
> +
> + JNLPClassLoader loader = (JNLPClassLoader) cl;
>
> if (loader != null&& loader.getApplication() != null) {
> return loader.getApplication();
> diff -r 64b9d3a8239c plugin/icedteanp/java/sun/applet/PluginAppletViewer.java
> --- a/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java Thu Mar 03 17:56:00 2011 -0500
> +++ b/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java Fri Mar 04 15:46:36 2011 -0500
> @@ -1646,7 +1646,14 @@
> {
> public void run()
> {
> - ThreadGroup tg = ((JNLPClassLoader) p.applet.getClass().getClassLoader()).getApplication().getThreadGroup();
> + ClassLoader cl = p.applet.getClass().getClassLoader();
> +
> + // Since we want to deal with JNLPClassLoader, extract it if this
> + // is a codebase loader
> + if (cl instanceof JNLPClassLoader.CodeBaseClassLoader)
> + cl = ((JNLPClassLoader.CodeBaseClassLoader) cl).getParentJNLPClassLoader();
> +
> + ThreadGroup tg = ((JNLPClassLoader) cl).getApplication().getThreadGroup();
>
> appletShutdown(p);
> appletPanels.removeElement(p);
Other than the change in JNLPClassLoader.getInstance, this looks fine.
Cheers,
Omair
More information about the distro-pkg-dev
mailing list