RFR(S): 8200466: Revisit the setting of _transitive_interfaces in InstanceKlass

[coleen.phillimore at oracle.com]

On 4/30/18 4:58 PM, Calvin Cheung wrote:
> Hi Coleen,
>
> Thanks for your review.
>
> On 4/30/18, 10:23 AM, coleen.phillimore at oracle.com wrote:
>>
>> This sharing of metadata has been prone to bugs.  Can you have the 
>> same bug with the _secondary_super field? 
>> InstanceKlass::deallocate_contents() checks:
>>
>>       secondary_supers() != transitive_interfaces() &&
>>
>> and will free the field possibly from the super class if 
>> transitive_interfaces() is still NULL.  (If I understand this 
>> correctly).
> If it passes the conditions check in the 'if' block, it will 
> deallocate the _secondary_supers.
> It maybe fine because the crash was in visit_all_interfaces() where it 
> doesn't dereference the _secondary_supers.

It seems like it would deallocate _secondary_supers if an OOM occurred 
after it's set to the super's transitive_interfaces and before 
_transitive_interfaces is set.  There could be a state that could cause 
this to happen.  I would have to trace through to see what sort of crash 
would happen.

>>
>> Wouldn't it be easier just to eagerly set the super class, and keep 
>> the call to initialize_supers() where it is?
> Do you mean adding the following in 
> ClassFileParser::apply_parsed_class_metadata() and reverting the rest 
> of the changes?
>
> this_klass->set_super(const_cast<Klass*>(_super_klass));
>

Yes, can you try this?  This line with a comment would be less 
complicated than passing NULL to initialize_supers() for the arrayKlass 
versions.


> BTW, I think the first arg to initialize_supers() should be casted to 
> Klass* like above because the _super_klass is already an InstanceKlass*.
>
> ik->initialize_supers(const_cast<InstanceKlass*>(_super_klass) ...
>
>
I think we still need some cast here and above because _super_klass is 
defined as const, and this function and set_super are both not 
const-safe, which is a bigger change.   Or you could make 
ClassFileParser::_super_klass not const since const is cast away.

>>
>> http://cr.openjdk.java.net/~ccheung/8200466/webrev.00/src/hotspot/share/classfile/classFileParser.cpp.udiff.html 
>>
>>
>> **** 5884,5893 *****
>> *--- 5889,5899 ----*
>> *Annotations::free_contents(_loader_data, _fields_annotations);*
>> *Annotations::free_contents(_loader_data, _fields_type_annotations);*
>> *}*
>> **
>> *clear_class_metadata();*
>> *+ _transitive_interfaces = NULL;*
>> **
>>
>> clear_class_metadata() already sets the _transitive_interfaces to 
>> NULL, so this shouldn't be needed.
> The current change does not set the _trasitive_interfaces to NULL in 
> clear_class_metadata().
>
> From the udiff above:
>
> **** 5832,5842 *****
> *--- 5838,5847 ----*
> *     _cp = NULL;*
> *     _fields = NULL;*
> *     _methods = NULL;*
> *     _inner_classes = NULL;*
> *     _local_interfaces = NULL;*
> *     _transitive_interfaces = NULL;*
> *     _combined_annotations = NULL;*
> *     _annotations = _type_annotations = NULL;*
> *     _fields_annotations = _fields_type_annotations = NULL;*
> *   }*

Oh I see it now and why you changed it.  Can you see if setting the 
Klass::_super ahead of setting _transitive_interfaces would work better?

thanks,
Coleen

>
> thanks,
> Calvin
>>
>> thanks,
>> Coleen
>>
>> On 4/27/18 12:46 AM, Calvin Cheung wrote:
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8200466
>>>
>>> webrev: http://cr.openjdk.java.net/~ccheung/8200466/webrev.00/
>>>
>>> This bug was discovered during the investigation of JDK-8200078 
>>> <https://bugs.openjdk.java.net/browse/JDK-8200078>.
>>>
>>> I've included the following comment in classFileParser.cpp to 
>>> summarize the change:
>>>
>>> // Delay the setting of _transitive_interfaces until after 
>>> initialize_supers() in
>>> // fill_instance_klass(). It is because the _transitive_interfaces 
>>> may be shared with
>>> // its _super. If an OOM occurs while loading the current klass, its 
>>> _super field
>>> // may not have been set. When GC tries to free the klass, the 
>>> _transitive_interfaces
>>> // may be deallocated mistakenly in 
>>> InstanceKlass::deallocate_interfaces(). Subsequent
>>> // dereferences to the deallocated _transitive_interfaces will 
>>> result in a crash.
>>>
>>> Testing: (on Oracle platforms)
>>>     hs-tier{1,2,3}
>>>     closed (soon will be open) PCL (parallel class loading) tests
>>>
>>> thanks,
>>> Calvin
>>