RFR(s) 8221351 Crash in KlassFactory::check_shared_class_file_load_hook

[Calvin Cheung]

Hi Ioi,

The updated webrev looks good.

thanks,
Calvin

On 3/27/19, 11: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
>>>
>