Review Request: JDK-8173978: Lookup.in should allow teleporting from a lookup class in a named module without dropping all access

[Mandy Chung]

Hi Daniel,

Thanks for catching these.

On 7/2/19 8:03 AM, Daniel Fuchs wrote:
> Hi Mandy,
>
> Don't count me as reviewer; this is only editorial:
>
> In MethodHandles.java:
>
> ============
>
>  165      * A caller, specified as a {@code Lookup} object, is in 
> module {@code M1} is
>  166      * allowed to do deep reflection on module {@code M2} and 
> package of the target class
>  167      * if and only if all of the following conditions are {@code 
> true}:
>
> There are two 'is' in this sentence, which makes it a bit difficult
> to parse. Should the first be removed?
>

Yes the first "is" is removed.
> * A caller, specified as a {@code Lookup} object, in module {@code M1} 
> is ...
>
> ============
>
>  169      * <li>If there is a security manager, its {@code 
> checkPermission} method is
>  170      * called to check {@code 
> ReflectPermission("suppressAccessChecks")} and
>  171      * that must return normally.
>  172      * <li>
>  173      * If there is a security manager, its {@code 
> checkPermission} method is
>  174      * called to check {@code 
> ReflectPermission("suppressAccessChecks")}.
>  175      * This method must return normally.
>
> Probably one of these two bullets should be removed: they look
> the same to me.
>

Removed. Thanks for catching it.

> =============
>
>  585      * A {@code Lookup} with {@link #PUBLIC} mode and a lookup 
> class in {@code M1}
>  586      * can access public types in a module {@code M2} when {@code 
> M2} is readable to
>  587      * {@code M1} when the type is in a package of {@code M2} 
> that is exported to at least {@code M1}.
>                      ^^^^
> I believe there is "and" missing here:
>
>   <<... when M2 is readable to M1 *and* when the type ....>>
>

Fixed.
> ===========
>
> Is the specdiff report incomplete? I was hoping to see the table
> added to the Lookup class level API under
>
>  582      * <h1><a id="cross-module-lookup"></a>Cross-module lookups</h1>
>
> but it seems to be missing from the specdiff?
>

The specdiff tool is broken.  Sigh... I workaround to generate the 
method/field summary but hit another bug that it does not generate the 
class summary.
> BTW: is <h1> the right header? I thought <h1> was reserved for the
>      class name. John Gibbon will know more ;-)
>

It should be <h2>.   Updated.   This was written prior to that change.

thanks
Mandy
> best regards,
>
> -- daniel
>
> On 02/07/2019 03:47, Mandy Chung wrote:
>>
>>
>> On 7/1/19 12:51 PM, Mandy Chung wrote:
>>> This is an enhancement to |`Lookup::in`| and 
>>> |`MethodHandles::privateLookupIn`| API
>>> for cross module teleporting.  A `Lookup` object will record the 
>>> previous
>>> lookup class from which this |Lookup| object was teleported such that
>>> the access check will use both the previous lookup class and the 
>>> current
>>> |lookup| context (current lookup class and allowed modes) to 
>>> determine if
>>> a type is accessible to this `Lookup` object or not.
>>>
>>> In a nutshell, `T` in M2 is accessible to a `Lookup` object on `C`
>>> (lookup class in M1) and `PLC` (previous lookup class in M0) if and 
>>> only if
>>> 1. both M0 and M1 can read M2
>>> 2. T is in a package that is exported from M2 at least to both M0 
>>> and M1
>>>
>>> Detailed specification is in Lookup class spec and `accessClass` 
>>> javadoc.
>>> The relevant spec about cross-module teleporting is in the Lookup class
>>> spec and `Lookup::in` and `MethodHandles::privateLookupIn`.
>>>
>>> CSR: https://bugs.openjdk.java.net/browse/JDK-8226916
>>>
>>> webrev:
>>> http://cr.openjdk.java.net/~mchung/jdk14/8173978/webrev.00
>>>
>>> javadoc:
>>> http://cr.openjdk.java.net/~mchung/jdk14/8173978/javadoc/java.base/java/lang/invoke/MethodHandles.Lookup.html 
>>>
>>>
>>> http://cr.openjdk.java.net/~mchung/jdk14/8173978/javadoc/java.base/java/lang/invoke/MethodHandles.html#privateLookupIn(java.lang.Class,java.lang.invoke.MethodHandles.Lookup) 
>>>
>>>
>>> I have yet to generate the spec diff. The tool is currently broken
>>> due to javadoc change.  I'll try to workaround it and post the
>>> spec diff soon.
>>
>> specdiff:
>>
>> http://cr.openjdk.java.net/~mchung/jdk14/8173978/specdiff/overview-summary.html 
>>
>>
>> Mandy
>