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

[Jaikiran Pai]

On Fri, 29 Nov 2024 08:04:33 GMT, Jan Lahoda <jlahoda at openjdk.org> wrote:

>> 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

Thank you for these inputs, Jan and Alan. I've now updated this PR to remove this additional code and related error property key (I've verified it's not used anywhere else). 
tier1, tier2 and tier3 testing continues to pass with these latest changes.

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

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