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

Deepak Bhole dbhole at redhat.com
Wed Aug 8 13:25:58 PDT 2012


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

Cheers,
Deepak

> >
> >Cheers,
> >Deepak
> >
> 



More information about the distro-pkg-dev mailing list