Review request for 8021368: Launch of Java Web Start app fails with ClassCircularityError exception

[Alan Bateman]

On 12/12/2013 18:29, Mandy Chung wrote:
> JDK-8021368: Launch of Java Web Start app fails with 
> ClassCircularityError exception in 7u25
> https://bugs.openjdk.java.net/browse/JDK-8021368
>
> This is a fix for 7u60 only.  It's a regression in 7u25 due to 
> JDK-8010117 where it calls Class.getMethod to determine if the 
> checkMemberAccess method is overridden in a SecurityManager subclass 
> but Class.getMethod causes parameter types and returned type of other 
> public methods to be resolved and hence ClassCircularityError.  It is 
> not an issue in JDK 8 as SecurityManager.checkMemberAccess is 
> deprecated and not called by the JDK (see JDK-8007035).
>
> Webrev at:
> http://cr.openjdk.java.net/~mchung/jdk7u/webrevs/8021368/webrev.00/
>
> An alternative implementation is to add a new VM entry point to look 
> up the declaring class of an overridden method instead of using 
> JNI_GetMethodID and get a reflective method for a faster query. Since 
> this check is only performed once in most cases, this performance cost 
> of using JNI is not too bad that the new VM entry point doesn't 
> necessarily buy much more.

I looked at changes and the approach seems okay to me (at least I can't 
of other ways to check for the override without also tickling the issue. 
I think you are right that a special entry point for this is excessive 
given that the result can be cached.

A minor point on the SecurityManagerHelper constructor, shouldn't it be:

boolean overridden = false;
if (smgr.getClass() != SecurityManager.class) {
     try { overridden = ... }
}

A minor comment on naming where "cache" seems a bit general given that 
Class has several caches (I know useCaches is general too). Also I see 
the new code is using "smgr" whereas everywhere every seems to be using 
"sm", "security" or other.

-Alan.