<AWT Dev> RFR: 8255800: Raster creation methods need some specification clean up

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

<AWT Dev> RFR: 8255800: Raster creation methods need some specification clean up

Phil Race
https://bugs.openjdk.java.net/browse/JDK-8255800 could have been a one line spec clean up but
it didn't take a lot of looking to realize there were many more inconsistencies between spec and implementation.
I've spent a lot of time on what is just small number of factory methods in Raster because there are so
many possible exceptions and in some cases they rely on other API they call to generate exceptions and
these may have not been documented or documented acc. to some long lost behavior.
I've mostly tried to ONLY change spec. But I couldn't help myself when some checks were missed that
ended up with bizarre and dubious behavior - throwing NegativeArrayIndexException which just about
always has to be an internal bug !

The supplied test passes on JDK 16 as well as this code, because the (relatively) small number of
cases where JDK 16 threw NegativeArrayIndexException are caught and allowed only for releases < 17
So where you see those in the test it corresponds to the behavioral changes from NegativeArrayIndexException
to IllegalArgumentException.
JCK conformance tests still pass so they must not test those conditions.

-------------

Commit messages:
 - 8255800: Raster creation methods need some specification clean up

Changes: https://git.openjdk.java.net/jdk/pull/3223/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3223&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8255800
  Stats: 1469 lines in 5 files changed: 1399 ins; 35 del; 35 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3223.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3223/head:pull/3223

PR: https://git.openjdk.java.net/jdk/pull/3223
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8255800: Raster creation methods need some specification clean up

Phil Race
On Fri, 26 Mar 2021 19:53:44 GMT, Phil Race <[hidden email]> wrote:

> https://bugs.openjdk.java.net/browse/JDK-8255800 could have been a one line spec clean up but
> it didn't take a lot of looking to realize there were many more inconsistencies between spec and implementation.
> I've spent a lot of time on what is just small number of factory methods in Raster because there are so
> many possible exceptions and in some cases they rely on other API they call to generate exceptions and
> these may have not been documented or documented acc. to some long lost behavior.
> I've mostly tried to ONLY change spec. But I couldn't help myself when some checks were missed that
> ended up with bizarre and dubious behavior - throwing NegativeArrayIndexException which just about
> always has to be an internal bug !
>
> The supplied test passes on JDK 16 as well as this code, because the (relatively) small number of
> cases where JDK 16 threw NegativeArrayIndexException are caught and allowed only for releases < 17
> So where you see those in the test it corresponds to the behavioral changes from NegativeArrayIndexException
> to IllegalArgumentException.
> JCK conformance tests still pass so they must not test those conditions.

oh and yes I know this requires a CSR ! But I want to do the code review first and then submit a CSR based on the approved version.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3223
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8255800: Raster creation methods need some specification clean up

Sergey Bylokhov-2
In reply to this post by Phil Race
On Fri, 26 Mar 2021 19:53:44 GMT, Phil Race <[hidden email]> wrote:

> https://bugs.openjdk.java.net/browse/JDK-8255800 could have been a one line spec clean up but
> it didn't take a lot of looking to realize there were many more inconsistencies between spec and implementation.
> I've spent a lot of time on what is just small number of factory methods in Raster because there are so
> many possible exceptions and in some cases they rely on other API they call to generate exceptions and
> these may have not been documented or documented acc. to some long lost behavior.
> I've mostly tried to ONLY change spec. But I couldn't help myself when some checks were missed that
> ended up with bizarre and dubious behavior - throwing NegativeArrayIndexException which just about
> always has to be an internal bug !
>
> The supplied test passes on JDK 16 as well as this code, because the (relatively) small number of
> cases where JDK 16 threw NegativeArrayIndexException are caught and allowed only for releases < 17
> So where you see those in the test it corresponds to the behavioral changes from NegativeArrayIndexException
> to IllegalArgumentException.
> JCK conformance tests still pass so they must not test those conditions.

src/java.desktop/share/classes/java/awt/image/ComponentSampleModel.java line 127:

> 125:      * @throws IllegalArgumentException if {@code pixelStride} is less than 0
> 126:      * @throws IllegalArgumentException if {@code scanlineStride} is less than 0
> 127:      * @throws NullPointerException if {@code bandOffsets} is {@code null}

I like the NPE here, but probably it makes sense to use the same exception for all errors? "IllegalArgumentException "

-------------

PR: https://git.openjdk.java.net/jdk/pull/3223
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8255800: Raster creation methods need some specification clean up

