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

[David Holmes]

On 8/09/2016 7:52 PM, serguei.spitsyn at oracle.com wrote:
> 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.

Okay so ... that means this:

JvmtiEnv::AddToSystemClassLoaderSearch(const char* segment) {
   jvmtiPhase phase = get_phase();

   if (phase == JVMTI_PHASE_ONLOAD) {
     for (SystemProperty* p = Arguments::system_properties(); p != NULL; 
p = p->next()) {
       if (strcmp("java.class.path", p->key()) == 0) {
         p->append_value(segment);
         break;
       }
     }
     return JVMTI_ERROR_NONE;

is now dead code as we never call this in the ONLOAD phase?

Cheers,
David
-----

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