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

[Poonam Bajaj Parhar]

Hello Stefan,

The changes look good to me.

Perhaps the comment for the following code can be modified from:

3132     // The annotations below has been transfered the _combined_annotations
3133     // so these fields can now be cleared.
3134     _annotations             = NULL;
3135     _type_annotations        = NULL;
3136     _fields_annotations      = NULL;
3137     _fields_type_annotations = NULL;
3138 }

to

3132     // The annotations arrays below have been transferred to the _combined_annotations

Also, if possible, would be good if we can change the name of 
_annotations field to _class_annotations; more like the names of the 
other annotations arrays.

Thanks,
Poonam


On 12/4/2014 5:25 AM, harold seigel wrote:
> 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
>>>
>>
>