<Swing Dev> [9] Review fix for JDK-8020039 : SynthTableHeaderUI refers to possibly null parameter in cell renderer

[Ajit Ghaisas]

Hi,

 

   There are the some good points made by Miguel regarding null checks – but, in this case the table being null is a valid case. 

   While rendering the TableHeader, it is possible to be in a situation where ‘table’ parameter can be null.

 

  Please refer to a similar discussion : https://community.oracle.com/thread/2279601?start=0&tstart=0  

 

 

   I have done corrections to address Sergey’s second question and simplified the test code.

    Please review updated webrev :  http://cr.openjdk.java.net/~arapte/ajit/8020039/webrev.04/ 

 

Regards,

Ajit

 

From: Miguel Munoz [mailto:swingguy1024 at yahoo.com] 
Sent: Thursday, February 18, 2016 4:29 PM
To: Ajit Ghaisas; Sergey Bylokhov; Rajeev Chamyal; Alexander Scherbatiy; swing-dev at openjdk.java.net
Subject: Re: <Swing Dev> [9] Review fix for JDK-8020039 : SynthTableHeaderUI refers to possibly null parameter in cell renderer

 

It would seem to me that if the table is null, you have a bug in another class. Is there a valid reason for the table to be null? I would guess not.

In my experience, many null checks are either unnecessary or only serve to hide bugs. Steve Maguire writes about this in his book Writing Solid Code.

 

-- Miguel Muñoz

 

On Wednesday, February 17, 2016 10:52 PM, Ajit Ghaisas <HYPERLINK "mailto:ajit.ghaisas at oracle.com"ajit.ghaisas at oracle.com> wrote:

 

Hi,

    Goods finding Sergey.

    1. In file, src/java.desktop/macosx/classes/com/apple/laf/AquaTableHeaderUI.java, call to setText() and setBorder() should be made irrespective of whether table in null or not. I have made this change.
    2. The changes to SynthTableHeaderUI.java are correct. We cannot assume a table property (enabled) when table is null. 
        A call to super. getTableCellRendererComponent() is already made from this method which in turn calls setText() and setBorder().

    Please review updated webrev : http://cr.openjdk.java.net/~arapte/ajit/8020039/webrev.03/

Regards,
Ajit




-----Original Message-----
From: Sergey Bylokhov 
Sent: Wednesday, February 17, 2016 8:41 PM
To: Rajeev Chamyal; Ajit Ghaisas; Alexander Scherbatiy; HYPERLINK "mailto:swing-dev at openjdk.java.net"swing-dev at openjdk.java.net
Subject: Re: <Swing Dev> [9] Review fix for JDK-8020039 : SynthTableHeaderUI refers to possibly null parameter in cell renderer

Hi, Hello Ajit.
I am not sure that exclusion of the code is a correct fix here. For example can you clarify should we call setText(or other setXXX) when the table is null or not? Another question is: should we skip the code in SynthTableHeaderUI.java or we can assume table==null as enabled table?

On 17.02.16 14:29, Rajeev Chamyal wrote:
> Looks good to me.
>
> Regards,
>
> Rajeev Chamyal
>
> *From:*Ajit Ghaisas
> *Sent:* 17 February 2016 16:51
> *To:* Rajeev Chamyal; Sergey Bylokhov; Alexander Scherbatiy; 
> HYPERLINK "mailto:swing-dev at openjdk.java.net"swing-dev at openjdk.java.net
> *Subject:* RE: <Swing Dev> [9] Review fix for JDK-8020039 :
> SynthTableHeaderUI refers to possibly null parameter in cell renderer
>
> Hi,
>
>      I have corrected formatting of test code and removed the 
> additional System.out.printlns.
>
>      Please review :
> http://cr.openjdk.java.net/~arapte/ajit/8020039/webrev.02/
>
> Regards,
>
> Ajit
>
> *From:*Ajit Ghaisas
> *Sent:* Tuesday, February 16, 2016 3:12 PM
> *To:* Rajeev Chamyal; Sergey Bylokhov; Alexander Scherbatiy; 
> HYPERLINK "mailto:swing-dev at openjdk.java.net"swing-dev at openjdk.java.net <mailto:HYPERLINK "mailto:swing-dev at openjdk.java.net"swing-dev at openjdk.java.net>
> *Subject:* Re: <Swing Dev> <Swing-dev> [9] Review fix for JDK-8020039 :
> SynthTableHeaderUI refers to possibly null parameter in cell renderer
>
> Hi,
>
>      Thanks Rajeev for review comments.
>
>      I have checked - windows LAF WindowsTableHeaderUI handles it 
> correctly.
>
>      I have added null check for MacOs Aqua.
>
>      Also, I have added a test checking for this null pointer access.
>
>      Please review --
> http://cr.openjdk.java.net/~arapte/ajit/8020039/webrev.01/
>
> Regards,
>
> Ajit
>
> *From:*Rajeev Chamyal
> *Sent:* Monday, February 15, 2016 5:39 PM
> *To:* Ajit Ghaisas; Sergey Bylokhov; Alexander Scherbatiy; 
> HYPERLINK "mailto:swing-dev at openjdk.java.net"swing-dev at openjdk.java.net <mailto:HYPERLINK "mailto:swing-dev at openjdk.java.net"swing-dev at openjdk.java.net>
> *Subject:* RE: <Swing-dev> [9] Review fix for JDK-8020039 :
> SynthTableHeaderUI refers to possibly null parameter in cell renderer
>
> Hello Ajit,
>
> Can you please if similar fix is required for other LAF windows ,Aqua etc.
>
> Please add a regression test case also.
>
> Regards,
>
> Rajeev Chamyal
>
> *From:*Ajit Ghaisas
> *Sent:* 15 February 2016 17:30
> *To:* Rajeev Chamyal; Sergey Bylokhov; Alexander Scherbatiy; 
> HYPERLINK "mailto:swing-dev at openjdk.java.net"swing-dev at openjdk.java.net <mailto:HYPERLINK "mailto:swing-dev at openjdk.java.net"swing-dev at openjdk.java.net>
> *Subject:* <Swing-dev> [9] Review fix for JDK-8020039 :
> SynthTableHeaderUI refers to possibly null parameter in cell renderer
>
> Hi,
>
> Please review the fix for jdk9,
>
>                  Bug: https://bugs.openjdk.java.net/browse/JDK-8020039
>
> Webrev: http://cr.openjdk.java.net/~arapte/ajit/8020039/webrev.00/
>
> Issue :
>
>      If null is passed as 'table' parameter to
> SynthTableHeaderUI::getTableCellRendererComponent() method in
>  
> src/java.desktop/share/classes/javax/swing/plaf/synth/SynthTableHeader
> UI.java,
>
>      there is Null Pointer Exception.
>
> Analysis :
>
>    This method already has a null check for 'table' parameter for 
> second access of this parameter in method.
>
>    Whereas the first access of the 'table' parameter lacks this check.
>
> Fix :
>
>    Added null check for the first access of 'table' parameter in 
> SynthTableHeaderUI::getTableCellRendererComponent().
>
>    There is no else block added as the flow continues and passes 
> table to base class method using super. getTableCellRendererComponent().
>
>    The passed parameter is already checked in base class method 
> correctly. Hence, no change is needed in base class.
>
> Test :
>
>    The fix is pretty straight forward.
>
>    Executed the code snippet given in the bug description. There is 
> no NPE after the fix.
>
> Regards,
>
> Ajit
>


--
Best regards, Sergey.

 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20160219/3b2f7038/attachment.html>