RFR (S) 8175249: VMThread::run fails in VerifyBeforeExit : Universe::verify

[Jiangli Zhou]

Hi Ioi,

You are right. Thanks for pointing that out! I’ll try to construct a new test case. 

Regardless, Coleen’s change is correct. During unloading of a dead loader, it does dictionary()->do_unloading(is_alive_closure) for every other class loaders in the ClassLoaderDataGraph. If a loader does not have dictionary, then it doesn’t have a pd_set and there is no need to call do_unloading(is_alive_closure) for that particular loader.

Thanks!
Jiangli


> On Jan 25, 2018, at 11:34 AM, Ioi Lam <ioi.lam at oracle.com> wrote:
> 
> Hi Jiangli,
> 
> Your test uses "anonymous inner classes" at the Java language level. These classes are treated normally by the JVM. I.e., the class with the name "ClassForName$1" is entered into the system dictionary. "Anonymous" here simply means that the class is not explicitly named inside Java source code.
> 
> In the context of this bug, we are concerned about "anonymous classes" inside the JVM, which are the InstanceKlasses that are not entered into the system dictionary. You can see some discussion here: https://blogs.oracle.com/jrose/anonymous-classes-in-the-vm. They are created by calling the Java API Unsafe.defineAnonymousClass().
> 
> You can find some example test cases under test/hotspot/jtreg/runtime/defineAnonClass in the JDK repo.
> 
> Thanks
> - Ioi
> 
> On 1/24/18 5:12 PM, Jiangli Zhou wrote:
>> Hi Coleen,
>> 
>> I figured out how to add the anonymous class to the generated jar file properly in the jtreg test. Here is the diff with added anonymous that triggers access check. I verified the gc.log. The host PD is added for the anonymous class. The PD entries are cleared properly.
>> 
>> diff for ClassForName.java
>> ======================
>> 
>> 32,35d31
>> <     interface MyTest {
>> <         public void test();
>> <     }
>> <
>> 45c41,42
>> <             Class.forName(java.math.BigDecimal.class.getName(), false, ClassLoader.getSystemClassLoader());
>> ---
>>>             Class.forName(java.util.List.class.getName(), false,
>>>                           ClassLoader.getSystemClassLoader());
>> 49,59d45
>> <
>> <         MyTest mt = new MyTest() {
>> <             public void test() {
>> <                 try {
>> <                     Class.forName(java.math.BigInteger.class.getName(), false, ClassLoader.getSystemClassLoader());
>> <                 } catch (Throwable e) {
>> <                     e.printStackTrace();
>> <                 }
>> <            }
>> <         };
>> <         mt.test();
>> 
>> 
>> 
>> Diff for ProtectionDomainCacheTest.java
>> ================================
>> 
>> <         JarUtils.createJarFile(jarFilePath, testClassesDir, CLASSFILENAME, "ClassForName$1.class", "ClassForName$MyTest.class");
>> ---
>>>         JarUtils.createJarFile(jarFilePath, testClassesDir, CLASSFILENAME);
>> 77,80d76
>> <         Files.delete(classFile);
>> <         classFile = FileSystems.getDefault().getPath(TESTCLASSES, "ClassForName$1.class");
>> <         Files.delete(classFile);
>> <         classFile = FileSystems.getDefault().getPath(TESTCLASSES, "ClassForName$MyTest.class");
>> 
>> Thanks,
>> Jiangli
>> 
>>> On Jan 24, 2018, at 12:32 PM, coleen.phillimore at oracle.com wrote:
>>> 
>>> 
>>> Can you share this test?  It might be nice to add also.
>>> thanks,
>>> Coleen
>>> 
>>> On 1/24/18 3:22 PM, Jiangli Zhou wrote:
>>>>> On Jan 23, 2018, at 4:31 PM, David Holmes <david.holmes at oracle.com> wrote:
>>>>> 
>>>>> On 24/01/2018 12:55 AM, coleen.phillimore at oracle.com wrote:
>>>>>> On 1/23/18 1:52 AM, Jiangli Zhou wrote:
>>>>>>> Hi Coleen,
>>>>>>> 
>>>>>>> The approach looks very clean.
>>>>>>> 
>>>>>>> I have a question for the following in ClassLoaderDataGraph::do_unloading(), which may not be an issue. The ‘if' check is for anonymous class loader, which does not have dictionary. So do_unloading(is_alive_closure) is not happening during unloading of an anonymous class loader. Could a ‘pd’ for anonymous class being added to any other loaders’ pd_set? I’ll try to construct a test case.
>>>>>> Yes anonymous classes do not have dictionaries, so their pd_set list in the dictionary don't need to be cleaned out.
>>>>>> I don't know how you'd get a pd for an anonymous class added to other loader's pd_set, but this would clean them out too.  This seems hard to maybe impossible to do, but thank you for thinking of this further.   We don't put anonymous classes in the dictionaries so I think this is impossible.
>>>>> IIUC if an anonymous class loads another class via another CL then its PD will be associated with that other CL. But the PD for a VMAC is the PD of its host-class, so that can't become unreachable until the host-class is unreachable. So processing of the host-class will clean out the PD from the other CL.
>>>> David is right. I constructed a standalone test case and conformed that today. The host PD is used for an anonymous class. So there is no issue when the anonymous class is unloaded without going through do_unloading(is_alive_closure). As long as the host class is alive, the host PD is also alive. When the host PD is unloaded, the host PD is cleared from other loader’s pd_set with the do_unloading(is_alive_closure) work.
>>>> 
>>>> Thanks,
>>>> Jiangli
>>>>> David
>>>>> -----
>>>>> 
>>>>>> thanks,
>>>>>> Coleen
>>>>>>> 1294       if (data->dictionary() != NULL) {
>>>>>>> 1295 data->dictionary()->do_unloading(is_alive_closure);
>>>>>>> 1296       }
>>>>>>> Thanks,
>>>>>>> Jiangli
>>>>>>> 
>>>>>>>> On Jan 22, 2018, at 3:52 PM, coleen.phillimore at oracle.com <mailto:coleen.phillimore at oracle.com> wrote:
>>>>>>>> 
>>>>>>>> Summary: protection domain package access cache needs to be walked in unloading
>>>>>>>> 
>>>>>>>> Tested with mach5 tier1-5 on linux and windows, and with tests in the bug report.
>>>>>>>> 
>>>>>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8175249.01/webrev <http://cr.openjdk.java.net/%7Ecoleenp/8175249.01/webrev>
>>>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8175249
>>>>>>>> 
>>>>>>>> Thanks,
>>>>>>>> Coleen
>