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

[Stefan Karlsson]

Hi Poonam,

Thanks for reviewing.

On 2014-12-04 17:19, Poonam Bajaj Parhar wrote:
> 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

I'll change the comment before pushing.

>
> 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.

It's a good suggestion, but that change itself is around two times 
larger in the number of changed lines, so I don't want to do it as a 
part of this bug fix. I've created the following bug:
https://bugs.openjdk.java.net/browse/JDK-8066774

Thanks,
StefanK

>
> 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
>>>>
>>>
>>
>