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

Wed Feb 24 13:30:12 UTC 2016

Looks fine.

On 19.02.16 12:04, Ajit Ghaisas wrote:
> 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
> <ajit.ghaisas at oracle.com <mailto: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;
> swing-dev at openjdk.java.net <mailto: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;
>>swing-dev at openjdk.java.net <mailto: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;
>>swing-dev at openjdk.java.net <mailto:swing-dev at openjdk.java.net>
> <mailto:swing-dev at openjdk.java.net <mailto: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;
>>swing-dev at openjdk.java.net <mailto:swing-dev at openjdk.java.net>
> <mailto:swing-dev at openjdk.java.net <mailto: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;
>>swing-dev at openjdk.java.net <mailto:swing-dev at openjdk.java.net>
> <mailto:swing-dev at openjdk.java.net <mailto: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.
>

-- 
Best regards, Sergey.