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

[Deepak Bhole]

* Omair Majid <omajid at redhat.com> [2011-03-04 17:25]:
> On 03/04/2011 05:24 PM, Deepak Bhole wrote:
> >* 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.
> >
> 
> Ah. Things will certainly become interesting when we try to handle those.
> 
> >>>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).
> >
> 
> Thanks for the explanation. Patch is okay for HEAD.
> 

Great, thanks for reviewing!

Cheers,
Deepak

> Cheers,
> Omair