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

[Prasanta Sadhukhan]

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