Re: RFR: 8319436: Proxy.newProxyInstance throws NPE if loader is null and interface not visible from class loader [v2]
On Mon, 6 Nov 2023 20:22:43 GMT, Mandy Chung wrote: >> This is a regression caused by JDK-8302791. IAE should be thrown when an >> interface is not visible to the given class loader but NPE is thrown instead >> when the loader is null. The boot loader has no name and so the fix will >> print `null` in the exception message. >> `test/jdk/java/lang/reflect/Proxy/ClassRestrictions.java` is converted to >> junit and updated to test IAE thrown because proxy interface is not visible >> to null loader. > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > Review feedback The fix to getLoaderNameID looks fine, the update and conversion of the test looks okay too. - Marked as reviewed by alanb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16525#pullrequestreview-1717031599
Re: RFR: 8319436: Proxy.newProxyInstance throws NPE if loader is null and interface not visible from class loader [v2]
> This is a regression caused by JDK-8302791. IAE should be thrown when an > interface is not visible to the given class loader but NPE is thrown instead > when the loader is null. The boot loader has no name and so the fix will > print `null` in the exception message. > `test/jdk/java/lang/reflect/Proxy/ClassRestrictions.java` is converted to > junit and updated to test IAE thrown because proxy interface is not visible > to null loader. Mandy Chung has updated the pull request incrementally with one additional commit since the last revision: Review feedback - Changes: - all: https://git.openjdk.org/jdk/pull/16525/files - new: https://git.openjdk.org/jdk/pull/16525/files/5fef84a4..4be40b5d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16525=01 - incr: https://webrevs.openjdk.org/?repo=jdk=16525=00-01 Stats: 65 lines in 1 file changed: 10 ins; 34 del; 21 mod Patch: https://git.openjdk.org/jdk/pull/16525.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16525/head:pull/16525 PR: https://git.openjdk.org/jdk/pull/16525
Re: RFR: 8319436: Proxy.newProxyInstance throws NPE if loader is null and interface not visible from class loader
On Mon, 6 Nov 2023 19:46:40 GMT, Alan Bateman wrote:
>> This is a regression caused by JDK-8302791. IAE should be thrown when an
>> interface is not visible to the given class loader but NPE is thrown instead
>> when the loader is null. The boot loader has no name and so the fix will
>> print `null` in the exception message.
>> `test/jdk/java/lang/reflect/Proxy/ClassRestrictions.java` is converted to
>> junit and updated to test IAE thrown because proxy interface is not visible
>> to null loader.
>
> test/jdk/java/lang/reflect/Proxy/ClassRestrictions.java line 90:
>
>> 88: } catch (IllegalArgumentException e) {
>> 89: System.err.println(e.getMessage());
>> 90: // assume exception is for intended failure
>
> The "throw new Error(message)" could be replaced with fail(message) or use
> assertThrows(IllegalArgument.class, () -> Proxy.getProxyClass(...)).
Will do. I should clean up more of the existing code.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16525#discussion_r1383905730
Re: RFR: 8319436: Proxy.newProxyInstance throws NPE if loader is null and interface not visible from class loader
On Mon, 6 Nov 2023 19:12:28 GMT, Mandy Chung wrote:
> This is a regression caused by JDK-8302791. IAE should be thrown when an
> interface is not visible to the given class loader but NPE is thrown instead
> when the loader is null. The boot loader has no name and so the fix will
> print `null` in the exception message.
> `test/jdk/java/lang/reflect/Proxy/ClassRestrictions.java` is converted to
> junit and updated to test IAE thrown because proxy interface is not visible
> to null loader.
test/jdk/java/lang/reflect/Proxy/ClassRestrictions.java line 61:
> 59: }
> 60:
> 61: static Stream proxyInterfaces() {
These are cases where IAE is expected so maybe the method source should be
badProxyInterfaces to make it clearer that they are expected to fail?
test/jdk/java/lang/reflect/Proxy/ClassRestrictions.java line 90:
> 88: } catch (IllegalArgumentException e) {
> 89: System.err.println(e.getMessage());
> 90: // assume exception is for intended failure
The "throw new Error(message)" should be replaced with fail(message) or use
assertThrows(IllegalArgument.class, () -> Proxy.getProxyClass(...)).
test/jdk/java/lang/reflect/Proxy/ClassRestrictions.java line 105:
> 103: if (Modifier.isPublic(nonPublic2.getModifiers())) {
> 104: throw new Error("Interface " + nonPublicIntrfaceName +
> 105: " is public and need to be changed!");
You could use assertEquals here, or at least replace the throw new Error with
fail, up to you.
test/jdk/java/lang/reflect/Proxy/ClassRestrictions.java line 125:
> 123:
> 124: private static final String[] CPATHS =
> System.getProperty("test.classes", ".")
> 125:
> .split(File.pathSeparator);
It might be a bit clearer to define TEST_CLASSES to be the value
System.getProperty("test.classes", "."), and split it in
testNonVisibleInterface.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16525#discussion_r1383882661
PR Review Comment: https://git.openjdk.org/jdk/pull/16525#discussion_r1383885042
PR Review Comment: https://git.openjdk.org/jdk/pull/16525#discussion_r1383886740
PR Review Comment: https://git.openjdk.org/jdk/pull/16525#discussion_r1383896386
RFR: 8319436: Proxy.newProxyInstance throws NPE if loader is null and interface not visible from class loader
This is a regression caused by JDK-8302791. IAE should be thrown when an interface is not visible to the given class loader but NPE is thrown instead when the loader is null. The boot loader has no name and so the fix will print `null` in the exception message. `test/jdk/java/lang/reflect/Proxy/ClassRestrictions.java` is converted to junit and updated to test IAE thrown because proxy interface is not visible to null loader. - Commit messages: - 8319436: Proxy.newProxyInstance throws NPE if loader is null and interface not visible from class loader Changes: https://git.openjdk.org/jdk/pull/16525/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16525=00 Issue: https://bugs.openjdk.org/browse/JDK-8319436 Stats: 184 lines in 2 files changed: 63 ins; 96 del; 25 mod Patch: https://git.openjdk.org/jdk/pull/16525.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16525/head:pull/16525 PR: https://git.openjdk.org/jdk/pull/16525
