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

[Omair Majid]

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.

> 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?

>
> 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