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

[Adam Domurad]

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