Review request 8153895 (proxy) redundant read edges to superinterfaces of proxy interfaces

[Mandy Chung]

> On Apr 12, 2016, at 6:16 AM, Peter Levart <peter.levart at gmail.com> wrote:
> 
> Hi Mandy,
> 
> There's another redundant validation and read edge addition I think. When deriving referenced types from methods of interfaces, static methods could be skipped as they are not implemented or overridden by Proxy class:

I think static methods could be skipped.

> 
> http://cr.openjdk.java.net/~plevart/jdk9-dev/Proxy.noStaticSignatures/webrev.01/
> 
> And there's also a question whether referenced types derived from method signatures should be checked for visibility from the specified class loader (in method validateProxyInterfaces). On one hand it is nice to ensure that later usage of the proxy class will not encounter problems of visibility of types in method signatures, but on the other, Java has been known to practice late binding and a normal class implementing an interface is loaded without problems although its method signatures reference types that are not visible from the class' class loader. Only when such method is called does the resolving happen and exception is thrown. So the question is whether such visibility checks are actually beneficial. For example, one could successfully use a proxy class by only invoking methods that reference visible types if this check was not performed.
> 

I noticed this missing visibility check when updating proxies to work with modules.  I don’t know the history why it only checks of proxy interfaces when the API was defined.  In typical cases, when a proxy interface is visible to the specified class loader, the referenced types are likely visible. On the other hand, I’m cautious of the compatibility risk if the visibility check applies to referenced types as well. I don’t think this check has vast benefits while past proxy changes broke existing libraries that show up the incompatibility risk is somewhat hard to assess.

> The problem is of course with the setup procedure of the dynamic module where you have to add exports from modules/packages of those referenced types and read edges to modules of those references types upfront. But this problem is resolvable. If a type is not visible from the proxy class' class loader, then we don't need an export from its module/package and we don't need a read edge to its module, do we?

True. If the type is not visible, NCFE will be thrown and this read edge and qualified exports would be redundant. Are you worrying the unused qualified exports causing security concerns?  The proxy classes in a dynamic module are encapsulate and I’m not too concerned of the redundant ones.

I’ll see what I would do for the static methods and send an updated webrev.

Mandy

> 
> What do you think?
> 
> Regards, Peter
> 
> On 04/12/2016 02:34 PM, Peter Levart wrote:
>> Hi Mandy,
>> 
>> This is OK, but the whole loop could be simplified. No need for Dequeue any more:
>> 
>>            // set up proxy class access to proxy interfaces
>>            Set<Class<?>> visited = new HashSet<>(interfaces);
>>            for (Class<?> c : visited) {
>>                ensureAccess(target, c);
>>            }
>> 
>> Regards, Peter
>> 
>> On 04/12/2016 01:45 AM, Mandy Chung wrote:
>>> Peter spots the redundant read edges are added to dynamic module when creating a proxy class.   Proxy class does not access super interfaces of proxy interfaces.   I have verified this simple patch with all core tests and JCK api tests.
>>> 
>>> diff --git a/src/java.base/share/classes/java/lang/reflect/Proxy.java b/src/java.base/share/classes/java/lang/reflect/Proxy.java
>>> --- a/src/java.base/share/classes/java/lang/reflect/Proxy.java
>>> +++ b/src/java.base/share/classes/java/lang/reflect/Proxy.java
>>> @@ -804,11 +804,6 @@
>>>                      continue;
>>>                  }
>>>                  ensureAccess(target, c);
>>> -
>>> -                // add all superinterfaces
>>> -                for (Class<?> intf : c.getInterfaces()) {
>>> -                    deque.add(intf);
>>> -                }
>>>              }
>>>                // set up proxy class access to types referenced in the method signature
>> 
>