[icedtea-web] RFC: Patch to support plugin classloader sharing

[Deepak Bhole]

* Omair Majid <omajid at redhat.com> [2011-03-04 17:16]:
> On 03/04/2011 05:09 PM, Deepak Bhole wrote:
> >* Omair Majid<omajid at redhat.com>  [2011-03-04 16:56]:
> >>On 03/04/2011 03:55 PM, Deepak Bhole wrote:
> ><snip>
> >>>
> >>
> >>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.
> >>
> >
> >Yep, that is absolutely correct!
> >
> >>>ChangeLog:
> >>>2011-03-04  Deepak Bhole<dbhole at redhat.com>
> >>>
> ><snip>
> >>>+                        baseLoader.merge(loader);
> >>>+                }
> >>>                  loader = baseLoader;
> >>>              }
> >>>
> >>
> >>This logic is getting quite convoluted. What's the purpose of this
> >>hunk (and the one before this)?
> >>
> >
> >Yeah I am not a big fan of it either, but the else was necessary.
> >
> >The original if says:
> >
> ><If there is no loader or the loader with the same key as this one is for
> >another JNLP file>:
> >
> >1. Create a new loader (CL)
> >2. Creation of CL may create additional extension loaders (jnlps can have
> >exts), so if any were created, ccumulate those, and merge it all into one.
> >
> >The above logic is only valid for JNLPs, as plugins have no concept of
> >extension loaders. Even for applets from the same page, creation of the
> >initial loader does no trigger any additional loader creation. So I
> >added an else logic which says:
> >
> 
> Hm... I wonder how (or if, even) the code handles plugins using JNLP
> extensions. AFAIK that's how JavaFX applets (do those exist any
> more? :P) run.
> 

I would be very surprised if the plugin could handle JNLP extensions..
AFAIK we dont support it yet.

> >Else, if loader for given key (page) is not null:
> >1. Create a new loader for this applet
> >2. Merge contents of new loader into existing loader that exists for this
> >page
> >
> 
> Was there an actual problem being caused by this? If the existing
> code working, then why are we changing it now?
>

The else condition didn't exist before since there was no plugin
classloader sharing. The patch I posted only adds that condition .. as
for the change to the if condition by the patch, that is to ensure that
that block is not entered for the plugin (before, since the uniqueKey
for each plugin instance was unique, it would have never been entered.
Now with key = documentBase, it qualifies to enter with the old if
condition).

Cheers,
Deepak

> >
> >I think the "or the loader with the same key..." condition can probably be
> >removed, but it is outside the scope of this patch. I will take a look
> >at it later.
> >
> 
> Good idea.
> 
> >>>@@ -529,7 +541,7 @@
> >>>       * loaded from the codebase are not cached.
> >>>       */
> >>>      public void enableCodeBase() {
> ><snip>
> >>>+        }
> >>>+    }
> >>
> >>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
> >
> ><snip>
> >>>+++ b/netx/net/sourceforge/jnlp/runtime/JNLPSecurityManager.java	Fri Mar 04 15:46:36 2011 -0500
> >>>@@ -199,8 +199,16 @@
> >>>                   appletPanels.removeElement(p);
> >>
> >>Other than the change in JNLPClassLoader.getInstance, this looks fine.
> >>
> >
> >Great, thanks for reviewing this so quickly!
> >
> 
> No problem.
> 
> Cheers,
> Omair