Hi Mandy,
> On 25 feb 2015, at 23:19, Mandy Chung <[email protected]> wrote:
>
>
> On 2/25/2015 5:19 AM, Joel Borggrén-Franck wrote:
>> InvocationHandler::invoke unfortunately throws Throwable, but I restructured
>> it a bit so it is easier to follow.
>>
>> http://cr.openjdk.java.net/~jfranck/8073056/webrev.01/
>
> 196 InvocationHandler handler = Proxy.getInvocationHandler(container);
>
> Do you want to ensure it's from our implementation?
> i.e. sun.reflect.annotation.AnnotationInvocationHandler
>
I think I have a mail somewhere where Joe states that annotations were designed
so that there could be other implementations of the invocation handler.
> 204 }catch (Throwable t) { // from InvocationHandler::invoke
>
> Missing space between } and catch
Will fix.
> 182 // According to JLS the container must have an array-valued value
> 183 // method. Get the AnnotationType, get the "value" method and invoke
> 184 // it to get the content.
> 190 Method m = annoType.members().get("value");
> 212 m.setAccessible(true);
>
> I am missing something here. I read the code and I think
> annoType is of sun.reflect.annotation.AnnotationType type.
> Does the old implementation still exist that is supported?
> Which method is it in the old implementation? If it's still
> supported, as Class.getAnnotationsByType is not specified to
> require any permission check nor throw any SecurityException,
> and it seems that setAccessible(true) should be wrapped with
> doPrivileged.
>
> If it's not used in our implementation after your patch,
> perhaps better to take m.setAccessible(true) out.
I realize now that this code will probably never have worked on other
hypothetical implementations of Annotations. The problem is with the default
method AnnotatedElement::getDeclaredAnnotationsByType. If someone has an
external implementation of AnnotatedElement (and one of the lessons from adding
these methods in 8 was that there are other implementations) then if that
external implementation haven’t added getDeclaredAnnotationsByType they will
call into our AnnotationSupport. This is all fine if that external
representation of AnnotatedElement uses our representation of Annotations, with
Proxies and all. But I suspect that the conditions that would end up taking the
non-proxy code path in the patch, will already fail in the check:
AnnotationType annoType =
AnnotationType.getInstance(containerClass);
if (annoType == null)
throw invalidContainerException(container, null);
So what is the best thing to do here if the annotation is not Proxy based and
is coming from an implementation of the AnnotatedElement interface that we
don’t control and that is missing the methods added in 8? I think throwing here
might be the best option, especially since we will probably already have failed
in the AnnotationType.getInstance check.
cheers
/Joel