Phil Race
On Fri, 26 Mar 2021 20:48:19 GMT, Sergey Bylokhov <[hidden email]> wrote:

>> https://bugs.openjdk.java.net/browse/JDK-8255800 could have been a one line spec clean up but
>> it didn't take a lot of looking to realize there were many more inconsistencies between spec and implementation.
>> I've spent a lot of time on what is just small number of factory methods in Raster because there are so
>> many possible exceptions and in some cases they rely on other API they call to generate exceptions and
>> these may have not been documented or documented acc. to some long lost behavior.
>> I've mostly tried to ONLY change spec. But I couldn't help myself when some checks were missed that
>> ended up with bizarre and dubious behavior - throwing NegativeArrayIndexException which just about
>> always has to be an internal bug !
>>
>> The supplied test passes on JDK 16 as well as this code, because the (relatively) small number of
>> cases where JDK 16 threw NegativeArrayIndexException are caught and allowed only for releases < 17
>> So where you see those in the test it corresponds to the behavioral changes from NegativeArrayIndexException
>> to IllegalArgumentException.
>> JCK conformance tests still pass so they must not test those conditions.
>
> src/java.desktop/share/classes/java/awt/image/ComponentSampleModel.java line 127:
>
>> 125:      * @throws IllegalArgumentException if {@code pixelStride} is less than 0
>> 126:      * @throws IllegalArgumentException if {@code scanlineStride} is less than 0
>> 127:      * @throws NullPointerException if {@code bandOffsets} is {@code null}
>
> I like the NPE here, but probably it makes sense to use the same exception for all errors? "IllegalArgumentException "

Meaning even though you like the NPE you would change it to IAE like the rest ?

-------------

PR: https://git.openjdk.java.net/jdk/pull/3223
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8255800: Raster creation methods need some specification clean up

Sergey Bylokhov-2
In reply to this post by Phil Race
On Fri, 26 Mar 2021 19:53:44 GMT, Phil Race <[hidden email]> wrote:

> https://bugs.openjdk.java.net/browse/JDK-8255800 could have been a one line spec clean up but
> it didn't take a lot of looking to realize there were many more inconsistencies between spec and implementation.
> I've spent a lot of time on what is just small number of factory methods in Raster because there are so
> many possible exceptions and in some cases they rely on other API they call to generate exceptions and
> these may have not been documented or documented acc. to some long lost behavior.
> I've mostly tried to ONLY change spec. But I couldn't help myself when some checks were missed that
> ended up with bizarre and dubious behavior - throwing NegativeArrayIndexException which just about
> always has to be an internal bug !
>
> The supplied test passes on JDK 16 as well as this code, because the (relatively) small number of
> cases where JDK 16 threw NegativeArrayIndexException are caught and allowed only for releases < 17
> So where you see those in the test it corresponds to the behavioral changes from NegativeArrayIndexException
> to IllegalArgumentException.
> JCK conformance tests still pass so they must not test those conditions.

src/java.desktop/share/classes/java/awt/image/Raster.java line 263:

> 261:      *         both > 0
> 262:      * @throws IllegalArgumentException if the product of {@code w}
> 263:      *         and {@code h} is greater than {@code Integer.MAX_VALUE}

Do we really want to specify this? I hit this bug during HiDPI work and it seems this is an implementation bug that we do not support big size(w*h).

src/java.desktop/share/classes/java/awt/image/Raster.java line 337:

> 335:      *         {@code DataBuffer.TYPE_BYTE},
> 336:      *         {@code DataBuffer.TYPE_USHORT}
> 337:      *         or {@code DataBuffer.TYPE_INT}

What about other types like TYPE_SHORT or we assume it is unsupported?

-------------

PR: https://git.openjdk.java.net/jdk/pull/3223
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8255800: Raster creation methods need some specification clean up

Sergey Bylokhov-2
In reply to this post by Phil Race
On Fri, 26 Mar 2021 22:45:56 GMT, Phil Race <[hidden email]> wrote:

>> src/java.desktop/share/classes/java/awt/image/ComponentSampleModel.java line 127:
>>
>>> 125:      * @throws IllegalArgumentException if {@code pixelStride} is less than 0
>>> 126:      * @throws IllegalArgumentException if {@code scanlineStride} is less than 0
>>> 127:      * @throws NullPointerException if {@code bandOffsets} is {@code null}
>>
>> I like the NPE here, but probably it makes sense to use the same exception for all errors? "IllegalArgumentException "
>
> Meaning even though you like the NPE you would change it to IAE like the rest ?

