On Wed, 24 Jan 2024 17:45:06 GMT, Aleksey Shipilev <sh...@openjdk.org> wrote:
>> Oli Gillespie has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - Rename newSecureRandom -> create
>> - Add copyright header to new file
>
> src/java.base/share/classes/java/security/Provider.java line 1560:
>
>> 1558: final boolean supportsParameter;
>> 1559: final String constructorParameterClassName;
>> 1560: private volatile Class<?> constructorParameterClass;
>
> Style: no need for `private` here, match what other fields are doing.
I don't disagree in principle but it was like this before the revert, and is
still like this in 17.
> src/java.base/share/classes/java/security/Provider.java line 1568:
>
>> 1566: }
>> 1567:
>> 1568: Class<?> getConstructorParameterClass() throws
>> ClassNotFoundException {
>
> I think we want to do what `Service.getImplClass` is doing, for extra safety:
> retain cached class on a `WeakReference`. This would avoid accidental class
> leakage in this code.
Thanks. I reverted it exactly from JDK-8280970 because a) that particular code
was there for a long time and presumably was reasonably well used/tested
without big issues and b) I want to backport the optimization to JDK17 which
still has this exact implementation.
Aside: The classes saved here are limited to the 31 explicitly added in
`Provider.<clinit>`. I'm not sure if that helps limit the leak potential
significantly?
> src/java.base/share/classes/java/security/Provider.java line 1909:
>
>> 1907: } else {
>> 1908: ctrParamClz = cap.constructorParameterClassName ==
>> null?
>> 1909: null : cap.getConstructorParameterClass();
>
> Maybe we should move the ternary for `cap.constructorParameterClassName ==
> null` into new method to begin with.
Thanks. Similar reasoning as above. If we decide to diverge from the original
implementation then I'll update this too.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17559#discussion_r1465346119
PR Review Comment: https://git.openjdk.org/jdk/pull/17559#discussion_r1465346269
PR Review Comment: https://git.openjdk.org/jdk/pull/17559#discussion_r1465347099
