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

[Omair Majid]

On 04/15/2011 12:27 PM, Omair Majid wrote:
> 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.
>

Ping. Any other thoughts?

Cheers,
Omair