RFR: 8324646: Avoid Class.forName in SecureRandom constructor [v2]

[Oli Gillespie]

On Wed, 24 Jan 2024 17:45:06 GMT, Aleksey Shipilev <shade at 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