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:swingguy1...@yahoo.com] Sent: Thursday, February 18, 2016 4:29 PM To: Ajit Ghaisas; Sergey Bylokhov; Rajeev Chamyal; Alexander Scherbatiy; swing-dev@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.ghai...@oracle.com"ajit.ghai...@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@openjdk.java.net"swing-dev@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@openjdk.java.net"swing-dev@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@openjdk.java.net"swing-dev@openjdk.java.net > <mailto:HYPERLINK > "mailto:swing-dev@openjdk.java.net"swing-dev@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@openjdk.java.net"swing-dev@openjdk.java.net > <mailto:HYPERLINK > "mailto:swing-dev@openjdk.java.net"swing-dev@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@openjdk.java.net"swing-dev@openjdk.java.net > <mailto:HYPERLINK > "mailto:swing-dev@openjdk.java.net"swing-dev@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.
