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

[harold seigel]

Hi,

How about adding an 'else' statement at line 4342?  That would make the 
code consistent with the comments at lines 4329-4330 and 4343-4344.

4328   if (_combined_annotations != NULL) {
4329     // After all annotations arrays have been created, they are installed into the
4330     // Annotations object that will be assigned to the InstanceKlass being created.
4331
4332     // Deallocate the Annotations object and the installed annotations arrays.
4333     _combined_annotations->deallocate_contents(_loader_data);
4334
4335     // If the _combined_annotations pointer is non-NULL,
4336     // then the other annotations fields should have been cleared.
4337     assert(_annotations             == NULL, "Should have been cleared");
4338     assert(_type_annotations        == NULL, "Should have been cleared");
4339     assert(_fields_annotations      == NULL, "Should have been cleared");
4340     assert(_fields_type_annotations == NULL, "Should have been cleared");
4341   }
4342
4343   // If the annotations arrays were not installed into the Annotations object,
4344   // then they have to be deallocated explicitly.
4345   MetadataFactory::free_array<u1>(_loader_data, _annotations);
4346   MetadataFactory::free_array<u1>(_loader_data, _type_annotations);
4347   Annotations::free_contents(_loader_data, _fields_annotations);
4348   Annotations::free_contents(_loader_data, _fields_type_annotations);

Thanks, Harold

On 12/3/2014 4:36 PM, Coleen Phillimore wrote:
>
> On 12/3/14, 2:31 PM, 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));
>
> Oh, this is my fault. https://bugs.openjdk.java.net/browse/JDK-8066624
> If it looks inconsistent, maybe it can be changed to CHECK_(nullHandle);
>>
>>
>> 2. In ~ClassFileParser(), does MetadataFactory::free_array() and 
>> Annotations::free_contents() need to be called if the *_annotation 
>> fields are NULL?
>
> free_array() checks for null and is inlined so there isn't another 
> need for checking if the Array is null first.
>
> This change looks good to me.
>
> Coleen
>>
>>
>> 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
>>
>