RFR(s) 8221351 Crash in KlassFactory::check_shared_class_file_load_hook

[Ioi Lam]

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