RFR: 8065634: Crash in InstanceKlass::clean_method_data when _method is NULL

[Coleen Phillimore]

I agree, this is good.
Coleen

On 12/5/14, 9:06 AM, Stefan Karlsson wrote:
> On 2014-12-05 09:04, harold seigel wrote:
>> Hi Stefan,
>>
>> Thanks for restoring the 'else' and the 'nullHandle'.  It looks good!
>
> Thanks!
>
> StefanK
>
>>
>> Harold
>>
>> On 12/5/2014 8:51 AM, Stefan Karlsson wrote:
>>> Hi Harold,
>>>
>>> Thanks for looking at the patch. My first patch used nullHandle and 
>>> did the freeing was done inside an else statement, but during a 
>>> pre-review of the patch I got feedback to change it to CHECK_NULL 
>>> and get rid of the else statement. :)
>>>
>>> Here's a new webrev with nullHandle and the else statement:
>>> http://cr.openjdk.java.net/~stefank/8065634/webrev.02
>>>
>>> Thanks,
>>> StefanK
>>>
>>> On 2014-12-03 14:31, harold seigel wrote:
>>>> Hi Stefan,
>>>>
>>>> The change looks good but I have a couple of questions.
>>>>
>>>> 1. Why does the call to create_combined_annotations(), in 
>>>> parseClassFile(), use CHECK_NULL when other calls use 
>>>> CHECK_(nullHandle) ?
>>>>
>>>> 4021     ClassAnnotationCollector parsed_annotations;
>>>> 4022     parse_classfile_attributes(&parsed_annotations, 
>>>> CHECK_(nullHandle));
>>>> 4023
>>>> 4024     // Finalize the Annotations metadata object,
>>>> 4025     // now that all annotation arrays have been created.
>>>> 4026     create_combined_annotations(CHECK_NULL);
>>>> 4027
>>>> 4028     // Make sure this is the end of class file stream
>>>> 4029     guarantee_property(cfs->at_eos(), "Extra bytes at the end 
>>>> of class file %s", CHECK_(nullHandle));
>>>>
>>>>
>>>> 2. In ~ClassFileParser(), does MetadataFactory::free_array() and 
>>>> Annotations::free_contents() need to be called if the *_annotation 
>>>> fields are NULL?
>>>>
>>>>
>>>> Thanks, Harold
>>>>
>>>> On 12/3/2014 9:21 AM, Stefan Karlsson wrote:
>>>>> Hi all,
>>>>>
>>>>> Please, review this patch to fix an issue where the GC encounters 
>>>>> an InstanceKlass before it has come far enough in the class 
>>>>> loading process.
>>>>>
>>>>> Some background to the fix:
>>>>>
>>>>> When the JVM loads classes it has to keep track of the allocated 
>>>>> Metadata, and in the event of an exception deallocate the 
>>>>> Metadata. The ClassFileParser saves the Metadata pointers so that 
>>>>> it knows what needs to be deallocated. When the class loading has 
>>>>> come far enough there's a point were the GC can take over the 
>>>>> ownership of the Metadata and do deallocation of the both the 
>>>>> InstanceKlass and the Metadata, if needed. At that point the 
>>>>> Metadata pointers in the ClassFileParser are cleared and 
>>>>> transfered over to the InstanceKlass.
>>>>>
>>>>> Most Metadata allocations happen before the InstanceKlass is 
>>>>> allocated, but the Annotations are allocated after the 
>>>>> InstanceKlass but before the transferal and clearing of the 
>>>>> Metadata pointers in the ClassFileParser. If the GC stops when 
>>>>> allocating the Annotations, it will likely find this InstanceKlass 
>>>>> and assume that the Metadata are correctly setup. This is exactly 
>>>>> what's happening in the bug report. The GC finds that the _methods 
>>>>> array, unexpectedly, points to NULL.
>>>>>
>>>>> The proposed solution to this problem is to allocate the 
>>>>> Annotations before the InstanceKlass is allocated.
>>>>>
>>>>> http://cr.openjdk.java.net/~stefank/8065634/webrev.01
>>>>> https://bugs.openjdk.java.net/browse/JDK-8065634
>>>>>
>>>>> Testing:
>>>>> JPRT, parallel_class_loading testlist, Aurora adhoc run
>>>>>
>>>>> Thanks,
>>>>> StefanK
>>>>
>>>
>>
>