8358729: jdk/internal/loader/URLClassPath/ClassnameCharTest.java depends on Applet #25703

justin-curtis-lu · 2025-06-09T20:42:43Z

Please review this PR which finishes Applet removal for the test: jdk/internal/loader/URLClassPath/ClassnameCharTest.java.

testclasses.jar is updated such that the two classes no longer extend Applet.

$ javap fo\ o.class 
public class fo o {
}
$ javap æ$'\302\211'$'\302\213'å$'\302\206'$'\302\214'.class 
public class 手册 {
}

The bug description of JDK-8358729 contains the original javap output for those classes.

Additionally, the security APIs that were marked for removal are also removed from this test as well.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8358729: jdk/internal/loader/URLClassPath/ClassnameCharTest.java depends on Applet (Bug - P3)

Reviewers

Jaikiran Pai (@jaikiran - Reviewer)
Lance Andersen (@LanceAndersen - Reviewer) Review applies to 02c76c74

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25703/head:pull/25703
$ git checkout pull/25703

Update a local copy of the PR:
$ git checkout pull/25703
$ git pull https://git.openjdk.org/jdk.git pull/25703/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25703

View PR using the GUI difftool:
$ git pr show -t 25703

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25703.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-06-09T20:43:17Z

👋 Welcome back jlu! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-06-09T20:43:34Z

@justin-curtis-lu This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8358729: jdk/internal/loader/URLClassPath/ClassnameCharTest.java depends on Applet

Reviewed-by: jpai, lancea

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 133 new commits pushed to the master branch:

1130c1b: 6798061: The removal of System.out.println from KeyboardManager
2f63d3a: 7116990: (spec) Socket.connect(addr,timeout) not clear if IOException because of TCP timeout
382f870: 8357976: GenShen crash in swap_card_tables: Should be clean
... and 130 more: https://git.openjdk.org/jdk/compare/15178aa298e43be3e27121343432f25884db4e5d...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2025-06-09T20:44:41Z

@justin-curtis-lu The following label will be automatically applied to this pull request:

core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2025-06-09T20:51:41Z

Webrevs

jaikiran · 2025-06-11T16:07:13Z

Hello Justin, how was the class file with those space characters in the method name and the class name generated? If we can do that within the test itself then I think it should also allow us to generate that JAR file dynamically in the test instead of having to continue maintaining this binary file.

justin-curtis-lu · 2025-06-11T16:56:48Z

Hi Jai, I had trouble creating class "fo o" through traditional means, so I used the ClassFile API to create those class files. Using JDKToolFinder.getCompileJDKTool("jar") with the CF API seems reasonable to do all the work dynamically. I will take a look, thanks for the idea.

jaikiran · 2025-06-11T17:05:02Z

so I used the ClassFile API to create those class files.

Use of ClassFile API I think is a good approach.

Using JDKToolFinder.getCompileJDKTool("jar") with the CF API seems reasonable to do all the work dynamically. I will take a look, thanks for the idea.

If you run into trouble using the jar tool, then we have other APIs/ways to generate such JAR file within the test. I'll let you continue experimenting with the jar tool and if you think you will need help at some point, then let me know and I can try a few things in this test.

justin-curtis-lu · 2025-06-11T22:37:50Z

Hi Jai,

ab8785c dynamically creates the class and jar file. I decided not to write "手册" as a Jar entry, since it's commented out in the test. It should be trivial to dynamically create an entry for that class in the future if needed.

jaikiran · 2025-06-12T15:09:42Z

test/jdk/jdk/internal/loader/URLClassPath/ClassnameCharTest.java

 * @summary cannot load class names containing some JSR 202 characters;
 *          plugin does not escape unicode character in http request
+ * @library /test/lib
 * @modules java.base/sun.net.www
 *          jdk.httpserver
 * @compile -XDignore.symbol.file=true ClassnameCharTest.java


Hello Justin, this is pre-existing but since we are updating this test, I think it better to remove this @compile line altogether. It isn't needed for this test.

jaikiran · 2025-06-12T15:11:12Z

test/jdk/jdk/internal/loader/URLClassPath/ClassnameCharTest.java

-                throw new ClassNotFoundException(name, e.getException());
-            }
+            } catch (Exception _) {}
+            throw new ClassNotFoundException(name);


I think we should propagate the underlying cause too, to help debugging if it fails for whatever reason. So something like:

catch (Exception e) { throw new ClassNotFoundException(name, e); }

