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

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

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

prasanta sadhukhan
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
Reply | Threaded
Open this post in threaded view
|

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

Sergey Bylokhov
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.
  - 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?

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


--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

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

prasanta sadhukhan
Hi Sergey,


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

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

Reply | Threaded
Open this post in threaded view
|

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

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


--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

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

prasanta sadhukhan
Ok. I have added the check for scrollRectToVisible() too.

http://cr.openjdk.java.net/~psadhukhan/8191639/webrev.02/

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

Reply | Threaded
Open this post in threaded view
|

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

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


--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

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

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

Reply | Threaded
Open this post in threaded view
|

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

Sergey Bylokhov
Looks fine.
Two small issues should be fixed before the push.
  - Typo in the summary "Verifies no NPE is thrown wjen pageup/down is
pressed in a JList"
  - "@headful" should be "* @key headful"

On 29/11/2017 01:04, 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
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>>
>


--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

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

semyon.sadetsky
In reply to this post by prasanta sadhukhan
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
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>>
>

Reply | Threaded
Open this post in threaded view
|

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

Reply | Threaded
Open this post in threaded view
|

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

semyon.sadetsky
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
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

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

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

Reply | Threaded
Open this post in threaded view
|

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

semyon.sadetsky
+1

--Semyon


On 12/05/2017 04:51 AM, Prasanta Sadhukhan wrote:

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