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

[Adam Domurad]

On 01/30/2013 11:01 AM, 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

Ping? Thoughts?
-Adam