Good point, also gave the other exception a more helpful message as well.

jaikiran · 2025-06-12T15:14:37Z

test/jdk/jdk/internal/loader/URLClassPath/ClassnameCharTest.java

 import sun.net.www.ParseUtil;

 public class ClassnameCharTest {
-    static String FNPrefix = System.getProperty("test.src", ".") + File.separator;
-    static File classesJar = new File(FNPrefix + "testclasses.jar");
+    private static final String JAR_PATH = Utils.TEST_CLASSES + Utils.FILE_SEPARATOR + "testclasses.jar";


I think it would be better to use java.nio.file.Path which is like:

private static final Path JAR_PATH = Path.of(".").resolve("testclasses.jar");

That way we don't have to reference the Utils.TEST_CLASSES. Path.of(".") will end up being the scratch directory of the test so jtreg can then retain this JAR file if the test fails for any reason.

Thanks, this is cleaner.

jaikiran · 2025-06-12T15:15:32Z

test/jdk/jdk/internal/loader/URLClassPath/ClassnameCharTest.java

-    static String FNPrefix = System.getProperty("test.src", ".") + File.separator;
-    static File classesJar = new File(FNPrefix + "testclasses.jar");
+    private static final String JAR_PATH = Utils.TEST_CLASSES + Utils.FILE_SEPARATOR + "testclasses.jar";
+    static File classesJar = new File(JAR_PATH);


If we switch to using Path for JAR_PATH, like I suggest above, then we can get rid of this field altogether and at the call site, we can just to JAR_PATH.toFile() if we want to have a File instance corresponding to that Path.

jaikiran · 2025-06-12T15:29:55Z

Thank you Justin for these changes. Overall I believe these are the correct changes to do. I have some review suggestions which I've added inline.

I went back and looked at the JDK-5017871 issue through which the fo o.class was being tested and from what I see in there, the current test changes continue to test that issue. I think there might be better ways to test that original use case and this test could be simplified a lot, but it's certainly not for this PR.

…JAR_PATH

jaikiran · 2025-06-13T06:01:53Z

test/jdk/jdk/internal/loader/URLClassPath/ClassnameCharTest.java

                }
-            } catch (PrivilegedActionException e) {
-                throw new ClassNotFoundException(name, e.getException());
+                // protocol/host/port mismatch, fail with RuntimeExc


Looks like a typo - should have been RuntimeException?

jaikiran

Hello Justin, the changes look OK to me. Please wait for a Lance to take a look too.

LanceAndersen

Thank you for the updates Justin.

Looks good overall. If you have to update this test again, it might be worthwhile considering converting to a junit test

LanceAndersen · 2025-06-12T21:42:26Z

test/jdk/jdk/internal/loader/URLClassPath/ClassnameCharTest.java

                }
