RFR(s) 8221351 Crash in KlassFactory::check_shared_class_file_load_hook

[David Holmes]

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