RFR: 8159746: (proxy) Support for default methods

[Mandy Chung]

On Thu, 15 Oct 2020 09:04:09 GMT, Peter Levart <plevart at openjdk.org> wrote:

>> `invokeDefaultMethod` is moved to a static method in `InvocationHandler`.
>>  I also add `@throws IllegalAccessException` if the specified default method is
>> inaccessible to the caller (access check was missing in the previous version).
>> The declaring interface of the default method must be exported to the caller to
>> use.  If it's a non-public interface, the caller class must be in the same runtime
>> package as the interface.  The example in javadoc includes the case when
>> the proxy interface is modified.
>
> Hi Mandy,
> 
> I'm beginning to think that we somewhat wondered away from the original intent which was to mimic the bytecode
> behaviour as much as possible. Bytecode behaviour for super calls (invokespecial) is very special and what you do in
> the last patch is guarding invocation to a super default method via @CallerSensitive method which checks the access of
> the caller as though this was a normal call. Similar access check when a default method might reside in a
> package-private class or when this class might not be exported is already performed when the code creates a Proxy
> class/instance (Proxy::getProxyClass/newProxyInstance). This code also designates an InvocationHandler for the proxy
> instance. From that point on, the designated invocation handler should have full access to invoking super default
> methods on the proxy instance regardless of where it resides because it has been chosen as the delegate of the Proxy
> class with full powers.  Since we are exposing the invokeDefaultMethod functionality in a public static method, it is
> hard (or not quite possible) to check that the code doing the invocation is in fact the designated invocation handler
> code. One approximation might be the following check:  `Proxy.getInvocationHandler(proxy).getClass() == callerClass`
> ...but that would not work for code residing in the invocation handler superclass even though such code might be
> eligible to call the super default method. OTOH such check is over permissive as it allows calling the super method
> from static context too.  So I was thinking about an alternative approach. To expose the method as a protected final
> instance method in an abstract class implementing InvocationHandler:  public interface InvocationHandler {
>   ...
>   abstract class Support implements InvocationHandler {
>     protected final Object invokeDefaultMethod(Object proxy, Method method, Object... args) throws
>     InvocationTargetException {
>       if (this != Proxy.getInvocationHandler(proxy)) throw ...
>       ...
>     }
>   }
> }
> 
> Users would have to extend `InvocationHandler.Support` to access this feature. This might be OK for most users, but
> those wanting to extend some other class will not be able to do that.
> So WDYT? In case you still prefer your approach of re-checking the permissions in invokeDefaultMethod, then I might
> have some ideas of how to speed-up theses checks. As currently stands, invoking public exported interface default
> methods does exhibit a little more penalty, but for non-public or non-exported interfaces, the penalty is bigger:
> Mandy's original:
> Benchmark             Mode  Cnt    Score    Error  Units
> ProxyBench.implClass  avgt    5    3.709 ±  0.026  ns/op
> ProxyBench.implProxy  avgt    5  926.215 ± 11.835  ns/op
> 
> Peter's performance patch:
> 
> Benchmark             Mode  Cnt   Score   Error  Units
> ProxyBench.implClass  avgt    5   3.777 ± 0.005  ns/op
> ProxyBench.implProxy  avgt    5  27.579 ± 0.250  ns/op
> 
> Moved to InvocationHandler + added access check
> 
> Benchmark               Mode  Cnt    Score   Error  Units
> ProxyBench.implClass    avgt    5    3.740 ± 0.004  ns/op
> ProxyBench.implProxy    avgt    5   34.226 ± 0.125  ns/op
> ProxyBench.ppImplClass  avgt    5    3.780 ± 0.004  ns/op
> ProxyBench.ppImplProxy  avgt    5  147.318 ± 1.422  ns/op
> 
> The updated benchmark code is here: https://gist.github.com/plevart/f6e0a5224c3ffbf14b28b47755599226
> (the ppImpl... benchmarks are against package-private interface)

Hi Peter,

The current `newProxyInstance` API does not perform access check against the proxy interfaces.    I want to avoid
adding new caller-sensitive method where possible.   I did consider any alternative to make it non-static method which
only allows a proxy's invocation handler to call `invokeDefaultMethod` if `proxy.getInvocationHandler() == this`.  If
access check done at invocation time, `invokeDefaultMethod` still needs to be caller-sensitive which must be a final
method (can't be on a default method)  In addition, an invocation handler to call `invokeDefaultMethod` can't be
implemented in a lambda because it can't self-reference itself.  So the invocation handler that wants to invoke default
methods must be a concrete or anonymous class which is a limitation.

Alan and I also discussed other alternative earlier.   I agree to prototype a slightly different alternative that will
perform access check at proxy instance creation time: 1. define an abstract `DelegatingInvocationHandler` class that
defines a protected final `invokeDefault` method (shortened method name). 2. `Proxy::newProxyClass` will perform access
check if the given `InvocationHandler` is an instance of `DelegatingInvocationHandler` 3. `Proxy(InvocationHandler h)`
constructor should throw an exception if the given `h` is an instance of `DelegatingInvocationHandler`.
`Proxy::getProxyClass` is deprecated and it's recommended to call `newProxyInstance` instead.  It's strongly
discouraged to invoke the Proxy constructor and so I think not supporting `DelegatingInvocationHandler` may be
reasonable. 4. `Proxy::getInvocationHandler` needs to check if the caller class has access to the proxy interfaces if
it's a `DelegatingInvocationHandler`.

So one can't get a proxy instance whose invocation handler can invoke default methods if the caller has no access to
the proxy interfaces; similarly one can't get a `DelegatingInvocationHandler` from a proxy instance if the caller has
no access.

This alternative avoids `invokeDefault` being caller-sensitive but the limitation of `DelegatingInvocationHandler`
implementation in a concrete/anonymous class.

Thought/feedback?

-------------

PR: https://git.openjdk.java.net/jdk/pull/313