-            } catch (PrivilegedActionException e) {
-                throw new ClassNotFoundException(name, e.getException());
+                // protocol/host/port mismatch, fail with RuntimeExc


Typo RuntimeExc?

02c76c7 corrects the comment typo and also makes a conversion of this test to JUnit.

…a' methods

LanceAndersen

Thank you for the changes Justin

jdlib · 2025-06-13T20:56:49Z

test/jdk/jdk/internal/loader/URLClassPath/ClassnameCharTest.java

+    @BeforeAll
+    static void setup() throws IOException {
+        var bytes = ClassFile.of().build(ClassDesc.of("fo o"), _ -> {});
+        try (JarOutputStream jos = new JarOutputStream(new FileOutputStream(JAR_PATH.toFile()))) {


Suggestion: The original test used testclasses.jar to provide two class files with invalid names packaged in a jar. Since you now dynamically construct the class files, there is no need to write them to disk as a jar. Why not have a Map<String,byte[]), mapping a resource path to bytes, to store all used class files in memory.

Thanks, I think we can make this simplification because the original version of this test used to test directly against a "fo o.class" binary file, and changed overtime to adapt the jar. (Thanks @jaikiran for confirming). I updated it such that we just read the class file bytes directly, and all JAR related logic is dropped. I did not put it in a map, since the second class is commented out, and we have been testing only "fo o" for quite some time now.

jdlib · 2025-06-13T21:08:01Z

I went back and looked at the JDK-5017871 issue through which the fo o.class was being tested and from what I see in there, the current test changes continue to test that issue. I think there might be better ways to test that original use case and this test could be simplified a lot, but it's certainly not for this PR.

Maybe labelling this test with @bug 4957669 5017871 was a mistake, test/jdk/java/net/Socket/IDNTest.java does address 4957669 and 5017871.

jdlib · 2025-06-17T07:52:03Z

test/jdk/jdk/internal/loader/URLClassPath/ClassnameCharTest.java

-                return super.findClass(name);
-            } catch (ClassNotFoundException e) {
-            }
-
            // Otherwise, try loading the class from the code base URL
            //      final String path = name.replace('.', '/').concat(".class").concat(cookie);
            String encodedName = ParseUtil.encodePath(name.replace('.', '/'), false);
            final String path = (new StringBuffer(encodedName)).append(".class").append(cookie).toString();


Suggestion: final String path = encodedName + ".class" + cookie;, avoiding StringBuffer

jdlib · 2025-06-17T08:16:15Z

I am still wondering what this (old) test tries to proof. It is filed below jdk.onternal.loader.URLClassPath but doesn't use or test URLClassPath, and it succeeds to construct a Class with an invalid name "fo o".

justin-curtis-lu · 2025-06-17T21:47:23Z

I am still wondering what this (old) test tries to proof. It is filed below jdk.onternal.loader.URLClassPath but doesn't use or test URLClassPath, and it succeeds to construct a Class with an invalid name "fo o".

URLClassPath is used in URLClassLoader, so I think this is OK in the current test structure. In any case, any re-examination would need to be done in a separate issue. The scope of this PR is to simply remove the remnants of Applet.

jaikiran

Thank you for the updates Justin. This looks good to me.

init

5d4faa3

justin-curtis-lu mentioned this pull request Jun 9, 2025

8359053: Implement JEP 504 - Remove the Applet API #25698

Open

4 tasks

openjdk bot changed the title ~~JDK-8358729: jdk/internal/loader/URLClassPath/ClassnameCharTest.java depends on Applet~~ 8358729: jdk/internal/loader/URLClassPath/ClassnameCharTest.java depends on Applet Jun 9, 2025

openjdk bot added the rfr Pull request is ready for review label Jun 9, 2025

openjdk bot added the core-libs [email protected] label Jun 9, 2025

Jai's review - dynamically create jar file

ab8785c

jaikiran reviewed Jun 12, 2025

View reviewed changes

justin-curtis-lu added 3 commits June 12, 2025 09:29

Jai's review - Cleanup & JAR_PATH String -> Path

afda9fe

removing a typo

a9cad71

Address offline review -> comments for maintainers, simplify exc and …

47f62aa

…JAR_PATH

jaikiran reviewed Jun 13, 2025

View reviewed changes

jaikiran approved these changes Jun 13, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 13, 2025

LanceAndersen approved these changes Jun 13, 2025

View reviewed changes

Address review - Convert to JUnit, correct comment typo, remove 'Infr…

02c76c7

…a' methods

openjdk bot removed the ready Pull request is ready to be integrated label Jun 13, 2025

LanceAndersen approved these changes Jun 13, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 13, 2025

jdlib reviewed Jun 13, 2025

View reviewed changes

Review suggestion - read class file from memory as bytes

9e395b2

openjdk bot removed the ready Pull request is ready to be integrated label Jun 16, 2025

jdlib reviewed Jun 17, 2025

View reviewed changes

SB -> String

58c6c4a

jaikiran approved these changes Jun 18, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 18, 2025

8358729: jdk/internal/loader/URLClassPath/ClassnameCharTest.java depends on Applet #25703

Are you sure you want to change the base?

8358729: jdk/internal/loader/URLClassPath/ClassnameCharTest.java depends on Applet #25703

Uh oh!

Conversation

justin-curtis-lu commented Jun 9, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Jun 9, 2025

Uh oh!

openjdk bot commented Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Jun 9, 2025

Uh oh!

mlbridge bot commented Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

jaikiran commented Jun 11, 2025

Uh oh!

justin-curtis-lu commented Jun 11, 2025

Uh oh!

jaikiran commented Jun 11, 2025

Uh oh!

justin-curtis-lu commented Jun 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaikiran commented Jun 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaikiran left a comment

Choose a reason for hiding this comment

Uh oh!

LanceAndersen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LanceAndersen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdlib commented Jun 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdlib commented Jun 17, 2025

Uh oh!

justin-curtis-lu commented Jun 17, 2025

Uh oh!

jaikiran left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

justin-curtis-lu commented Jun 9, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Jun 9, 2025 •

edited

Loading

mlbridge bot commented Jun 9, 2025 •

edited

Loading