<AWT Dev> RFR: [XS] 8214380: AwtDragSource function LoadCache misses a ReleaseLongArrayElements in special case

[Baesken, Matthias]

Thanks. Can I have a second Review please?

> Am 01.12.2018 um 01:20 schrieb Sergey Bylokhov <Sergey.Bylokhov at oracle.com>:
> 
> Looks fine.
> 
>> On 30/11/2018 05:22, Baesken, Matthias wrote:
>> Hi Sergey,   I prepared a second webrev  :
>> http://cr.openjdk.java.net/~mbaesken/webrevs/8214380.1/
>> - removed the unused isCopy
>> - added a NULL check  after  GetLongArrayElements
>> Best regards, Matthias
>>> -----Original Message-----
>>> From: Baesken, Matthias
>>> Sent: Donnerstag, 29. November 2018 10:01
>>> To: 'Sergey Bylokhov' <Sergey.Bylokhov at oracle.com>; 'awt-
>>> dev at openjdk.java.net' <awt-dev at openjdk.java.net>
>>> Subject: RE: <AWT Dev> RFR: [XS] 8214380: AwtDragSource function
>>> LoadCache misses a ReleaseLongArrayElements in special case
>>> 
>>> Btw when looking at the coding,  I  wonder  why  we save the copy-state  in
>>> isCopy  :
>>> 
>>> Docu :    "If isCopy is not NULL, then *isCopy is set to JNI_TRUE if a copy is
>>> made; or it is set to JNI_FALSE if no copy is made"
>>> 
>>>  397     jboolean isCopy;
>>>  398     jlong *lFormats = env->GetLongArrayElements(formats, &isCopy),
>>> 
>>>  But  then we do not use it afterwards ,   so should we better call   it with
>>> NULL  instead?
>>> 
>>> Regards, Matthias
>>> 
>>> 
>>>> -----Original Message-----
>>>> From: Baesken, Matthias
>>>> Sent: Donnerstag, 29. November 2018 09:17
>>>> To: 'Sergey Bylokhov' <Sergey.Bylokhov at oracle.com>; awt-
>>>> dev at openjdk.java.net
>>>> Subject: RE: <AWT Dev> RFR: [XS] 8214380: AwtDragSource function
>>>> LoadCache misses a ReleaseLongArrayElements in special case
>>>> 
>>>> Hi Sergey,   I  think it is (at least)  a good practice  to NULL -  check  the result
>>>> of  env->GetLongArrayElements .
>>>> Probably it should be done  right after  the
>>>> 
>>>>      jlong *lFormats = env->GetLongArrayElements(formats, &isCopy),
>>>> 
>>>> call.
>>>> See  jdk/src/java.base/unix/native/libjava/ProcessHandleImpl_unix.c   for
>>>> examples of usages   of the function (with NULL checks) .
>>>> 
>>>> Or see the documentation at
>>>> 
>>> https://docs.oracle.com/javase/7/docs/technotes/guides/jni/spec/function
>>>> s.html
>>>> Where we find :
>>>> 
>>>> 
>>>> Get<PrimitiveType>ArrayElements Routines
>>>>    ....
>>>> PARAMETERS:
>>>> 
>>>> env: the JNI interface pointer.
>>>> array: a Java string object.
>>>> isCopy: a pointer to a boolean.
>>>> 
>>>> 
>>>> RETURNS:
>>>> 
>>>> Returns a pointer to the array elements, ****or NULL if the operation
>>>> fails.****
>>>> 
>>>> 
>>>> So returning NULL might  happen .
>>>> 
>>>> Best regards, Matthias
>>>> 
>>>> 
>>>> 
>>>>> -----Original Message-----
>>>>> From: Sergey Bylokhov <Sergey.Bylokhov at oracle.com>
>>>>> Sent: Donnerstag, 29. November 2018 00:22
>>>>> To: Baesken, Matthias <matthias.baesken at sap.com>; awt-
>>>>> dev at openjdk.java.net
>>>>> Subject: Re: <AWT Dev> RFR: [XS] 8214380: AwtDragSource function
>>>>> LoadCache misses a ReleaseLongArrayElements in special case
>>>>> 
>>>>> Hi, Matthias.
>>>>> 
>>>>> Do we need the null check in the fix, if "yes" then probably
>>>>> the same check should be added before this line as well?
>>>>>    465     env->ReleaseLongArrayElements(formats, saveFormats, 0);
>>>>> 
>>>>>> On 28/11/2018 00:33, Baesken, Matthias wrote:
>>>>>> Hello, please review  this small fix.
>>>>>> 
>>>>>> It adds a missing ReleaseLongArrayElements  to
>>>>>> 
>>>>>> void AwtDragSource::LoadCache(jlongArray formats)
>>>>>> 
>>>>>> in an early special   "pseudo return"  (leave function via throw)  case.
>>>>>> 
>>>>>> Webrev/bug :
>>>>>> 
>>>>>> http://cr.openjdk.java.net/~mbaesken/webrevs/8214380.0/
>>>>>> 
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8214380
>>>>>> 
>>>>>> Thanks, Matthias
>>>>>> 
>>>>> 
>>>>> 
>>>>> --
>>>>> Best regards, Sergey.
> 
> 
> -- 
> Best regards, Sergey.