[8u] RFR(xs): 8068042: Check jdk/src/share/native/sun/misc/URLClassPath.c for JNI pending exception

[David Holmes]

Looks good.

Thanks,
David

On 2/12/2015 7:05 AM, Calvin Cheung wrote:
> Hi Jiangli, David, Ioi,
>
> Thank you for the review.
>
> Here's an updated webrev:
> http://cr.openjdk.java.net/~ccheung/8068042_8u/webrev.01/
>
> Basically, the JNU_ThrowOutOfMemoryError call has been replaced with a
> comment.
>
> thanks,
> Calvin
>
> On 11/30/15, 8:52 PM, Ioi Lam wrote:
>>
>>
>> On 11/30/15 8:21 PM, David Holmes wrote:
>>> On 1/12/2015 12:10 PM, Jiangli Zhou wrote:
>>>> Hi Calvin,
>>>>
>>>> I see getUTF() throws OutOfMemoryError if malloc fails before
>>>> returning NULL. Do you know if there is any case that getUTF()
>>>> returns NULL without throwing any exception? The usage of getUTF()
>>>> and exception handling don’t seem to be consistent in existing idk
>>>> code. Some places check the NULL return value without throwing a new
>>>> exception, while the other places throws a new OOM without checking
>>>> for pending exception (like the code you are fixing). Maybe the
>>>> callee does not need to re-throw the exception for the NULL return
>>>> value case, and you can add a comment saying “an OutOfMemoryError”
>>>> has already been thrown by getUTF()?
>>>
>>> According to classloader.c:
>>>
>>> /* Convert java string to UTF char*. Use local buffer if possible,
>>>    otherwise malloc new memory. Returns null IFF malloc failed. */
>>>
>>> So NULL implies OOME has been thrown. I'd suggest removing the new
>>> code and the JNU_ThrowOutOfMemory with a comment as Jiangli suggested.
>>>
>>> Also note we have the same kinds of patterns in classLoader.c in 8
>>> and 9.
>>>
>> I introduced this bug in URLClassPath.c because I copied the code from
>> ClassLoader.c, which had the same pattern, and has been reported in
>> https://bugs.openjdk.java.net/browse/JDK-6952230 (5 years!)
>>
>> - Ioi
>>
>>> Thanks,
>>> David
>>>
>>>> Thanks,
>>>> Jiangli
>>>>
>>>>
>>>>> On Nov 30, 2015, at 5:13 PM, Calvin Cheung
>>>>> <calvin.cheung at oracle.com> wrote:
>>>>>
>>>>> Please review this small fix for adding the check for JNI pending
>>>>> exception in URLClassPath.c.
>>>>>
>>>>> This fix is only for jdk8u and isn't applicable to 9.
>>>>>
>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8068042
>>>>>       (unfortunately it is a closed bug)
>>>>>
>>>>> webrev: http://cr.openjdk.java.net/~ccheung/8068042_8u/webrev.00/
>>>>>
>>>>> Tests:
>>>>>     JPRT with -testset hotspot
>>>>>     RBT hotspot/test/:hotspot_all,vm.regression.testlist, vm.quick
>>>>>
>>>>> thanks,
>>>>> Calvin
>>>>
>>