RFR 8201265: Native memory leak in ClassLoader::add_to_exploded_build_list

[David Holmes]

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.

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