[rfc][icedtea-web] fixing failing unittests CodeBaseClassLoaderTest.java

[Jiri Vanek]

On 08/09/2012 03:29 PM, Deepak Bhole wrote:
> * Jiri Vanek <jvanek at redhat.com> [2012-08-09 07:30]:
>> On 08/08/2012 10:25 PM, Deepak Bhole wrote:
>>> * Jiri Vanek <jvanek at redhat.com> [2012-08-08 08:35]:
>>>> On 08/07/2012 09:39 PM, Deepak Bhole wrote:
>>>>> * Jiri Vanek <jvanek at redhat.com> [2012-08-07 12:06]:
>>>>>> On 08/07/2012 05:53 PM, Deepak Bhole wrote:
>>>>>>> * Jiri Vanek <jvanek at redhat.com> [2012-08-01 14:29]:
>>>>>>>>
>>>>>>>> Those three tests have started to fail with NPE. I'm not sure if test is just badly written or
>>>>>>>> IcedTea web is broken (but as there are no regressions in reproducers then I'm turning to badly
>>>>>>>> written  test)
>>>>>>>>
>>>>>>>> Please note once more, I'm not sure if fix this test this way is correct, or we have really got
>>>>>>>> regression in CodeBaseClassLoader
>>>>>>>>
>>>>>>>> But IMHO we should not have jnlp file null for anything except pure applet.
>>>>>>>> If this is correct, then this null should be probably thrown from
>>>>>>>>    new SecurityDesc(HERE, SecurityDesc.SANDBOX_PERMISSIONS, null) this contcructor (after check for
>>>>>>>> javaws/applet) ??
>>>>>>>>
>>>>>>>
>>>>>>> Hi Jiri,
>>>>>>>
>>>>>>> I am not sure I fully understand. The test tries to find a class in the
>>>>>>> codebase classloader and then asserts that the exception is an instance
>>>>>>> of NPE?  Shouldn't it be an instance of CNFE when not found?
>>>>>>>
>>>>>>> findClass should never throw NPE IMO.
>>>>>>
>>>>>> The crucial difference in test is this:
>>>>>> -                return new SecurityDesc(null, SecurityDesc.SANDBOX_PERMISSIONS, null);
>>>>>> ---->>                                   -|-
>>>>>> +                return new SecurityDesc(this, SecurityDesc.SANDBOX_PERMISSIONS, null);
>>>>>>
>>>>>> The mock JNLPfile used in this class to test classlaoder is creating
>>>>>> the security description with null jnlp file. Imho it is not
>>>>>> possible in "real life".
>>>>>>
>>>>>> So I have replaced null by (imho correct) itself.
>>>>>>
>>>>>
>>>>> I am referring to this part:
>>>>
>>>> Ah, sorry.
>>>>>
>>>>>>>> +
>>>>>>>> +        Exception ex2=null;
>>>>>>>> +        try{
>>>>>>>> +        classLoader.findClass("foo");
>>>>>>>> +        }catch (Exception exx){
>>>>>>>> +            ex2=exx;
>>>>>>>> +        }
>>>>>>>> +        Assert.assertNotNull(ex2);
>>>>>>>> +        Assert.assertTrue(ex2 instanceof NullPointerException);
>>>>>>>> +
>>>>>>>> +        Exception ex=null;
>>>>>>>> +        try{
>>>>>>>> +        classLoader.findResource("net/sourceforge/jnlp/about/Main.class");
>>>>>>>> +        }catch (Exception exx){
>>>>>>>> +            ex=exx;
>>>>>>>> +        }
>>>>>>>> +        Assert.assertNotNull(ex);
>>>>>>>> +        Assert.assertTrue(ex instanceof NullPointerException);
>>>>>>>> +
>>>>>
>>>>> Why is an NPE expected and asserted when findClass should never be
>>>>> throwing an NPE?
>>>>
>>>> Because security manager is asked, and he is taking SecurityDesc
>>> >from JNLPfile. But it is null in this test.
>>>>
>>>> If you will ask me to prevent this (and so add if
>>>> securityDesc.getJnlpFile()==null){...) Then I must admit I'm not
>>>> sure what behaviour should be allowed - as jnlp file is quite
>>>> crucial for this.
>>>>
>>>> Does it sounds better now?
>>>
>>> If findClass() runs into any exception, it should also convert it to a
>>> CNFE and throw that IMO. Callers should not have to worry about
>>> implementation specific issues when dealing with a classloader.
>>>
>> I see your point now.
>>
>> Will
>>   +        Assert.assertNotNull(ex);
>>   +        Assert.assertTrue(ex instanceof RuntimeException);
>>
>> or jusr
>>>>>>>> +        Assert.assertNotNull(ex);
>>>>>>>> +        //Assert.assertTrue(ex instanceof NullPointerException);
>>
>> Satisfy you?
>>
>
> Close :) I think this will be better:
> Assert.assertNotNull(ex);
> Assert.assertTrue(ex instanceof ClassNotFoundException);
>
> And we fix JNLPCLassLoader to throw the proper exception first and then
> commit the test.
>
>>
>> I will look into catch of exception later. But I'm not sure whether
>> catch of NPE and rethrow as CNFE is an good idea. The cause is not
>> "not found class" but the null jnlp file in security description.
>> Wrapped as CNFE can be misleading.
>>
>
>  From the callers perspective, they don't know anything about JNLP, they
> only know that it is a classloader and the class is either found or
> not.. the caller should never have to worry about a RuntimeException
> generally (i.e. for all cases where we know there is an RTE, we should
> throw a CNFE). Depending on the code, the caller may be able to work
> around a CFNE (a class may not be required) and throwing an NPE in such
> a case would unnecessarily crash the caller.
>

Very well :).

What do you think now?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: codeBaseLoaderTest_2.patch
Type: text/x-patch
Size: 15436 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20120810/bf62e91c/codeBaseLoaderTest_2.patch 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: codeBaseLoaderFix_2.patch
Type: text/x-patch
Size: 3911 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20120810/bf62e91c/codeBaseLoaderFix_2.patch