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

Deepak Bhole dbhole at redhat.com
Thu Aug 9 06:29:00 PDT 2012


* 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.

Cheers,
Deepak

> 
> J.
> 



More information about the distro-pkg-dev mailing list