RFR: 8212117 : Class.forName may return a reference to a loaded but not linked Class

[Brent Christian]

Hi, Mandy

On 9/5/19 6:03 PM, Mandy Chung wrote:
> On 9/5/19 5:09 PM, Brent Christian wrote:
> 
> jvm.h
> 
>   349  * Link the 'arg' class (unless ClassForNameDeferLinking is set)
> 
> I suggest to drop "(unless ...)" phrase because the flag is temporary.

Sure, good idea.

> jni.cpp
> 
> The implementation of JNI FindClass has the same behavior as 1-arg
> Class.forName(name).  The JNI spec needs fixing but it's a separate
> issue.

Right - looks like David filed JDK-8230685.

> find_class_from_class_loader
>    I like David's suggestion to assert that if init == true, link
>    must be true.  Alternatively, pass an enum instead of two booleans
>    clearly tell linking or initializing.

Yup, assert added.

> Lookup::findClass:
> 
> "In particular, the method first attempts to load the requested class"
> 
> I think this sentence is no longer needed together with your change.  What about:
> 
>           /**
> -         * Looks up a class by name from the lookup context defined by this {@code Lookup} object. The static
> -         * initializer of the class is not run.
> +         * Looks up a class by name from the lookup context defined by this {@code Lookup} object.
> +         * This method attempts to locate, load, and link the class, and then determines whether
> +         * the class is accessible to this {@code Lookup} object.
> +         * The static initializer of the class is not run.
>            * <p>
>            * The lookup context here is determined by the {@linkplain #lookupClass() lookup class}, its class
> -         * loader, and the {@linkplain #lookupModes() lookup modes}. In particular, the method first attempts to
> -         * load the requested class, and then determines whether the class is accessible to this lookup object.
> +         * loader, and the {@linkplain #lookupModes() lookup modes}.

Looks good to me.

> The note you added in this method should also be added to 2-arg
> Class::forName for consistency.

OK, sure.


Update webrev is here:
   http://cr.openjdk.java.net/~bchristi/8212117/webrev11/
with changes to jvm.h, MethodHandles.java, and Class.java.

I went ahead and put together a specdiff[1] for the changed methods 
([2][3][4]).  I've updated and finalized the CSR.

Thanks for the review,
-Brent

1. 
http://cr.openjdk.java.net/~bchristi/8212117/webrev11-specdiff/specdiff-summary.html

2. 
http://cr.openjdk.java.net/~bchristi/8212117/webrev11-specdiff/Class-report.html#forName(java.lang.String,boolean,java.lang.ClassLoader)

3. 
http://cr.openjdk.java.net/~bchristi/8212117/webrev11-specdiff/Class-report.html#forName(java.lang.Module,java.lang.String)

4. 
http://cr.openjdk.java.net/~bchristi/8212117/webrev11-specdiff/invoke/MethodHandles.Lookup-report.html#findClass(java.lang.String)