RFR 8201265: Native memory leak in ClassLoader::add_to_exploded_build_list

[Lois Foltan]

On 4/11/2018 8:38 AM, David Holmes wrote:

> On 11/04/2018 10:21 PM, Lois Foltan wrote:
>> On 4/11/2018 1:29 AM, David Holmes wrote:
>>
>>> On 11/04/2018 5:47 AM, harold seigel wrote:
>>>> Hi Lois,
>>>>
>>>> Thanks for the review.
>>>>
>>>> I do not think that an EXCEPTION_MARK is needed.  Method 
>>>> add_to_exploded_build_list(..., TRAPS) will just propagate 
>>>> exceptions back to its caller.
>>>
>>> Also if you did add an EXCEPTION_MARK prior to line 870 then it 
>>> would trigger an abort if the potential exception from:
>>>
>>> 880     ClassPathEntry* new_entry = create_class_path_entry(path, 
>>> &st, false, false, CHECK);
>>>
>>> eventuated.
>> Hmm, isn't that what we want since this setup occurs during 
>> bootstrapping?  Just trying to understand why the 2 other methods, 
>> ClassLoader::setup_patch_mod_entries and 
>> ClassLoader::setup_boot_search_path, both have EXCEPTION_MARKs at 
>> line #752 & #816 respectively ahead of calls to 
>> create_class_path_entry in code that is pretty much identical to the 
>> code in add_to_exploded_build_list.
>
> Can't comment on those two, but the use of EXCEPTION_MARK in a code 
> block where there is a CHECK macro makes no sense. The EXCEPTION_MARK 
> expects any pending exception to be dealt with and cleared before 
> getting to the end of the block. The CHECK propagates the exception to 
> the caller, thus escaping the block.

Okay, got it, thanks for the explanation.  Understanding why the 
EXCEPTION_MARK is present on line #752 & #816 is a separate exercise 
then.  Harold, I'm fine with your current webrev.
Thanks,
Lois

>
> David
>
>> Thanks,
>> Lois
>>
>>>
>>> Changes look good.
>>>
>>> Cheers,
>>> David
>>> -----
>>>
>>>> Harold
>>>>
>>>>
>>>> On 4/10/2018 3:30 PM, Lois Foltan wrote:
>>>>>
>>>>> On 4/10/2018 2:49 PM, harold seigel wrote:
>>>>>
>>>>>> Hi Lois,
>>>>>>
>>>>>> I think your suggestion is a good one.  Please see updated webrev 
>>>>>> at:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~hseigel/bug_8201265.2/webrev/
>>>>>>
>>>>>
>>>>> Looks good Harold!  Thanks for fixing.  Minor note, an 
>>>>> EXCEPTION_MARK maybe required ahead of line #870?  I don't need to 
>>>>> see another webrev.
>>>>> Thanks,
>>>>> Lois
>>>>>
>>>>>> Thanks, Harold
>>>>>>
>>>>>>
>>>>>> On 4/9/2018 5:16 PM, Lois Foltan wrote:
>>>>>>> On 4/9/2018 4:35 PM, harold seigel wrote:
>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Please review this small fix for JDK-8201265.  The fix deletes 
>>>>>>>> the native memory before propagating back the exception.
>>>>>>>>
>>>>>>>> Open Webrev: 
>>>>>>>> http://cr.openjdk.java.net/~hseigel/bug_8201265/webrev/index.html
>>>>>>> Hi Harold,
>>>>>>>
>>>>>>> I think the use of NEW_C_HEAP_ARRAY at line #874 is incorrect 
>>>>>>> within ClassLoader::add_to_exploded_build_list().  If you look 
>>>>>>> at both lines #754 & 818, NEW_RESOURCE_ARRAY is used prior to 
>>>>>>> the call to create_class_path_entry().  I believe all the 
>>>>>>> ClassLoader::create_class_path_entry() constructors for file, 
>>>>>>> dir, zip, etc. allocate their own C_HEAP_ARRAY and make a copy 
>>>>>>> of the "path" parameter.
>>>>>>>
>>>>>>> So I think the correct fix is to change #874 to use 
>>>>>>> NEW_RESOURCE_ARRAY and remove the lines you added to check for 
>>>>>>> pending exception.  And remove the FREE_C_HEAP_ARRAY at line #900.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Lois
>>>>>>>
>>>>>>>>
>>>>>>>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8201265
>>>>>>>>
>>>>>>>> This fix was tested with Mach5 tiers 1 and 2 tests and builds 
>>>>>>>> on all Mach5 platforms and with tiers 3-5 tests on Linux-x64.
>>>>>>>>
>>>>>>>> Thanks, Harold
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>