RFR(s) 8221351 Crash in KlassFactory::check_shared_class_file_load_hook

[Ioi Lam]

Thanks David & Calvin for the code review!

- Ioi

On 3/27/19 3:32 PM, David Holmes wrote:
> Hi Ioi,
>
> This all looks good to me. Thanks for the additional test coverage.
>
> David
>
> On 28/03/2019 4:36 am, Ioi Lam wrote:
>> On 3/26/19 7:08 PM, David Holmes wrote:
>>> Hi Ioi,
>>>
>>> This all seems fine to me. Will this also now account for modules 
>>> loaded with the App loader or even a custom loader?
>>>
>>> Is there anywhere else we might assume we're dealing with the boot 
>>> loader?
>>>
>> Hi David,
>>
>> Thanks for the review.
>>
>> The new code in FileMapInfo::open_stream_for_jvmti is intended for 
>> only the 3 built-in loaders (boot/platform/app):
>>
>>    int path_index = ik->shared_classpath_index();
>>    assert(path_index >= 0, "should be called for shared built-in 
>> classes only");
>>
>> open_stream_for_jvmti is needed because these 3 loaders can load a 
>> shared class without first getting its classfile data.
>>
>> Custom loaders don't go through this path. A custom loader will first 
>> prepare the classfile data and pass that to the VM (using one of the 
>> JVM_DefineClass calls in jvm.cpp). The VM will do a fingerprint and 
>> name check on the classfile data to match it with a shared class 
>> (whose path_index  is -1). So we already have the classfile data.
>>
>> To test all the cases (the 3 built-in loaders, and custom loaders), 
>> I've added 2 more test cases to add 
>> -XX:StartFlightRecording=dumponexit=true to the command-line. This 
>> has the side effect of enabling ClassFileLoadHook so that 
>> open_stream_for_jvmti() is called.
>>
>> http://cr.openjdk.java.net/~iklam/jdk13/8221351-cds-jfr-jvmti-crash.v02/
>> http://cr.openjdk.java.net/~iklam/jdk13/8221351-cds-jfr-jvmti-crash.v02-delta/ 
>>
>>
>> In addition, I've added the hotspot_appcds_with_jfr test group, to 
>> run all AppCDS tests (122 of them) with 
>> -XX:StartFlightRecording=dumponexit=true. That should have pretty 
>> good coverage.
>>
>> (The reason of adding HelloCustom_JFR.java and 
>> ModulePathAndCP_JFR.java is to have a quick validation without too 
>> much jtreg command-line fiddling).
>>
>> I'll file a separate RFE to add the hotspot_appcds_with_jfr test 
>> group into tiered testing.
>>
>> Thanks
>> - Ioi
>>
>>
>>
>>
>>> Thanks,
>>> David
>>>
>>> On 27/03/2019 4:26 am, Ioi Lam wrote:
>>>> https://bugs.openjdk.java.net/browse/JDK-8221351
>>>> http://cr.openjdk.java.net/~iklam/jdk13/8221351-cds-jfr-jvmti-crash.v01/ 
>>>>
>>>>
>>>> The crash happens when we try to read the classfile data for the 
>>>> shared
>>>> class java.sql.SQLException. This ultimately goes to
>>>> ClassPathImageEntry::open_stream, which calls JImageFindResource.
>>>>
>>>> However, JImageFindResource needs to know the module_name ("java.sql")
>>>> for this class, but the module_name look up is done only using the 
>>>> boot
>>>> loader, which doesn't know about the java.sql module, which is 
>>>> loaded by the
>>>> built-in platform loader.
>>>>
>>>> The fix is to pass the defining class loader down to
>>>> ClassPathImageEntry::open_stream_for_loader, so that the 
>>>> module_name look-up
>>>> can be done using the correct loader.
>>>>
>>>> While fixing this, I also removed an obsolete "#if INCLUDE_CDS" in
>>>> classLoader.cpp. This was relic from the days when we still used 
>>>> the boot
>>>> loader to load platform/app classes. This code is no longer needed 
>>>> after
>>>> JDK-8186842 (Use Java class loaders for creating the CDS archive).
>>>>
>>>> Tested with tier-{1,2,3}
>>>>
>>>> Thanks
>>>> - Ioi
>>>>
>>