Yes and no, I just wanted to highlight this place, since somebody may prefer IAE here.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3223
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8255800: Raster creation methods need some specification clean up

Phil Race
On Wed, 31 Mar 2021 21:17:07 GMT, Sergey Bylokhov <[hidden email]> wrote:

>> Meaning even though you like the NPE you would change it to IAE like the rest ?
>
> Yes and no, I just wanted to highlight this place, since somebody may prefer IAE here.

Well IAE was my initial thought but I decided to minimize the behavioral changes.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3223
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8255800: Raster creation methods need some specification clean up

Phil Race
In reply to this post by Sergey Bylokhov-2
On Wed, 31 Mar 2021 21:21:24 GMT, Sergey Bylokhov <[hidden email]> wrote:

>> https://bugs.openjdk.java.net/browse/JDK-8255800 could have been a one line spec clean up but
>> it didn't take a lot of looking to realize there were many more inconsistencies between spec and implementation.
>> I've spent a lot of time on what is just small number of factory methods in Raster because there are so
>> many possible exceptions and in some cases they rely on other API they call to generate exceptions and
>> these may have not been documented or documented acc. to some long lost behavior.
>> I've mostly tried to ONLY change spec. But I couldn't help myself when some checks were missed that
>> ended up with bizarre and dubious behavior - throwing NegativeArrayIndexException which just about
>> always has to be an internal bug !
>>
>> The supplied test passes on JDK 16 as well as this code, because the (relatively) small number of
>> cases where JDK 16 threw NegativeArrayIndexException are caught and allowed only for releases < 17
>> So where you see those in the test it corresponds to the behavioral changes from NegativeArrayIndexException
>> to IllegalArgumentException.
>> JCK conformance tests still pass so they must not test those conditions.
>
> src/java.desktop/share/classes/java/awt/image/Raster.java line 263:
>
>> 261:      *         both > 0
>> 262:      * @throws IllegalArgumentException if the product of {@code w}
>> 263:      *         and {@code h} is greater than {@code Integer.MAX_VALUE}
>
> Do we really want to specify this? I hit this bug during HiDPI work and it seems this is an implementation bug that we do not support big size(w*h).

The implementation issues that constrain this are substantial.
I find it unlikely we'd find a reason to support it.
I mean a bigger issue is we can'r even support an image that's Integer.MAX_VALUE wide and 1 pixel high.

Removing this from the spec. at that time would be trivial compared to the work in supporting it.
And likely it would be some other method that would support that anyway for the aforementioned int reason.
And we also (and always have) documented an exception if adding the location.x + w would overflow an int.
So in summary, yes, let's document it.

> src/java.desktop/share/classes/java/awt/image/Raster.java line 337:
>
>> 335:      *         {@code DataBuffer.TYPE_BYTE},
>> 336:      *         {@code DataBuffer.TYPE_USHORT}
>> 337:      *         or {@code DataBuffer.TYPE_INT}
>
> What about other types like TYPE_SHORT or we assume it is unsupported?

The existing code throws IAE if it is not one of these exact types. See around line 460 in this updated file.
And that code is in a public factory method called by this factory method. And that callee has exactly this documentation. So I am just copying it over so that it is consistently documented.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3223
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8255800: Raster creation methods need some specification clean up

Sergey Bylokhov-2
In reply to this post by Phil Race
On Fri, 26 Mar 2021 19:53:44 GMT, Phil Race <[hidden email]> wrote:

> https://bugs.openjdk.java.net/browse/JDK-8255800 could have been a one line spec clean up but
> it didn't take a lot of looking to realize there were many more inconsistencies between spec and implementation.
> I've spent a lot of time on what is just small number of factory methods in Raster because there are so
> many possible exceptions and in some cases they rely on other API they call to generate exceptions and
> these may have not been documented or documented acc. to some long lost behavior.
> I've mostly tried to ONLY change spec. But I couldn't help myself when some checks were missed that
> ended up with bizarre and dubious behavior - throwing NegativeArrayIndexException which just about
> always has to be an internal bug !
>
> The supplied test passes on JDK 16 as well as this code, because the (relatively) small number of
> cases where JDK 16 threw NegativeArrayIndexException are caught and allowed only for releases < 17
> So where you see those in the test it corresponds to the behavioral changes from NegativeArrayIndexException
> to IllegalArgumentException.
> JCK conformance tests still pass so they must not test those conditions.

Marked as reviewed by serb (Reviewer).

