RFR(S): 8173743: Failures during class definition can lead to memory leaks in metaspace

[Coleen Phillimore]

On 2/7/17 9:13 AM, Volker Simonis wrote:
> Hi Mikael,
>
> On Tue, Feb 7, 2017 at 2:05 PM, Mikael Gerdin <mikael.gerdin at oracle.com> wrote:
>> Hi,
>>
>> On 2017-02-06 19:40, Volker Simonis wrote:
>>> Hi,
>>>
>>> can somebody please review the following change:
>>>
>>> http://cr.openjdk.java.net/~simonis/webrevs/2017/8173743.v1/
>>
>> Reading this I come to think of the other part of this problem:
>> failures during class byte stream parsing.
>>
>> Currently most of InstanceKlass::deallocate_contents is reproduced inside
>> ~ClassFileParser - but in a slightly different way.
>> It would be good to try to coalesce these at some point in the future
>> otherwise I think we are going to see even more resource leaks due to the
>> two separate implementations.
>>
> I totally agree. While doing this fix, I also looked at
> InstanceKlass::deallocate_contents() and was a little surprised that
> it seems it isn't really called anywhere until now. So yes, a cleanup
> in 10 would be good.

InstanceKlass::deallocate_contents is called by 
hotspot/src/share/vm/memory/metadataFactory.hpp template function 
free_metadata, and add_to_deallocate_list() is used for redefinition.  
Also, inside of ClassFileParser destructor, the individual InstanceKlass 
fields are freed but once the InstanceKlass is created, it should go 
through deallocate_contents.  This would be cleaner if we had an 
InstanceKlass created first, but we can't because until we've parsed the 
whole thing, and figured out how big to make the vtables and itables, we 
don't know how big to make it.

There may be more places to explicitly add an InstanceKlass to the 
deallocate list.  One instance in the bug was missed because at the 
time, the effort to make parallel class loading default was withdrawn.

Thanks,
Coleen

>
>> Again, I think your fix is good for 9 but I just want to share my thoughts
>> about what we might want to fix for 10+.
>>
> Thanks :)
>
>> Thanks
>> Mikael
>>
>>
>>> https://bugs.openjdk.java.net/browse/JDK-8173743
>>>
>>> It fixes some problems during class definitions where instance klasses
>>> can leak into the metaspace in cases where the class definition fails
>>> after the class was successfully loaded from the bytecode stream.
>>>
>>> There are actually two cases to consider:
>>>
>>> 1. If we load the same class several times into the same class loader,
>>> we will get LinkageErrors after the first successful load. But
>>> nevertheless we will first construct a new instanceKlass for each new
>>> load attempt and this instanceKlass will never be deleted.
>>>
>>> 2. If we have a parallel capable class loader, set
>>> -XX:-UnsyncloadClass and/or -XX:+AllowParallelDefineClass and load a
>>> class from several threads at the same time in parallel, it can happen
>>> that we create several instance klasses for the same class. At the end
>>> only one of them will be added to the system dictionary, but all the
>>> other ones will never be deleted. Notice that if we run this scenario
>>> without setting either of -XX:-UnsyncloadClass or
>>> -XX:+AllowParallelDefineClass, this scenario will degrade into the
>>> case above and we will get LinkageErrors for all but the first
>>> successful load.
>>>
>>> The change comes with a regression test which checks for the two cases
>>> just describe and also for the failing class redefinition case, which
>>> currently doesn't produce a memory leak.
>>>
>>> I've already committed this to the hs-demo-submit/hotspot/ forest and
>>> it went through without a problem. So in theory it should have passed
>>> the internal JPRT tests although I'm not sure if the test set of the
>>> "demo-submit" forest and the real hotspot repo are exactly the same
>>> (CC'ed Tim and Brian).
>>>
>>> Thank you and best regards,
>>> Volker
>>>