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

[Daniel Fuchs]

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?

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

=============

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

===========

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?

BTW: is <h1> the right header? I thought <h1> was reserved for the
      class name. John Gibbon will know more ;-)

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