RFR: 8345153: Clean up SecurityManager references from jdk.compiler module

[Jan Lahoda]

On Thu, 28 Nov 2024 18:21:21 GMT, Alan Bateman <alanb at openjdk.org> wrote:

>> Can I please get a review of this change which removes references to SecurityManager related APIs from the `jdk.compiler` module?
>> 
>> With the removal of SecurityManager implementation, various parts of the JDK, including classloader construction or `ServiceLoader` usage will no longer do the `SecurityManager` backed security checks. Failure of such security checks would previously raise a `SecurityException`. With these checks now no longer applicable, the `SecurityException` is treated as any other regular `RuntimeException`. The commit in this PR removes the special treatment of `SecurityException` and other usages of SecurityManager implementation.
>> 
>> No new tests have been added and existing tests in tier1, tier2 and tier3 continue to pass with this change.
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/processing/JavacProcessingEnvironment.java line 354:
> 
>> 352:      * relevant path, otherwise if processors are present, logs an
>> 353:      * error.  Called when a service loader is unavailable for some
>> 354:      * reason, for example, because a service loader class cannot be found.
> 
> The comment "because a service loader class cannot be found" is puzzling. Zooming out, the remaining usage of handleServiceLoaderUnavailability is in ServiceIterator where it assumes that the 2-arg SL.load or SL::iterator can throw. SL.load will only throw if the caller (in jdk.compiler) doesn't `uses Processor` or that service class is not accessible, neither is true here. Maybe Jan or someone else can comment further but it looks to me that handleServiceLoaderUnavailability can be removed and the "Fail softly" handling in ServiceIterator can be removed too.

I went through the code, and I think this is right - the whole `handleServiceLoaderUnavailability` can be dropped, and everything that is referred to from it (`needClassLoader`, `handleException`, and especially(!) `ServiceProxy`; and probably also relevant keys from `compiler.properties`, unless they are used elsewhere) can, I think, be dropped.

More specifically, from `ServiceIterator.<init>`:

            try {
                try {
                    loader = ServiceLoader.load(Processor.class, classLoader);
                    this.iterator = loader.iterator();
                } catch (Exception e) {
                    // Fail softly if a loader is not actually needed.
                    this.iterator = handleServiceLoaderUnavailability("proc.no.service", null);
                }
            } catch (Throwable t) {
                log.error(Errors.ProcServiceProblem);
                throw new Abort(t);
            }


my reading of both the javadoc and code in the `ServiceLoader` is that we can drop both of the `try-catch` statements, leaving only:

loader = ServiceLoader.load(Processor.class, classLoader);
this.iterator = loader.iterator();


Although if we wanted to be very careful, we could keep the outter `try-catch`.

CC @jddarcy

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/22431#discussion_r1863105438