Review request for bug 100053

[Roman Kennke-5]

Hi guys,

This patch here fixes the NPE in Pisces renderer as reported in:

https://bugs.openjdk.java.net/show_bug.cgi?id=100053

The webrev is here:

http://cr.openjdk.java.net/~rkennke/100053/webrev.00/

It basically adds nullchecks in the offending code. As far as I can see
this should be sufficient because these arrays are only accessed in the
scope of endRendering(). A testcase is also included.

Thanks, Roman

[Dmitri Trembovetski]

   Hi Roman,

   a comment about the test: would the bug reproduce if you just rendered into a
BufferedImage? If so, no need for creating a frame and such.

   Regarding the fix, it looks ok - but there are other places in the code where
the 'crossings' is accessed - are those safe from an NPE?

   Thanks,
     Dmitri


Roman Kennke wrote:

> Hi guys,
>
> This patch here fixes the NPE in Pisces renderer as reported in:
>
> https://bugs.openjdk.java.net/show_bug.cgi?id=100053
>
> The webrev is here:
>
> http://cr.openjdk.java.net/~rkennke/100053/webrev.00/
>
> It basically adds nullchecks in the offending code. As far as I can see
> this should be sufficient because these arrays are only accessed in the
> scope of endRendering(). A testcase is also included.
>
> Thanks, Roman
>
>

[Roman Kennke-5]

Hi Dmitri,

>    a comment about the test: would the bug reproduce if you just rendered into a
> BufferedImage? If so, no need for creating a frame and such.

Oh yes. Stupid me :-)

>    Regarding the fix, it looks ok - but there are other places in the code where
> the 'crossings' is accessed - are those safe from an NPE?

I followed all usages of crossings and crossingsIndices and they all end
up in endRendering(). There is a loop at the beginning of which the
arrays get initialized using setCrossingsExtends(). The problem occured
when this loop was never entered, in that case we hit
crossingListFinished() with (possibly) null arrays. There I added the
null checks and it should cover all potential NPEs on these arrays.

The updated webrev is:

http://cr.openjdk.java.net/~rkennke/100053/webrev.01/

Ok now?

Thanks, Roman

[Dmitri Trembovetski]

   Hi Roman,

Roman Kennke wrote:

> Hi Dmitri,
>
>>    a comment about the test: would the bug reproduce if you just rendered into a
>> BufferedImage? If so, no need for creating a frame and such.
>
> Oh yes. Stupid me :-)
>
>>    Regarding the fix, it looks ok - but there are other places in the code where
>> the 'crossings' is accessed - are those safe from an NPE?
>
> I followed all usages of crossings and crossingsIndices and they all end
> up in endRendering(). There is a loop at the beginning of which the
> arrays get initialized using setCrossingsExtends(). The problem occured
> when this loop was never entered, in that case we hit
> crossingListFinished() with (possibly) null arrays. There I added the
> null checks and it should cover all potential NPEs on these arrays.
>
> The updated webrev is:
>
> http://cr.openjdk.java.net/~rkennke/100053/webrev.01/
>
> Ok now?

   Yes, looks fine.

   Thanks,
     Dmitri


>
> Thanks, Roman
>
>

[Jim Graham-3]

I could go too ways on this.

It looks like the code is looking to drop arrays that have grown so that
it doesn't waste memory.  Do we reuse these objects?  If not, then the
code can be deleted.

If we do reuse them, then why not just set them to null and let them get
recreated the next time rather than allocate something which may not get
used for a while?

In either case, if the array was null, is there a need to allocate one?
  Shouldn't it be "if (array != null && array.length > DEFAULT)"?

                        ...jim

Dmitri Trembovetski wrote:

>
>   Hi Roman,
>
> Roman Kennke wrote:
>> Hi Dmitri,
>>
>>>    a comment about the test: would the bug reproduce if you just
>>> rendered into a BufferedImage? If so, no need for creating a frame
>>> and such.
>>
>> Oh yes. Stupid me :-)
>>
>>>    Regarding the fix, it looks ok - but there are other places in the
>>> code where the 'crossings' is accessed - are those safe from an NPE?
>>
>> I followed all usages of crossings and crossingsIndices and they all end
>> up in endRendering(). There is a loop at the beginning of which the
>> arrays get initialized using setCrossingsExtends(). The problem occured
>> when this loop was never entered, in that case we hit
>> crossingListFinished() with (possibly) null arrays. There I added the
>> null checks and it should cover all potential NPEs on these arrays.
>>
>> The updated webrev is:
>>
>> http://cr.openjdk.java.net/~rkennke/100053/webrev.01/
>>
>> Ok now?
>
>   Yes, looks fine.
>
>   Thanks,
>     Dmitri
>
>
>>
>> Thanks, Roman
>>
>>

[Roman Kennke-5]

Hi Jim,

I think you are right. I think the idea is to keep the array around to
avoid massive load on the GC. And if the array needs to be grown (i.e.
very rarely), it gets shrinked back to default size. So I would go with:

>   Shouldn't it be "if (array != null && array.length > DEFAULT)"?

The updated webrev is here:

http://cr.openjdk.java.net/~rkennke/100053/webrev.02/

I tested this with the testcase and Java2Demo and had no problems with
this.

Ok now?

Thanks, Roman

[Jim Graham-3]

That looks fine.

A note on code style, though.  The following form would better match the
indentation/continuation style used in the rest of this file and the 2D
code:

  781         if (crossingIndices != null &&
  782             crossingIndices.length > DEFAULT_INDICES_SIZE)
  783         {

                        ...jim

Roman Kennke wrote:

> Hi Jim,
>
> I think you are right. I think the idea is to keep the array around to
> avoid massive load on the GC. And if the array needs to be grown (i.e.
> very rarely), it gets shrinked back to default size. So I would go with:
>
>>   Shouldn't it be "if (array != null && array.length > DEFAULT)"?
>
> The updated webrev is here:
>
> http://cr.openjdk.java.net/~rkennke/100053/webrev.02/
>
> I tested this with the testcase and Java2Demo and had no problems with
> this.
>
> Ok now?
>
> Thanks, Roman
>
>