[10] Review Request: 8187442 Xcheck:jni produces various "WARNING in native method" in launcher

[Sergey Bylokhov]

On 9/12/17 16:03, David Holmes wrote:
> call. That is fine, but still leaves the problem that you are skipping 
> the DeleteLocalRef call.

The leak was there before: if we will get an exception at
"1512 SetByteArrayRegion()"
then we never call this DeleteLocalRef().

I assumed it was done intentionally since this will be kind of "fatal" 
error.
The same pattern is used in the other places for example in
NewPlatformStringArray:

         jstring str = NewPlatformString(env, *strv++);
         NULL_CHECK0(str);
         (*env)->SetObjectArrayElement(env, ary, i, str);
         (*env)->DeleteLocalRef(env, str);

> 
> Thanks - and sorry again for my confusion on this.
> 
> David
> -----
> 
>>> In addition this does nothing to clear the pending exception so I can 
>>> not see how it would affect any warnings.
>>
>> It does not clear an exception but preserve it instead, and does not 
>> use the result of the method which produced an exception.
>>
>>>
>>> David
>>>
>>>> This value will be propagated to JavaMain() and I as far as 
>>>> understand will stop the execution.
>>>>
>>>>
>>>> On 9/12/17 13:56, David Holmes wrote:
>>>>> Hi Sergey,
>>>>>
>>>>> On 13/09/2017 5:18 AM, Sergey Bylokhov wrote:
>>>>>> Hello,
>>>>>> Please review the fix for jdk10/jdk10.
>>>>>>
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8187442
>>>>>> Webrev can be found at: 
>>>>>> http://cr.openjdk.java.net/~serb/8187442/webrev.00
>>>>>
>>>>> This doesn't look right to me:
>>>>>
>>>>>                str = (*env)->CallStaticObjectMethod(env, cls,
>>>>>                        makePlatformStringMID, USE_STDERR, ary);
>>>>> +             CHECK_EXCEPTION_RETURN_VALUE(0);
>>>>>                (*env)->DeleteLocalRef(env, ary);
>>>>>                return str;
>>>>>
>>>>> If there is an exception pending you fail to delete the local ref. 
>>>>> And there's no need to clear the exception before calling 
>>>>> DeleteLocalRef as that is okay to call with a pending exception. 
>>>>> But there is no point returning zero if an exception occurred 
>>>>> because in that case str will already be 0/NULL.
>>>>>
>>>>> The same here:
>>>>>
>>>>> 1596     appClass = (*env)->CallStaticObjectMethod(env, cls, mid);
>>>>> 1597     CHECK_EXCEPTION_RETURN_VALUE(0);
>>>>> 1598     return appClass;
>>>>>
>>>>> If an exception is pending then appClass will be 0/NULL.
>>>>>
>>>>> In addition CHECK_EXCEPTION_RETURN_VALUE doesn't clear the pending 
>>>>> exception so I can't see what warnings this would be clearing up ???
>>>>>
>>>>> Thanks,
>>>>> David
>>>>> -----
>>>>>
>>>>>> The simple application with empty main method produce some 
>>>>>> "warnings" when Xcheck:jni is used. Because of that it is hard to 
>>>>>> cleanup other parts of jdk from such warnings.
>>>>>>
>>>>>> Two additional checks were added, in both cases the new code will 
>>>>>> return 0 in the same way as NULL_CHECK0 in the same methods.
>>>>>>
>>>>>>
>>>>
>>>>
>>
>>


-- 
Best regards, Sergey.