On Tue, 25 Mar 2025 11:08:24 GMT, Timofei Pushkin <tpush...@openjdk.org> wrote:
> If a base class is package-private then its subclasses should have the same
> package name and defining class loader, otherwise `IllegalAccessError` is
> thrown when linking a subclass. Currently when dumping a static archive
> separate `URLClassLoader`s are used for each unregistered classes' source.
> Thus if two unregistered classes, a package-private base class and a sub
> class, from the same package reside in different sources `IllegalAccessError`
> will be thrown when linking the sub class. This can be unexpected because the
> app could have used a single class loader for both classes and thus not have
> seen the error — see `DifferentSourcesApp.java` from this patch for an
> example of such app.
>
> This patch fixes the issue by using a single class loader for all
> unregistered classes. CDS does not allow classes with the same name making
> such solution possible.
>
> Implementation note: `URLClassLoader` does not allow selecting a specific URL
> to load a specific class — I used reflection to override a private part of
> `URLClassLoader` responsible for URL selection while being able to use the
> rest of its implementation.
Hi Timofei, thanks for fixing this.
(Sorry I didn't notice this PR until now ...)
I have a suggestion for further simplification.
src/java.base/share/classes/jdk/internal/misc/CDS.java line 420:
> 418: // class loader. Thus it is safe to delegate their
> loading to system class loader
> 419: // (our parent) - this is what the default
> implementation of loadClass() will do.
> 420: return (Class<?>) DEFINE_CLASS.invoke(this, name, res);
Instead of subclassing from `URLClassLoader` and go through the trouble of
calling its `defineClass()` method, maybe we should just subclass from
`ClassLoader` and maintain a hashtable of java.util.jar.JarFiles.
HashMap<String, JarFile> map = ....;
JarFile jar = map.get(source) ... or open a new JarFile if not found
byte[] buffer = read jar file for the given class name;
call ClassLoader.defineClass(buffer, ...)
src/java.base/share/classes/jdk/internal/misc/CDS.java line 446:
> 444: protected Class<?> findClass(String name) throws
> ClassNotFoundException {
> 445: // Unregistered classes should be found in load(...),
> registered classes should be
> 446: // handeled by parent loaders
This does seem like a good simplification, as we no longer need to check the
`currentClassName`, `currentSuperClass`, etc.
We had a plan for supported multiple classes for the same name, but we never go
around doing it (even though the classlist format supports that with the use of
IDs).
I am not sure if the current tests includes a case where a class name `Foo`
exists both in the classpath as well as in a custom loader. If not, I think we
should add such a case (to ensure that UnregisteredClassLoader never delegates
to the application class loader and finds the wrong `Foo`).
-------------
PR Review: https://git.openjdk.org/jdk/pull/24223#pullrequestreview-2741852934
PR Review Comment: https://git.openjdk.org/jdk/pull/24223#discussion_r2028155863
PR Review Comment: https://git.openjdk.org/jdk/pull/24223#discussion_r2028141440
