[Nestmates] RFR: 8196320: [Nestmates] Restore the old enclosing-class based isSamePackageMember check in sun/invoke/util/VerifyAccess.java

[mandy chung]

On 1/31/18 6:51 PM, David Holmes wrote:
> Hi Mandy,
>
> Thanks for looking at this.
>
> On 1/02/2018 5:13 AM, mandy chung wrote:
>> On 1/30/18 8:54 PM, David Holmes wrote:
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8196320
>>> webrev: http://cr.openjdk.java.net/~dholmes/8196320/webrev/
>>>
>>> I've restored the original enclosing-class check and the 
>>> isSamePackageMember method, but renamed it to areNestMates. It will 
>>> first do the real Reflection.areNestMates check, and if that fails 
>>> fall-back to the pre-nestmate enclosing class check.
>>>
>>
>> Does core reflection work with the pre-nestmate class scenario?
>
> No. Pre-nestmates core reflection doesn't permit private access 
> between nestmates. (Though core reflection could have used the same 
> enclosing class check that is in VerifyAccess.)
>

OK.

>> Should JVM_AreNestMates handle the pre-nestmate class and return true 
>> if they are the same class or same runtime package?
>
> Not sure what you are getting at here. That function is purely for 
> checking nestmate attributes and establishing nestmateship based on 
> that attribute. For any pre-nestmate classfile each class is 
> considered its own nesthost and a check between any two different 
> pre-nestmate classes will yield false. If you pass the same class it 
> will vacuously yield true.

OK (I was confused with the semantics of JVM_AreNestMates).  It does 
return true on the same class.   It would help if you can add the 
comment describing this function in jvm.h.

>
> Callers to Reflection.areNestmates (which calls JVM_AreNestMates) can 
> do their own "optimizations" if they wish to pre-check if we are 
> dealing with the same class or different packages - as the original 
> isSameMemberPackage check does.

These optimization in VerifyAccess::areNestMates can be moved to 
Reflection::areNestMates so that it's clearer the difference is the 
top-level enclosing class check.

>
>>> I've added a test under runtime/Nestmates/legacy (where I'll 
>>> accumulate tests that check legacy code still works after the 
>>> nestmate changes - for specific areas like this.) 
>>
>> At some point we should look at where the nestmates tests should be 
>> placed.  For example, this test seems to be appropriate to land in 
>> test/jdk/java/lang/invoke.   Just a note to consider when integrating 
>> this to jdk.
>
> A bit OT for this review but our test structure is based around 
> regression tests, not functional testing, but is now being used more 
> and more for functional tests for new features. It becomes difficult 
> when a feature impacts a large number of areas as there are multiple 
> ways the tests could be structured.
>
> :
> but that kind of structure is only partially in existence today, and 
> as I said I'd prefer to see the tests kept together as a functional 
> group. That doesn't mean hotspot/jtreg/runtime/Nestmates is 
> necessarily the final landing place - but again we're not really set 
> up for feature based functional test organization.

No issue with that as it's convenient to keep them together for now.   
The test organization can be decided when it's prepared for integration.

>
> True. And if compiled for 11 but run on 10 that wont work anyway. :) 
> Okay I can delete this conservative check.
>

That's good.

>> Nit:
>>
>>    30  * @compile -source 10 -target 10 TestPrivateLookup.java
>>
>> Alternatively it can use --release 10 instead of -source and -target.
>
> Done. Though I wasn't sure if it would try to be too clever and tell 
> me Class.getNestHost() doesn't exist in JDK 10. Hmmm perhaps it still 
> will as right now we still appear to be JDK 10. ??
>

Ah good point.  This test will fail when the javac symbols are updated 
when it bumps to 11 (--release 10 will ensure it can only use JDK 10 
API).   You will need to split the C class to a separate source file to 
prepare that change.

Mandy