RFR 8201265: Native memory leak in ClassLoader::add_to_exploded_build_list

[Lois Foltan]

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.

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
>>>>>>
>>>>>
>>>>
>>>
>>