On Sat, 16 Nov 2024 15:24:03 GMT, Attila Szegedi <att...@openjdk.org> wrote:
>> Roger Riggs has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Apply review comments:
>> Retain public static constants for permission names to avoid source/binary
>> compatible changes that affect cross version use.
>> Remove obsolete mention of security considerations.
>> (The protected constructor is retained to avoid changing the access for
>> subclass use)
>
> src/jdk.dynalink/share/classes/jdk/dynalink/SecureLookupSupplier.java line 46:
>
>> 44: * {@link #getLookup()} method.
>> 45: */
>> 46: public static final String GET_LOOKUP_PERMISSION_NAME =
>> "dynalink.getLookup";
>
> This public member is [referenced from
> Nashorn](https://github.com/search?q=repo%3Aopenjdk%2Fnashorn%20GET_LOOKUP_PERMISSION_NAME&type=code).
> If we remove it, Nashorn will have to attempt to look it up reflectively as
> long as it supports older versions of Java. (Presuming the
> `java.security.AccessController` and related classes aren't also removed
> altogether. Are they?)
>
> FWIW, as long as we're making breaking changes, this whole class could be
> removed – all it did was serve as a secure gate to access to a
> `MethodHandles.Lookup`. References to it could be replaced by the lookup
> object itself. I can understand if this might be too much work for this PR,
> though.
>
> On the other hand, if we do _not_ remove the whole class, I'd prefer to keep
> the public string constant for the permission name so that we don't break
> binary compatibility for whoever might be referencing it (specifically,
> Nashorn.)
Agreed, it is cleaner to retain the permission names in the public API.
> src/jdk.dynalink/share/classes/jdk/dynalink/linker/GuardingDynamicLinkerExporter.java
> line 65:
>
>> 63: if (sm != null) {
>> 64: sm.checkPermission(AUTOLOAD_PERMISSION);
>> 65: }
>
> We can probably remove the whole empty default constructor now. The class is
> abstract, the autogenerated default constructor will be protected anyway.
Keeping the explicitly declared constructor for clarity.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/22152#discussion_r1846872687
PR Review Comment: https://git.openjdk.org/jdk/pull/22152#discussion_r1846873840