-------------

PR: https://git.openjdk.java.net/jdk/pull/3223
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8255800: Raster creation methods need some specification clean up [v2]

Phil Race
In reply to this post by Phil Race
> https://bugs.openjdk.java.net/browse/JDK-8255800 could have been a one line spec clean up but
> it didn't take a lot of looking to realize there were many more inconsistencies between spec and implementation.
> I've spent a lot of time on what is just small number of factory methods in Raster because there are so
> many possible exceptions and in some cases they rely on other API they call to generate exceptions and
> these may have not been documented or documented acc. to some long lost behavior.
> I've mostly tried to ONLY change spec. But I couldn't help myself when some checks were missed that
> ended up with bizarre and dubious behavior - throwing NegativeArrayIndexException which just about
> always has to be an internal bug !
>
> The supplied test passes on JDK 16 as well as this code, because the (relatively) small number of
> cases where JDK 16 threw NegativeArrayIndexException are caught and allowed only for releases < 17
> So where you see those in the test it corresponds to the behavioral changes from NegativeArrayIndexException
> to IllegalArgumentException.
> JCK conformance tests still pass so they must not test those conditions.

Phil Race has updated the pull request incrementally with one additional commit since the last revision:

  8255800: Raster creation methods need some specification clean up

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3223/files
  - new: https://git.openjdk.java.net/jdk/pull/3223/files/07424a32..85148fd5

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3223&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3223&range=00-01

  Stats: 5 lines in 2 files changed: 1 ins; 3 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3223.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3223/head:pull/3223

PR: https://git.openjdk.java.net/jdk/pull/3223
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8255800: Raster creation methods need some specification clean up [v2]

Phil Race
In reply to this post by Sergey Bylokhov-2
On Thu, 1 Apr 2021 00:56:41 GMT, Sergey Bylokhov <[hidden email]> wrote:

>> Phil Race has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   8255800: Raster creation methods need some specification clean up
>
> Marked as reviewed by serb (Reviewer).

I've updated the PR with one very minor typo correction and a removal of a duplicate throws clause.
Also I've prepared a draft CSR which needs to be reviewed  https://bugs.openjdk.java.net/browse/JDK-8264625

-------------

PR: https://git.openjdk.java.net/jdk/pull/3223
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8255800: Raster creation methods need some specification clean up [v2]

Sergey Bylokhov-2
In reply to this post by Phil Race
On Thu, 1 Apr 2021 20:27:37 GMT, Phil Race <[hidden email]> wrote:

>> https://bugs.openjdk.java.net/browse/JDK-8255800 could have been a one line spec clean up but
>> it didn't take a lot of looking to realize there were many more inconsistencies between spec and implementation.
>> I've spent a lot of time on what is just small number of factory methods in Raster because there are so
>> many possible exceptions and in some cases they rely on other API they call to generate exceptions and
>> these may have not been documented or documented acc. to some long lost behavior.
>> I've mostly tried to ONLY change spec. But I couldn't help myself when some checks were missed that
>> ended up with bizarre and dubious behavior - throwing NegativeArrayIndexException which just about
>> always has to be an internal bug !
>>
>> The supplied test passes on JDK 16 as well as this code, because the (relatively) small number of
>> cases where JDK 16 threw NegativeArrayIndexException are caught and allowed only for releases < 17
>> So where you see those in the test it corresponds to the behavioral changes from NegativeArrayIndexException
>> to IllegalArgumentException.
>> JCK conformance tests still pass so they must not test those conditions.
>
> Phil Race has updated the pull request incrementally with one additional commit since the last revision:
>
>   8255800: Raster creation methods need some specification clean up

Marked as reviewed by serb (Reviewer).

-------------

PR: https://git.openjdk.java.net/jdk/pull/3223
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8255800: Raster creation methods need some specification clean up [v3]

Phil Race
In reply to this post by Phil Race
> https://bugs.openjdk.java.net/browse/JDK-8255800 could have been a one line spec clean up but
> it didn't take a lot of looking to realize there were many more inconsistencies between spec and implementation.
> I've spent a lot of time on what is just small number of factory methods in Raster because there are so
> many possible exceptions and in some cases they rely on other API they call to generate exceptions and
> these may have not been documented or documented acc. to some long lost behavior.
> I've mostly tried to ONLY change spec. But I couldn't help myself when some checks were missed that
> ended up with bizarre and dubious behavior - throwing NegativeArrayIndexException which just about
> always has to be an internal bug !
>
> The supplied test passes on JDK 16 as well as this code, because the (relatively) small number of
> cases where JDK 16 threw NegativeArrayIndexException are caught and allowed only for releases < 17
> So where you see those in the test it corresponds to the behavioral changes from NegativeArrayIndexException
> to IllegalArgumentException.
> JCK conformance tests still pass so they must not test those conditions.

