[icedtea-web][rfc] Fix for PR580: http://www.horaoficial.cl/ loads improperly

[Jiri Vanek]

On 01/30/2013 05:01 PM, Adam Domurad wrote:
> On 01/29/2013 04:42 PM, Adam Domurad wrote:
>> Hi all -- so this was a fix that I was working on for the unsigned applet confirmation pop-up
>> dialogue (similar to Oracle's security levels).
>>
>> Turns out this applet is a pretty a good stress test. Basically it causes 13 createApplet calls,
>> which is a good way to reveal concurrency issues in getInstance.
>>
>> Prior to patch:
>>  - Some or all of the 3 times that show up never finish loading, except after refresh
>>  After patch:
>> - All times show without needing to refresh, as expected
>>
>> So why did it work mysteriously after refreshing ? After refresh, the shared classloader is still
>> around (although it -should- be cleared! thats another bug!), and on the second round of loading
>> all applets latch on to the correct classloader.
>>
>> There was some basic synchronization on urlToLoader, but not enough. It needs to synchronize all
>> instances of the unique key for the entire duration of the get/put.
>>
>> The first approach I took was to simply make JNLPClassLoader#getInstance 'synchronized' -- however
>> parallel loading of different-unique-key applets would suffer. While same-unique-key applets must
>> load sequentially for correct behaviour, a long-loading applet could make a short-loading applet
>> wait a long time if getInstance was simply synchronized.
>>
>> JNLPClassLoader continues to get more complicated, unfortunately ... but this fix is needed.
>> I have also removed a try ... catch that did nothing but re-threw the exception -- although I know
>> this is unrelated, it allowed the diff to be cleaner :-) Feel free to force it back ...
>>
>> Fix changelog:
>> 2013-XX-XX  Adam Domurad  <adomurad at redhat.com>
>>
>>     Fix PR580: http://www.horaoficial.cl/ loads improperly. Applets that
>>     must share a class-loader now load sequentially.
>>     * NEWS:
>>     Mention the fix.
>>     * netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
>>     (getUniqueKeyLock): New, atomically grabs or creates a lock for the
>>     unique key.
>>     (getInstance): Ensure classloader initialization is locked by unique
>>     key.
>>     (decrementLoaderUseCount): Ensure classloader deinitialization is
>>     locked by unique key, get rid of no-longer used locks.
>
>
> Sorry originally I had:
>
> -    private static Map<String, JNLPClassLoader> urlToLoader =
> -            new HashMap<String, JNLPClassLoader>(); // never garbage collected!
> +    /** map from JNLPFile unique key to shared classloader */
> +    private static Map<String, JNLPClassLoader> uniqueKeyToLoader = new ConcurrentHashMap<String,
> JNLPClassLoader>();
>
> the ConcurrentHashMap is needed for correct behaviour. It got lost in patch reworking. Please
> consider patch as if it had it.
>
> -Adam

ps:  do not forget this chunk!


J.