[icedtea-web] RFC: find jars based on Class-Path entry in manifest

[Omair Majid]

Updated patch attached.

On 04/14/2011 08:50 PM, Dr Andrew John Hughes wrote:
> On 19:09 Thu 14 Apr     , Omair Majid wrote:
>
> snip...
>
>> >  Each String in the Map is a classpath entry. The '+' is used to join the
>> >  jar's directory with the actual jar name. As an example, consider the
>> >  applet linked in the original email. The applet's jars are located at
>> >  /applet/jar/*. The 'Class-Path' entries contain just the names of the
>> >  jar - like foo.jar, bar.jar. The page (and hence the codebase) is
>> >  /applet/. If we just joined the name of the jar with the codebase, the
>> >  result is /applet/foo.jar, which is not the right path. This bit of code
>> >  above joins the original jar's path to the Class-Path jars to create
>> >  paths like /applet/jar/foo.jar and /applet/jar/bar.jar.
>> >
>> >  On second thought, perhaps I should have documented this a little more.
>> >  I thought my comment:
>> >  // we want to search for jars in the same subdir on the server
>> >  // as this jar, so find out that subdirectory
>> >  was sufficient, but it may not be.
>> >
> My main issue was that I wasn't familiar with this jar referencing syntax, but
> if you can make the documentation any clearer, that's always good.
>

Do the comments in getClassPathsFromManifest help?

>>>>> >  >>  >                                  if (index != null)
>>>>> >  >>  >                                      jarIndexes.add(index);
>>>>> >  >>  >                              }
>>>>> >  >>  >    @@ -1007,8 +1033,32 @@
>>>>> >  >>  >                  try {
>>>>> >  >>  >                      result = loadClassExt(name);
>>>>> >  >>  >                  } catch (ClassNotFoundException cnfe) {
>>>>> >  >>  >    +                // Not found in external loader either
>>>>> >  >>  >
>>>>> >  >>  >    -                // Not found in external loader either. As a last resort, look in any available indexes
>>>>> >  >>  >    +                // Look in 'Class-Path' as specified in the manifest file
>>>>> >  >>  >    +                try {
>>>>> >  >>  >    +                    for (String classpath: classpathsInManfest) {
>>>>> >  >>  >    +                        JARDesc desc;
>>>>> >  >>  >    +                        try {
>>>>> >  >>  >    +                            URL jarUrl = new URL(file.getCodeBase(), classpath);
>>>>> >  >>  >    +                            desc = new JARDesc(jarUrl, null, null, false, true, false, true);
>>>>> >  >>  >    +                        } catch (MalformedURLException mfe) {
>>>>> >  >>  >    +                            throw new ClassNotFoundException(name);
>>>>> >  >>  >    +                        }
>>>>> >  >>  >    +                        try {
>>>>> >  >>  >    +                            addNewJar(desc);
>>>>> >  >>  >    +                        } catch (Exception e) {
>>>>> >  >>  >    +                            throw new ClassNotFoundException(name);
>>>>> >  >>  >    +                        }
>>>>> >  >>  >    +                    }
>>>>> >  >>  >    +
>>>>> >  >>  >    +                    result = loadClassExt(name);
>>>>> >  >>  >    +                    return result;
>>>>> >  >>  >    +                } catch (ClassNotFoundException cnfe1) {
>>>>> >  >>  >    +                    // continue below
>>>>> >  >>  >    +                }
>>>>> >  >>  >    +
>>>>> >  >>  >    +                // As a last resort, look in any available indexes
>>> >  >  Exceptions are getting eaten.  The CNFE should use the thrown exceptions as the initCause.  I'm especially
>>> >  >  worried about just catching Exception; can we not be more specific?
>>> >  >
>> >
>> >  I believe we want exceptions to be eaten here. This method, loadClass,
>> >  can only throw ClassNotFoundExceptions. It has to search in a few
>> >  locations and attempt a number of things before it realizes that a class
>> >  can not be found. So the cause that it may throw (and that user code may
>> >  see) is very likely to be meaningless anyway.
>> >
> I have no problem with them throwing CNFEs but the cause should be there.
> If it's meaningless, it can be ignored, but if it's not there, we may lose
> essential debugging information.
>

Ok, so I have modified the code to throw CNFE with causes. However, the 
code then catches (and logs) it. This allows the other methods of 
finding the jar to continue and the user code never gets to see the 
original CNFE.

>> >  As for catching Exception, you do have a point. I had left it the way it
>> >  was originally (see the code below). I know that it can throw
>> >  IllegalArgumentException if the Resource is not being tracked (which
>> >  should not happen here). I suppose I can remove the catch block completely.
>> >
> Try it.
>

Done. I havent seen any problems with the patch. However, I still think 
this is one of few cases where we should catch any exception and 
continue as if nothing has happened.

Thanks,
Omair
-------------- next part --------------
A non-text attachment was scrubbed...
Name: add-new-jars-from-classpath-02.patch
Type: text/x-patch
Size: 7549 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20110415/3c1f834f/add-new-jars-from-classpath-02.patch