Phil Race has updated the pull request incrementally with one additional commit since the last revision:

  8255800: Raster creation methods need some specification clean up

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3223/files
  - new: https://git.openjdk.java.net/jdk/pull/3223/files/85148fd5..ba2ab76c

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3223&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3223&range=01-02

  Stats: 29 lines in 2 files changed: 27 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3223.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3223/head:pull/3223

PR: https://git.openjdk.java.net/jdk/pull/3223
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8255800: Raster creation methods need some specification clean up [v2]

Phil Race
In reply to this post by Sergey Bylokhov-2
On Fri, 2 Apr 2021 21:03:30 GMT, Sergey Bylokhov <[hidden email]> wrote:

>> Phil Race has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   8255800: Raster creation methods need some specification clean up
>
> Marked as reviewed by serb (Reviewer).

I updated the fix with some additional checks. This is implementation only - doesn't affect the spec clarification.
The problem was that without these new upfront checks for overflow of width+location.x, then on systems without
enough memory/swap you'd get an OOME before you got to the place where overflow was checked.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3223
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8255800: Raster creation methods need some specification clean up [v3]

Sergey Bylokhov-2
In reply to this post by Phil Race
On Fri, 2 Apr 2021 22:40:53 GMT, Phil Race <[hidden email]> wrote:

>> https://bugs.openjdk.java.net/browse/JDK-8255800 could have been a one line spec clean up but
>> it didn't take a lot of looking to realize there were many more inconsistencies between spec and implementation.
>> I've spent a lot of time on what is just small number of factory methods in Raster because there are so
>> many possible exceptions and in some cases they rely on other API they call to generate exceptions and
>> these may have not been documented or documented acc. to some long lost behavior.
>> I've mostly tried to ONLY change spec. But I couldn't help myself when some checks were missed that
>> ended up with bizarre and dubious behavior - throwing NegativeArrayIndexException which just about
>> always has to be an internal bug !
>>
>> The supplied test passes on JDK 16 as well as this code, because the (relatively) small number of
>> cases where JDK 16 threw NegativeArrayIndexException are caught and allowed only for releases < 17
>> So where you see those in the test it corresponds to the behavioral changes from NegativeArrayIndexException
>> to IllegalArgumentException.
>> JCK conformance tests still pass so they must not test those conditions.
>
> Phil Race has updated the pull request incrementally with one additional commit since the last revision:
>
>   8255800: Raster creation methods need some specification clean up

Marked as reviewed by serb (Reviewer).

-------------

PR: https://git.openjdk.java.net/jdk/pull/3223
Reply | Threaded
Open this post in threaded view
|

<AWT Dev> Integrated: 8255800: Raster creation methods need some specification clean up

Phil Race
In reply to this post by Phil Race
On Fri, 26 Mar 2021 19:53:44 GMT, Phil Race <[hidden email]> wrote:

> https://bugs.openjdk.java.net/browse/JDK-8255800 could have been a one line spec clean up but
> it didn't take a lot of looking to realize there were many more inconsistencies between spec and implementation.
> I've spent a lot of time on what is just small number of factory methods in Raster because there are so
> many possible exceptions and in some cases they rely on other API they call to generate exceptions and
> these may have not been documented or documented acc. to some long lost behavior.
> I've mostly tried to ONLY change spec. But I couldn't help myself when some checks were missed that
> ended up with bizarre and dubious behavior - throwing NegativeArrayIndexException which just about
> always has to be an internal bug !
>
> The supplied test passes on JDK 16 as well as this code, because the (relatively) small number of
> cases where JDK 16 threw NegativeArrayIndexException are caught and allowed only for releases < 17
> So where you see those in the test it corresponds to the behavioral changes from NegativeArrayIndexException
> to IllegalArgumentException.
> JCK conformance tests still pass so they must not test those conditions.

This pull request has now been integrated.

Changeset: adb860ec
Author:    Phil Race <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/adb860ec
Stats:     1494 lines in 5 files changed: 1424 ins; 35 del; 35 mod

8255800: Raster creation methods need some specification clean up

Reviewed-by: serb

-------------

PR: https://git.openjdk.java.net/jdk/pull/3223