<Swing Dev> [10] RFR JDK-8191639:NPE from BasicListUI.Actions.getNextPageIndex

Prasanta Sadhukhan prasanta.sadhukhan at oracle.com
Tue Dec 5 12:51:11 UTC 2017


Hi Semyon,

Ok. added index check
and followed previous review comment of returning index immediately 
instead of additional indentation
http://cr.openjdk.java.net/~psadhukhan/8191639/webrev.04/

Regards
Prasanta
On 12/4/2017 9:39 PM, Semyon Sadetsky wrote:
> Hi Prasanta,
>
> I'm not suggesting to remove the null check. I suggested to add check 
> for -1 since the bounds of the cell with index -1 should never exists 
> and it doesn't make scene to call the method to calculate its bounds.
>
> --Semyon
>
>
> On 12/03/2017 10:33 PM, Prasanta Sadhukhan wrote:
>> HI Semyon,
>>
>> I guess app can also override getCellBounds and return null (maybe 
>> mistakenly) so we need to check for return null.
>>
>> Regards
>> Prasanta
>> On 11/29/2017 10:39 PM, Semyon Sadetsky wrote:
>>> Hi Prasanta,
>>>
>>> I suggest to call list.getCellBounds() only if index != -1 since the 
>>> result will be always null in that case.
>>>
>>> --Semyon
>>>
>>>
>>> On 11/29/2017 01:04 AM, Prasanta Sadhukhan wrote:
>>>> OK. http://cr.openjdk.java.net/~psadhukhan/8191639/webrev.03/
>>>>
>>>> Regards
>>>> Prasanta
>>>>
>>>>
>>>> On 11/29/2017 12:35 AM, Sergey Bylokhov wrote:
>>>>> On 27/11/2017 23:45, Prasanta Sadhukhan wrote:
>>>>>> Ok. I have added the check for scrollRectToVisible() too.
>>>>>>
>>>>>> http://cr.openjdk.java.net/~psadhukhan/8191639/webrev.02
>>>>>
>>>>> Should the test use some specific L&F? Currently it is pass on 
>>>>> macOS and Aqua L&F before the fix.
>>>>>
>>>>>>
>>>>>> Regards
>>>>>> Prasanta
>>>>>> On 11/28/2017 1:01 PM, Sergey Bylokhov wrote:
>>>>>>> On 27/11/2017 22:40, Prasanta Sadhukhan wrote:
>>>>>>>> On 11/28/2017 3:33 AM, Sergey Bylokhov wrote:
>>>>>>>>> Hi, Prasanta.
>>>>>>>>> A few notes about the fix:
>>>>>>>>>  - Can we get an NPE in adjustScrollPositionIfNecessary at
>>>>>>>>>        2391 list.scrollRectToVisible(cellBounds); Not sure 
>>>>>>>>> that scrollRectToVisible is ready for null.
>>>>>>>> cellBounds is not null for this call due to this check in
>>>>>>>>
>>>>>>>> 2308             if (cellBounds != null && 
>>>>>>>> !visRect.contains(cellBounds)) {
>>>>>>>
>>>>>>> In the fix you added a check at line
>>>>>>> 2384 if (cellBounds != null) {
>>>>>>>
>>>>>>> And later you will pass the cellBounds as a parameter:
>>>>>>> 2393 list.scrollRectToVisible(cellBounds);
>>>>>>>
>>>>>>> So either cellBounds can be null or the new check is not needed.
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>>  - I think the assignment below can be simplified
>>>>>>>>>        cellBounds = startRect != null ? startRect : null;
>>>>>>>>>  - In the getNextPageIndex() probably we can "return index" 
>>>>>>>>> immediately instead of additional indentation?
>>>>>>>>>
>>>>>>>> Done. Modified webrev
>>>>>>>> http://cr.openjdk.java.net/~psadhukhan/8191639/webrev.01/
>>>>>>>>
>>>>>>>> Regards
>>>>>>>> Prasanta
>>>>>>>>> On 22/11/2017 03:05, Prasanta Sadhukhan wrote:
>>>>>>>>>> Hi All,
>>>>>>>>>>
>>>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8191639
>>>>>>>>>>
>>>>>>>>>> It is seen that when JList.locationToIndex() or 
>>>>>>>>>> getCellBounds() is overridden to return -1 or null respectively,
>>>>>>>>>> it causes NPE when PageUp/PageDown is pressed in JList.
>>>>>>>>>>
>>>>>>>>>>  From the spec 
>>>>>>>>>> [https://docs.oracle.com/javase/9/docs/api/javax/swing/JList.html#getCellBounds-int-int-] 
>>>>>>>>>>
>>>>>>>>>> of getCellBounds(), it is seen that it can return null under 
>>>>>>>>>> some circumstances. But, JList jdk code assumes it is always 
>>>>>>>>>> non-null.
>>>>>>>>>>
>>>>>>>>>> Proposed fix is to check for null return value of 
>>>>>>>>>> getCellBounds() and bail out.
>>>>>>>>>> webrev: 
>>>>>>>>>> http://cr.openjdk.java.net/~psadhukhan/8191639/webrev.00/
>>>>>>>>>>
>>>>>>>>>> Regards
>>>>>>>>>> Prasanta
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>




More information about the swing-dev mailing list