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

[Omair Majid]

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.

Cheers,
Omair