RFR: 8203885: ConcurrentLocksDump::dump_at_safepoint() should not allocate array in resource area

[Per Liden]

Hi Kim,

On 05/29/2018 08:09 AM, Kim Barrett wrote:
>> On May 28, 2018, at 8:16 AM, Per Liden <per.liden at oracle.com> wrote:
>>
>> ConcurrentLocksDump::dump_at_safepoint() creates a GrowableArray, which gets allocated in a resource area. This array is than passed down a call chain, where it can't control that another ResourceMark isn't created. In the leaf of this call chain, a closure (FindInstanceClosure) is executed, which appends to the array, which means it might need to be resized. This doesn't work if a new ResourceMark has been created, since the array resize will happen in a nested ResourceArea context. As a result, the append operation fails in GenericGrowableArray::check_nesting().
>>
>> This has so far gone unnoticed because CollectedHeap::object_iterate() in existing collectors typically don't create new ResourceMarks. This is not true for ZGC (and potentially other concurrent collectors), which needs to walk thread stacks, which in turn requires a ResourceMark.
>>
>> The proposed fix is to make this array C Heap allocated.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8203885
>> Webrev: http://cr.openjdk.java.net/~pliden/8203885/webrev.0
>>
>> Testing: hs-tier{1,3}
>>
>> /Per
> 
> Looks good.

Thanks for reviewing!

> 
> I was going to ask why the GrowableArray needs to be heap allocated?
> Why not just
> 
>    GrowableArray<oop> aos_objects(INITIAL_ARRAY_SIZE, true);
>    ... s/aos_objects/&aos_objects/ ...
>    // delete aos_objects no longer needed
> 
> After some digging, probably because of this, from growableArray.hpp:
> 
> 119    assert(!on_C_heap() || allocated_on_C_heap(), "growable array must be on C heap if elements are");

I had the same reaction and initially made it stack allocated, but 
noticed that GrowableArray doesn't allow that :(

> 
> Conflating the allocation of the GrowableArray object with the
> allocation of the underlying array.  That seems wrong, but not a
> problem to be solved as part of this change.
> 

I agree. I suspect someone wanted to protect against potential problems 
with having different life cycles of the GrowableArray and the backing 
array. But this seems overly strict.

/Per