RFR: 8338445: jdk.internal.loader.URLClassPath may leak JarFile instance when dealing with unexpected Class-Path entry in manifest [v2]

[Jaikiran Pai]

On Sun, 25 Aug 2024 15:08:27 GMT, Alan Bateman <alanb at openjdk.org> wrote:

>> Jaikiran Pai has updated the pull request incrementally with three additional commits since the last revision:
>> 
>>  - revert to old code comment
>>  - use "JAR file" consistently in test's code comments
>>  - rename closeLoaderIgnoreEx to closeQuietly
>
> src/java.base/share/classes/jdk/internal/loader/URLClassPath.java line 447:
> 
>> 445:                 loader = getLoader(url);
>> 446:                 // If the loader defines a local class path then construct
>> 447:                 // and add the URLs as the next URLs to be opened.
> 
> The change to URLClassLoad looks okay although I think the original comment was a bit clearer, no need to insert "then construct" here as this is just getting the class path URLs.

Hello Alan, I've reverted the change to this code comment in the latest update to this PR.

> src/java.base/share/classes/jdk/internal/loader/URLClassPath.java line 453:
> 
>> 451:                 if (DEBUG) {
>> 452:                     System.err.println("Failed to construct a loader or construct its" +
>> 453:                             " local classpath for " + url + ", cause:" + e);
> 
> At some point we should probably get rid of the DEBUG stuff, it looks like it was left over from early JDK releases.

The debug logs are currently enabled through the `sun.misc.URLClassPath.debug` system property. Do you mean we should completely get rid of the `sun.misc.URLClassPath.debug` system property in future?

> src/java.base/share/classes/jdk/internal/loader/URLClassPath.java line 482:
> 
>> 480: 
>> 481:     // closes the given loader and ignores any IOException that may occur during close
>> 482:     private static void closeLoaderIgnoreEx(final Loader loader) {
> 
> closeQuietly(Loader loader) would work too.

I had considered `safeClose(...)` but that didn't sound right. But what you suggest is more precise. I've updated the PR to use this suggestion.

> test/jdk/java/net/URLClassLoader/JarLoaderCloseTest.java line 78:
> 
>> 76: 
>> 77:     /*
>> 78:      * Creates a jar with a manifest which has a Class-Path entry value with malformed URLs.
> 
> I think you mean a "a JAR file" rather than "jar" here. Same nit in  testParsableClassPathEntry.

Fixed.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/20691#discussion_r1730676789
PR Review Comment: https://git.openjdk.org/jdk/pull/20691#discussion_r1730677519
PR Review Comment: https://git.openjdk.org/jdk/pull/20691#discussion_r1730677963
PR Review Comment: https://git.openjdk.org/jdk/pull/20691#discussion_r1730678041