RFR: 8135322: ConstantPool::release_C_heap_structures not run in some circumstances

[Andreas Eriksson]

Thanks Coleen!

- Andreas

On 2016-03-31 01:06, Coleen Phillimore wrote:
>
> Andreas,
>
> Good find!  This seems like a safe fix.  It's unfortunate to have to 
> walk the deallocate list, but I think it's short enough and less work 
> in comparison to having to call free_C_heap_structures on all the 
> other classes in the dead class loader data.   And deallocate_contents 
> removes the scratch_class from the CLD _klasses list so won't walk it 
> twice.
>
> This is really good.
>
> Coleen
>
> On 3/30/16 8:12 AM, Andreas Eriksson wrote:
>> Hi,
>>
>> Please review this fix for 8135322: 
>> ConstantPool::release_C_heap_structures not run in some circumstances.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8135322
>> Webrev: http://cr.openjdk.java.net/~aeriksso/8135322/webrev.00
>>
>> The way this occurs is:
>>
>> 1. A ConstantPool, /merge_cp/, without a corresponding InstanceKlass is
>>    created during class redefinition.
>> 2. /Merge_cp/ is added to ClassLoaderData::_deallocate_list, but is not
>>    cleaned up by ClassLoaderDataGraph::do_unloading because the holding
>>    ClassLoaderData is already dead.
>> 3. ClassLoaderData::~ClassLoaderData is called, which usually runs
>>    InstanceKlass::release_C_heap_structures, which in turn will call
>>    ConstantPool::release_C_heap_structures. But since there is no
>>    corresponding InstanceKlass for /merge_cp/ it will never release its
>>    C heap structures.
>> 4. For JDK 8 this means we leak a Monitor (ConstantPool::_lock). For
>>    JDK 9 the Monitor has been removed, but we will miss doing a call to
>>    ConstantPool::unreference_symbols, which is probably not good either.
>>
>>
>> This change adds a call to free everything in the _deallocate_list 
>> first thing in ClassLoaderData::~ClassLoaderData.
>>
>> Note:
>> Running deallocate_contents() on everything in the list might be 
>> doing unnecessary work:
>> What we really would like to do is run release_C_heap_structures() on 
>> constant pools in the list that do not have a corresponding 
>> InstanceKlass. Not sure it's worth the effort to do the filtering, 
>> since I believe the _deallocate_list is usually not that long.
>>
>> Note 2:
>> After fixing in JDK 9 I'll backport this to JDK 8, where it has a 
>> more immediately visible effect.
>>
>> Regards,
>> Andreas
>>
>