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

[serguei.spitsyn at oracle.com]

Hi David,


On 9/8/16 02:30, David Holmes wrote:
> Hi Serguei,
>
> To the review ...

Thank you for looking at this!

>
> 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.

Correct.

>
> I presume that we don't care about the  appending to java.class.path 
> anymore in this scenario?

Correct.
We do not care about compatibility here (suggested by both Alan and Mandy).
It is highlighted in the CCC.


> But that we still need that for the onAttach scenario?

No.
In the onAttach scenario the JPLIS agent makes a call to the 
appendClassPath in the live phase.
It is why there is no problem here.

>
> 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);

The suggestion is taken, thanks!
I was thinking about the same but failed complete it.

>
> but no big deal.
>
> Otherwise changes seem fine.

Thanks!
Serguei

>
> 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
>>