On 4/28/18 6:52 PM, Peter Levart wrote:
Hi Mandy,
On 04/28/18 11:44, mandy chung wrote:
Webrev:
http://cr.openjdk.java.net/~mchung/jdk11/webrevs/8202113/webrev.00/
The reflection machinery stores the caller class in each
AccessibleObject
such that it can skip the access check if access to a member has been
verified for a given caller. At the first time accessing the
AccessibleObject,
it creates an Accessor object and then cache for subsequent use.
This cached
Accessor object keeps a reference to the AccessibleObject object that
will keep the caller class alive.
...because the same instance of Accessor object is also set on the
root AccessibleObject in order to be shared among other child
AccessibleObject instances created from it and because this root
AccessibleObject is retained by the cache of AccessibleObject(s) in
the declaring j.l.Class...
Yes
The implementation has a root object for each AccessibleObject and
the API returns a child object for users to access (that may suppress
access check via setAccessible). The caller class is set in the cache
of the child object. This patch proposes to change ReflectionFactory
newXXXAccessor methods to ensure to pass the root object rather than
the child object. The cache of the root object is always null.
...since root AccessibleObject(s) are never used for accessing the
member(s), they will never cache the caller Class and so they can be
retained by the Class-level cache together with cached Accessors that
now just point back to them and never to AccessibleObject(s) handed
out to users that do cache caller Class.
Exactly.
Right, this patch fixes the issue of retaining (leaking) the caller
class when using AccessibleObject(s) returned from Reflection API and
then throwing them away.
There might be a separate issue when user-handed AccessibleObjects are
cached by user code and such cache is located in a different class
loader than classes that make invocations. This could only be solved
with a WeakReference in the access-check cache I suppose...
User code caching the AccessibleObject should be for its own use and I
would think the cached caller would be itself.
But this patch is good anyway, because it ensures Accessor(s) only
retain root AccessibleObjects which by itself is more memory-friendly.
About the tests:
- There seems to be some strange ununiform formatting in AccessTest.
- For public and protected members, the test looks good, but for
private members, I don't think it is doing anything useful, because if
access-check fails, the caller class is not cached (from
AccessibleObject):
Right.
private boolean slowVerifyAccess(Class<?> caller, Class<?> memberClass,
Class<?> targetClass, int modifiers)
{
if (!Reflection.verifyMemberAccess(caller, memberClass,
targetClass, modifiers)) {
// access denied
return false;
}
...
...
securityCheckCache = cache; // write volatile
return true;
}
But for private members, there's no issue of leaking caller class(es)
because the only successfully caller is the declaring class itself.
including the private method/field is solely for completeness (still
partial since it does not cover the constructor). I can add a comment
to mention that it won't cache caller for such access failing case.
Mandy
