RFR: 8359808: JavaRuntimeURLConnection should only connect to non-directory resources

[David Beaumont]

On Wed, 18 Jun 2025 12:53:56 GMT, David Beaumont <duke at openjdk.org> wrote:

> Simplifying JavaRuntimeURLConnection to avoid accidentally returning non-resource data to users.
> 
> This change has the following distinct parts:
> 1. Refactor code to use Node instead of directly accessing low level ImageLocation type.
> 2. Remove unnecessary use of "Resource" interface and related URL generation code (completely unreachable).
> 3. Adding comments explaining why there's a non-obvious distinction in how module and resource names are treated with respect to URL percent encoding.
> 4. Small constructor logic simplification (module name cannot be null anymore)
> 5. Small simplification around 'READER' use, since it is impossible for that to ever be null (other users of ImageReaderFactory already assume it could never be null, and code path analysis agrees).
> 6. Adding tests for the non-resource cases.
> 7. Adding extra test data to check the behaviour with respect to things like percent escaping (previously untested).
> 8. Adding TODO comments for things I could do in this PR or later (reviewer opinions welcome).

Preloading a few explanatory comments.

src/java.base/share/classes/sun/net/www/protocol/jrt/JavaRuntimeURLConnection.java line 49:

> 47: 
> 48:     // ImageReader to access resources in jimage
> 49:     private static final ImageReader reader = ImageReaderFactory.getImageReader();

Examination of code and other users shows that this could never be null. Other users also name it "READER".

src/java.base/share/classes/sun/net/www/protocol/jrt/JavaRuntimeURLConnection.java line 56:

> 54: 
> 55:     // the Resource when connected
> 56:     private volatile Resource resource;

The Resource API was never needed here and adds uncalled methods such as `getURL()` which are then implemented with code that could never be invoked.

src/java.base/share/classes/sun/net/www/protocol/jrt/JavaRuntimeURLConnection.java line 64:

> 62:             throw new MalformedURLException(url + " missing path or /");
> 63:         if (path.length() == 1) {
> 64:             this.module = null;

There's no reason for the module==null case, since the only thing caring that this might be null just converts null to the empty string anyway.

src/java.base/share/classes/sun/net/www/protocol/jrt/JavaRuntimeURLConnection.java line 101:

> 99:             }
> 100:             this.resource = node;
> 101:             super.connected = true;

I know I don't *need* to use super here, but it documents the fact that this is not a field of this subclass, without readers having to go check. Mutable protected fields are weird and error prone, so I felt calling it out a bit was worth it. Happy to replace with a comment if people feel that's better though.

src/java.base/share/classes/sun/net/www/protocol/jrt/JavaRuntimeURLConnection.java line 119:

> 117:     public long getContentLengthLong() {
> 118:         try {
> 119:             return getResourceNode().size();

Having getResourceNode() return the (lazily fetched) node avoids the reader needing to know/reason about how "connect()" has the side-effect of making the "resource" field non-null.

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

PR Review: https://git.openjdk.org/jdk/pull/25871#pullrequestreview-2939094081
PR Review Comment: https://git.openjdk.org/jdk/pull/25871#discussion_r2154527602
PR Review Comment: https://git.openjdk.org/jdk/pull/25871#discussion_r2154529805
PR Review Comment: https://git.openjdk.org/jdk/pull/25871#discussion_r2154532882
PR Review Comment: https://git.openjdk.org/jdk/pull/25871#discussion_r2154538127
PR Review Comment: https://git.openjdk.org/jdk/pull/25871#discussion_r2154543346