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

[Adam Domurad]

On 02/13/2013 05:04 AM, Jiri Vanek wrote:
> On 01/29/2013 10: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.
>
>
> I think ok for head. I have created an reproducer for this, so I think 
> you can push to head. imho worthy to 1.3 after some  tiime in head
>
> J.
>

Thanks, pushed.

How about this small change ? Occurred to me after pushing ...

2013-02-13  Adam Domurad  <adomurad at redhat.com>

     * netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java:
     Make uniqueKeyToLock a weak map so locks can be collected.

Note that weak hash map is less error prone than trying to remove the 
lock from map, because if the lock were removed this (really rare) case 
could occur:
- lock grabbed in one thread
- then removed from map in another
- grabbed lock used in original thread
- next thread will create its own lock

Weak hash maps avoid that possibility.

Happy hacking,
-Adam
-------------- next part --------------
A non-text attachment was scrubbed...
Name: make-weak.patch
Type: text/x-patch
Size: 1039 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20130213/41299a9a/make-weak.patch