[core-libs] RFR (L): 8010319: Implementation of JEP 181: Nest-Based Access Control

[Alan Bateman]

On 15/05/2018 01:52, David Holmes wrote:
> This review is being spread across four groups: langtools, core-libs, 
> hotspot and serviceability. This is the specific review thread for 
> core-libs - webrev:
>
> http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v1/ 
The API additions mostly look good, just a few comments:

- Can the spec for Class::getNestHost be explicit that it returns "this" 
when the class is a primitive or array class?

- Can Class::getNestMembers specify if it runs the class initializer of 
any classes that it loads? Less of an issue for getNestHost as the host 
is likely loaded already.

- I suspect the @throws SecurityException in getNestMembers was copied 
from getNestHost as it uses "returned class" (singular). As the host and 
members are in the same runtime package then maybe it can be specified 
in terms of the host or members package?

- isNestmateOf could be a bit clearer that it returns false in the event 
that the attributes can't be parsed. I realize it's specified in terms 
of getNestHost but I'm sure you see what I mean.

The tests for core reflection are in test/jdk/java/lang/reflect and I'm 
wondering if there has been any discussion about adding at least some 
basic tests there? I see the tests in 
test/hotspot/jtreg/runtime/Nestmates/reflectionAPI but this isn't an 
obvious location for someone touching this code.

The test in InterfaceAccessFlagsTest.java can be disabled with 
@Test(enabled=false), might be cleaner than commenting it out.

Maybe a question for Kumar but are we planning to pull in any ASM 
updates for JDK 11? NestMembers extends Attribute looks okay, I'm less 
sure about the change to ClassReader as I don't know if there is 
somewhere else in ASM that has the list of attributes to always parse.

The update to the access check in Reflection.java looks okay (just need 
to use 4-space indent, not 2).  I think it's okay in VerifyAccess too 
but the "FIX ME" suggests there are doubts so I assume this needs more 
eyes to triple check.

In passing, you might want to double check the javadoc links. I noticed 
one #getNestHost that is missing parenthesis and I'm not sure how 
javadoc handles that.

-Alan.