[rfc][icedtea-web] RH976833 ClassLoader deadlock

[Andrew Azores]

On 09/05/13 07:42, Jiri Vanek wrote:
> On 08/15/2013 10:10 PM, Andrew Azores wrote:
>> Changelog:
>>
>> * netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java: (loadClass) 
>> made unsynchronized.
>> (available, jarIndexes, classpaths, jarEntries, 
>> jarLocationSecurityMap) wrapped in synchronized
>> classes.
>>
>> An earlier change to the classloader made it possible for two threads 
>> to enter deadlock. This is
>
> Can we have an reproducer for this?  But those are mostly very hard to 
> reproduce, so do not vaste to much time on it (no metter that 
> reprodcuer can have random behaviour as it "can deadlock")
>

I did spend some time previously trying to put together a reproducer, 
but I wasn't able to recreate the deadlock offline. The deadlock was 
affecting every webpage with multiple applets on it, however, at least 
on my system. The two sites I used as "reproducers" were:
[1] http://www.browserspy.dk/java.php
[2] 
http://www.cis.upenn.edu/~matuszek/General/JavaVersionTests/JavaTests.html
[1] being the site mentioned in the original bug report, and [2] being a 
site I randomly found by Googling for Java test applets. It has *9* on 
one page! Excellent for testing this particular scenario :)

>> resolved by not giving the thread which calls loadClass the lock on 
>> the JNLPClassLoader instance,
>> and instead synchronizing the member variables which are accessed in 
>> loadClass and other methods
>> called by loadClasss.
>>
>> Also made some for-loops into for-each-loops for readability.
>
>
> Sorry for being nitpicker, but please separate refactoring form fix. 
> Both looks good but the readability of patch is strongly reduced by 
> the mixture.
>
> Also I'm fan of "refactoring is good time to do an unittest"
> So the best would be for each method, you touch with  "for-loops into 
> fpor-each-loops " refactoring is an excelent candidate for unittest. 
> When I wrote this to Adam an year ago, he stopepd todo this :) So I do 
> not wont to force you to do, but please think about it.

Sure I can separate them out.

Unittesting on the refactors might be awkward because the methods 
containing the loop refactors are things like 
JNLPClassLoader#initializePermissions(), which takes no parameters and 
has no return value. It just adds values into a private list, which 
doesn't seem to be publicly exposed in any way either. I'm not really 
sure how I can go about testing the refactor of the for-loop into a 
for-each-loop in that kind of situation without doing even more 
refactoring and changing the interface of the class, just to support 
being able to unittest a much simpler refactor. TBH I'd rather just 
leave the slightly-messier looking for-loops in place in this case. If I 
had done a more thorough refactor with methods being split/extracted or 
anything like that then unit testing would seem more feasible.

Attached is the fix-only version of the patch, refactoring all removed 
for the time being.

>
> (snip)


-- 
Andrew A

-------------- next part --------------
A non-text attachment was scrubbed...
Name: fix.patch
Type: text/x-patch
Size: 10204 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20130905/1741b788/fix.patch