8221351: Crash in KlassFactory::check_shared_class_file_load_hook

Reviewed-by: clanger
Backport-of: e2ffa15

src/hotspot/share/classfile/classLoader.cpp

@@ -483,14 +483,18 @@ ClassPathImageEntry::~ClassPathImageEntry() {
   }
 }
 
+ClassFileStream* ClassPathImageEntry::open_stream(const char* name, TRAPS) {
+  return open_stream_for_loader(name, ClassLoaderData::the_null_class_loader_data(), THREAD);
+}
+
 // For a class in a named module, look it up in the jimage file using this syntax:
 //    /<module-name>/<package-name>/<base-class>
 //
 // Assumptions:
 //     1. There are no unnamed modules in the jimage file.
 //     2. A package is in at most one module in the jimage file.
 //
-ClassFileStream* ClassPathImageEntry::open_stream(const char* name, TRAPS) {
+ClassFileStream* ClassPathImageEntry::open_stream_for_loader(const char* name, ClassLoaderData* loader_data, TRAPS) {
   jlong size;
   JImageLocationRef location = (*JImageFindResource)(_jimage, "", get_jimage_version_string(), name, &size);
 
@@ -501,20 +505,8 @@ ClassFileStream* ClassPathImageEntry::open_stream(const char* name, TRAPS) {
     if (pkg_name != NULL) {
       if (!Universe::is_module_initialized()) {
         location = (*JImageFindResource)(_jimage, JAVA_BASE_NAME, get_jimage_version_string(), name, &size);
-#if INCLUDE_CDS
-        // CDS uses the boot class loader to load classes whose packages are in
-        // modules defined for other class loaders.  So, for now, get their module
-        // names from the "modules" jimage file.
-        if (DumpSharedSpaces && location == 0) {
-          const char* module_name = (*JImagePackageToModule)(_jimage, pkg_name);
-          if (module_name != NULL) {
-            location = (*JImageFindResource)(_jimage, module_name, get_jimage_version_string(), name, &size);
-          }
-        }
-#endif
-
       } else {
-        PackageEntry* package_entry = ClassLoader::get_package_entry(name, ClassLoaderData::the_null_class_loader_data(), CHECK_NULL);
+        PackageEntry* package_entry = ClassLoader::get_package_entry(name, loader_data, CHECK_NULL);
         if (package_entry != NULL) {
           ResourceMark rm;
           // Get the module name

src/hotspot/share/classfile/classLoader.hpp

@@ -61,6 +61,10 @@ class ClassPathEntry : public CHeapObj<mtClass> {
   // Attempt to locate file_name through this class path entry.
   // Returns a class file parsing stream if successfull.
   virtual ClassFileStream* open_stream(const char* name, TRAPS) = 0;
+  // Open the stream for a specific class loader
+  virtual ClassFileStream* open_stream_for_loader(const char* name, ClassLoaderData* loader_data, TRAPS) {
+    return open_stream(name, THREAD);
+  }
   // Debugging
   NOT_PRODUCT(virtual void compile_the_world(Handle loader, TRAPS) = 0;)
 };
@@ -140,7 +144,7 @@ class ClassPathImageEntry: public ClassPathEntry {
   ClassPathImageEntry(JImageFile* jimage, const char* name);
   virtual ~ClassPathImageEntry();
   ClassFileStream* open_stream(const char* name, TRAPS);
-
+  ClassFileStream* open_stream_for_loader(const char* name, ClassLoaderData* loader_data, TRAPS);
   // Debugging
   NOT_PRODUCT(void compile_the_world(Handle loader, TRAPS);)
 };

src/hotspot/share/classfile/klassFactory.cpp

@@ -58,7 +58,7 @@ InstanceKlass* KlassFactory::check_shared_class_file_load_hook(
     // Post the CFLH
     JvmtiCachedClassFileData* cached_class_file = NULL;
     if (cfs == NULL) {
-      cfs = FileMapInfo::open_stream_for_jvmti(ik, CHECK_NULL);
+      cfs = FileMapInfo::open_stream_for_jvmti(ik, class_loader, CHECK_NULL);
     }
     unsigned char* ptr = (unsigned char*)cfs->buffer();
     unsigned char* end_ptr = ptr + cfs->length();

src/hotspot/share/memory/filemap.cpp

@@ -24,7 +24,9 @@
 
 #include "precompiled.hpp"
 #include "jvm.h"
+#include "classfile/classFileStream.hpp"
 #include "classfile/classLoader.inline.hpp"
+#include "classfile/classLoaderData.inline.hpp"
 #include "classfile/classLoaderExt.hpp"
 #include "classfile/compactHashtable.inline.hpp"
 #include "classfile/stringTable.hpp"
@@ -1490,7 +1492,7 @@ ClassPathEntry* FileMapInfo::get_classpath_entry_for_jvmti(int i, TRAPS) {
   return ent;
 }
 
-ClassFileStream* FileMapInfo::open_stream_for_jvmti(InstanceKlass* ik, TRAPS) {
+ClassFileStream* FileMapInfo::open_stream_for_jvmti(InstanceKlass* ik, Handle class_loader, TRAPS) {
   int path_index = ik->shared_classpath_index();
   assert(path_index >= 0, "should be called for shared built-in classes only");
   assert(path_index < (int)_shared_path_table_size, "sanity");
@@ -1502,7 +1504,12 @@ ClassFileStream* FileMapInfo::open_stream_for_jvmti(InstanceKlass* ik, TRAPS) {
   const char* const class_name = name->as_C_string();
   const char* const file_name = ClassLoader::file_name_for_class_name(class_name,
                                                                       name->utf8_length());
-  return cpe->open_stream(file_name, THREAD);
+  ClassLoaderData* loader_data = ClassLoaderData::class_loader_data(class_loader());
+  ClassFileStream* cfs = cpe->open_stream_for_loader(file_name, loader_data, THREAD);
+  assert(cfs != NULL, "must be able to read the classfile data of shared classes for built-in loaders.");
+  log_debug(cds, jvmti)("classfile data for %s [%d: %s] = %d bytes", class_name, path_index,
+                        cfs->source(), cfs->length());
+  return cfs;
 }
 
 #endif

src/hotspot/share/memory/filemap.hpp

@@ -300,7 +300,7 @@ class FileMapInfo : public CHeapObj<mtInternal> {
   static void update_shared_classpath(ClassPathEntry *cpe, SharedClassPathEntry* ent, TRAPS);
 
 #if INCLUDE_JVMTI
-  static ClassFileStream* open_stream_for_jvmti(InstanceKlass* ik, TRAPS);
+  static ClassFileStream* open_stream_for_jvmti(InstanceKlass* ik, Handle class_loader, TRAPS);
 #endif
 
   static SharedClassPathEntry* shared_path(int index) {

test/hotspot/jtreg/TEST.groups

@@ -326,6 +326,21 @@ tier1_runtime_appcds_exclude = \
   runtime/appcds/ \
   -:tier1_runtime_appcds
 
+# This group should be executed with "jtreg -Dtest.cds.run.with.jfr=true ..."
+# to test interaction between AppCDS and JFR. It also has the side effect of
+# testing JVMTI ClassFileLoadHook.
+#
+# The excluded tests disallow the jdk.jfr module, which is required to
+# run with JFR.
+hotspot_appcds_with_jfr = \
+  runtime/appcds/ \
+ -runtime/appcds/cacheObject/ArchivedModuleCompareTest.java \
+ -runtime/appcds/jigsaw/classpathtests/BootAppendTests.java \
+ -runtime/appcds/jigsaw/classpathtests/ClassPathTests.java \
+ -runtime/appcds/jigsaw/classpathtests/EmptyClassInBootClassPath.java \
+ -runtime/appcds/jigsaw/JigsawOptionsCombo.java \
+ -runtime/appcds/jigsaw/modulepath/MainModuleOnly.java
+
 tier1_serviceability = \
   serviceability/dcmd/compiler \
   -serviceability/dcmd/compiler/CompilerQueueTest.java \

test/hotspot/jtreg/runtime/appcds/TestCommon.java

@@ -151,6 +151,15 @@ public static OutputAnalyzer createArchive(AppCDSOptions opts)
         return executeAndLog(pb, "dump");
     }
 
+    // This allows you to run the AppCDS tests with JFR enabled at runtime (though not at
+    // dump time, as that's uncommon for typical AppCDS users).
+    //
+    // To run in this special mode, add the following to your jtreg command-line
+    //    -Dtest.cds.run.with.jfr=true
+    //
+    // Some AppCDS tests are not compatible with this mode. See the group
+    // hotspot_appcds_with_jfr in ../../TEST.ROOT for details.
+    private static final boolean RUN_WITH_JFR = Boolean.getBoolean("test.cds.run.with.jfr");
 
     // Execute JVM using AppCDS archive with specified AppCDSOptions
     public static OutputAnalyzer runWithArchive(AppCDSOptions opts)
@@ -172,6 +181,22 @@ public static OutputAnalyzer runWithArchive(AppCDSOptions opts)
 
         for (String s : opts.suffix) cmd.add(s);
 
+        if (RUN_WITH_JFR) {
+            boolean usesJFR = false;
+            for (String s : cmd) {
+                if (s.startsWith("-XX:StartFlightRecording=") || s.startsWith("-XX:FlightRecorderOptions")) {
+                    System.out.println("JFR option might have been specified. Don't interfere: " + s);
+                    usesJFR = true;
+                    break;
+                }
+            }
+            if (!usesJFR) {
+                System.out.println("JFR option not specified. Enabling JFR ...");
+                cmd.add(0, "-XX:StartFlightRecording=dumponexit=true");
+                System.out.println(cmd);
+            }
+        }
+
         ProcessBuilder pb = ProcessTools.createJavaProcessBuilder(true, cmd);
         return executeAndLog(pb, "exec");
     }

test/hotspot/jtreg/runtime/appcds/customLoader/HelloCustom.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2015, 2017, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2015, 2019, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -28,8 +28,6 @@
  * @requires vm.cds
  * @requires vm.cds.custom.loaders
  * @library /test/lib /test/hotspot/jtreg/runtime/appcds
- * @modules java.base/jdk.internal.misc
- *          java.management
  * @compile test-classes/Hello.java test-classes/CustomLoadee.java
  * @build sun.hotspot.WhiteBox
  * @run driver ClassFileInstaller -jar hello.jar Hello
@@ -43,6 +41,9 @@
 
 public class HelloCustom {
     public static void main(String[] args) throws Exception {
+        run();
+    }
+    public static void run(String... extra_runtime_args) throws Exception {
         String wbJar = ClassFileInstaller.getJarPath("WhiteBox.jar");
         String use_whitebox_jar = "-Xbootclasspath/a:" + wbJar;
 
@@ -62,11 +63,12 @@ public static void main(String[] args) throws Exception {
                             use_whitebox_jar);
 
         output = TestCommon.exec(appJar,
-                                 // command-line arguments ...
-                                 use_whitebox_jar,
-                                 "-XX:+UnlockDiagnosticVMOptions",
-                                 "-XX:+WhiteBoxAPI",
-                                 "Hello", customJarPath);
+                                 TestCommon.concat(extra_runtime_args,
+                                     // command-line arguments ...
+                                     use_whitebox_jar,
+                                     "-XX:+UnlockDiagnosticVMOptions",
+                                     "-XX:+WhiteBoxAPI",
+                                     "Hello", customJarPath));
         TestCommon.checkExec(output);
     }
 }

test/hotspot/jtreg/runtime/appcds/jigsaw/modulepath/ModulePathAndCP.java

@@ -93,6 +93,10 @@ public static void buildTestModule() throws Exception {
     }
 
     public static void main(String... args) throws Exception {
+        run();
+    }
+
+    public static void run(String... extra_runtime_args) throws Exception {
         // compile the modules and create the modular jar files
         buildTestModule();
         String appClasses[] = {MAIN_CLASS, APP_CLASS};
@@ -104,6 +108,7 @@ public static void main(String... args) throws Exception {
                                         "-m", MAIN_MODULE);
         TestCommon.checkDump(output);
         String prefix[] = {"-cp", "\"\"", "-Xlog:class+load=trace"};
+        prefix = TestCommon.concat(prefix, extra_runtime_args);
 
         // run with the archive with the --module-path the same as the one during
         // dump time. The classes should be loaded from the archive.

test/hotspot/jtreg/runtime/appcds/jvmti/ClassFileLoadHook.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2016, 2017, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2016, 2019, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -41,8 +41,14 @@ public enum TestCaseId {
         SHARING_ON_CFLH_ON
     }
 
-    public static void main(String args[]) {
+    public static void main(String args[]) throws Exception {
         TestCaseId testCase = TestCaseId.valueOf(args[0]);
+        test1(testCase);
+        test2(testCase);
+    }
+
+    // Test rewriting the classfile data using CFLH
+    static void test1(TestCaseId testCase) {
         WhiteBox wb = WhiteBox.getWhiteBox();
 
         System.out.println("====== ClassFileLoadHook.main():testCase = " + testCase);
@@ -81,6 +87,20 @@ public static void main(String args[]) {
         }
     }
 
+    // Test the loading of classfile data for non-boot shared classes from jrt:/xxx.
+    // See JDK-8221351.
+    static void test2(TestCaseId testCase) throws Exception {
+        WhiteBox wb = WhiteBox.getWhiteBox();
+        Class c = Class.forName("java.sql.SQLException"); // defined by platform class loader.
+
+        switch (testCase) {
+        case SHARING_ON_CFLH_OFF:
+        case SHARING_AUTO_CFLH_ON:
+        case SHARING_ON_CFLH_ON:
+            assertTrue(wb.isSharedClass(c), "must be shared");
+        }
+    }
+
     private static void assertTrue(boolean expr, String msg) {
         if (!expr)
             throw new RuntimeException(msg);

test/hotspot/jtreg/runtime/appcds/jvmti/ClassFileLoadHookTest.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2016, 2017, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2016, 2019, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -27,9 +27,6 @@
  * @summary Test jvmti class file loader hook interaction with AppCDS
  * @library /test/lib /test/hotspot/jtreg/runtime/appcds
  * @requires vm.cds
- * @modules java.base/jdk.internal.misc
- *          jdk.jartool/sun.tools.jar
- *          java.management
  * @build ClassFileLoadHook
  * @run main/othervm/native ClassFileLoadHookTest
  */
@@ -46,7 +43,8 @@ public class ClassFileLoadHookTest {
         "ClassFileLoadHook",
         "ClassFileLoadHook$TestCaseId",
         "ClassFileLoadHook$1",
-        "LoadMe"
+        "LoadMe",
+        "java/sql/SQLException"
     };
 
     public static void main(String[] args) throws Exception {