RFR: 8160950 Agent JAR added to app class loader rather than system class loader when running with -Djava.system.class.loader

[David Holmes]

Hi Serguei,

To the review ...

On 8/09/2016 5:27 AM, serguei.spitsyn at oracle.com wrote:
> Please, review the fix for:
>   https://bugs.openjdk.java.net/browse/JDK-8160950
>
> Webrev:
>
> http://cr.openjdk.java.net/~sspitsyn/webrevs/2016/jdk/8160950-JLI-WLS.jdk2/

src/java.instrument/share/native/libinstrument/InvocationAdapter.c

So we used to call appendClassPath during onLoad, which means we hit the 
code in JvmtiEnv::AddToSystemClassLoaderSearch that only appended to 
java.class.path. Now we call appendClassPath from eventHandlerVMInit, 
which is presumably live phase, and so we hit the code that actually 
invokes appendToClassPathForInstrumentation. Ok.

I presume that we don't care about the  appending to java.class.path 
anymore in this scenario? But that we still need that for the onAttach 
scenario?

Only nit with the changes is that there is some duplication in the error 
reporting. Given we have:

650             fprintf(stderr, "System class loader does not define "
651                 "the appendToClassPathForInstrumentation method\n");

then this:

  464             fprintf(stderr, "Unable to add %s to system class path - "
  465                     "the system class loader does not define the "
  466                     "appendToClassPathForInstrumentation method or 
the method failed\n",
  467                     agent->mJarfile);

could simply say:

              fprintf(stderr, "Unable to add %s to system class path\n"
                      agent->mJarfile);

but no big deal.

Otherwise changes seem fine.

Thanks,
David


>
> Summary:
>   When running a java application with the options
> `-javaagent:myagent.jar -Djava.system.classloader=MyClassLoader`
>   then the myagent.jar is added to the application class loader rather
> than the custom system class loader.
>   The problem was that the libinstrument Agent_OnLoad did not invoke the
>   custom system class loader method appendToClassPathForInstrumentation.
>   Instead, it added the agent's jar path into the system property
> "java.class.path".
>
>   The solution is to save the jar file path in the global structure and
> later use it to
>   invoke the custom system class loader's method
> appendToClassPathForInstrumentation.
>
>   This breaks a compatibility in the rare case if a custom system class
> loader
>   does not define the method appendToClassPathForInstrumentation.
>   The compatibility risk is low because using a custom system class
> loader is very rare
>   (little known feature). In addition, any custom class loader used in
> environments with
>   tools that load agents into running VMs already define this method.
>   In the unlikely event that there is a custom system class loader that
> does not define this
>   method, and it is used with a java agent specified on command line,
> then the error will be
>   clear as the VM will refuse to start. It is trivially fixed by
> defining the method.
>
>   The previous version of the fix was preliminary reviewed by Alan and
> Mandy.
>
> Testing:
>   Ran the jtreg java.lang.instrument test and new unit test
>
> Thanks,
>
> Serguei
>