RFR: 8187577: JVM crash during gc doing concurrent marking

[Kim Barrett]

> On Feb 7, 2018, at 9:39 AM, Poonam Parhar <poonam.bajaj at oracle.com> wrote:
> 
> Hello Coleen, Kim,
> 
> Thanks for your review! I moved ensure_klass_alive() to jvmtiGetLoadedClasses.cpp. Please take a look at the following webrev:
> 
> http://cr.openjdk.java.net/~poonam/8187577/webrev.01/

A couple minor issues:

- ensure_klass_alive isn't indented properly.

- And as was already pointed out by others, the #include needs
INCLUDE_ALL_GCS protection.

And a couple of even more minor stylistic items that you can change or
ignore. 

- Not sure why the private access specifier was added, since it's
already in a private section.

- The call to ensure_klass_alive doesn't need to be qualified. 

Otherwise, looks good. I don't need a new webrev for any of the above.

> 
> Thanks,
> Poonam
> 
> On 2/6/2018 3:23 PM, coleen.phillimore at oracle.com wrote:
>> 
>> Hi Poonam,
>> 
>> Kim and I discussed this fix and we think it's correct for this case, but can you move ensure_klass_alive out of Klass and into a local static function in jvmtiGetLoadedClasses.cpp?   We don't       want this to be the normal way to do things in jdk11 but there are similar local functions in jdk10 and 8 for your backports, so this would be like those ones.
>> 
>> Long term, Kim convinced me that Stefan is right and we have to use the AccessAPI to keep alive class loaders (or holders) when classes and other things are iterated through the ClassLoaderDataGraph.   I'll make this change at some point soon when I do some other ClassLoaderDataGraph changes.
>> 
>> Thanks,
>> Coleen
>> 
>> On 2/2/18 4:22 PM, Poonam Parhar wrote:
>>> Adding Coleen to this RFR.
>>> On 2/2/2018 11:41 AM, Poonam Parhar wrote:
>>>> Hello,
>>>> 
>>>> Please review this fix for a G1GC crash occurring during concurrent marking.
>>>> 
>>>> Bug: JDK-8187577: JVM crash during gc doing concurrent marking
>>>> Webrev: http://cr.openjdk.java.net/~poonam/8187577/webrev.00/
>>>> 
>>>> Problem:
>>>> A klass that has been considered unreachable by G1, can be looked up in the CLD/SD, and its _java_mirror or _class_loader can be stored in a root or any other reachable object making it alive again. Whenever a klass is resurrected in this manner, the SATB part of G1 needs to be notified about this, otherwise the concurrent marking remark phase will wrongly unload that klass.
>>>> 
>>>> In this particular crash, while a G1 is doing concurrent marking and has prepared its list of unreachable classes, JVMTI on a Java thread traverses classes in the CLD and stores thread-local JNIHandles for the java_mirror of the loaded classes. G1 does not have knowledge of these thread-local JNIHandles, and in the remark phase it unloads classes as per its prior knowledge of unreachable classes. But when these JNIHandles are later scanned, it leads to a crash.
>>>> 
>>>> Solution:
>>>> Inform G1's SATB that a klass has been resurrected and it should not be unloaded.
>>>> 
>>>> Testing:
>>>> - Customer verification testing
>>>> - mach5 jdk-tier1-3, hs-tier1-2
>>>> 
>>>> Thanks,
>>>> Poonam