RFR: 8159055: Clarify handling of null and invalid image data for ImageIcon constructors and setImage method [v25]

[Alexey Ivanov]

On Sun, 3 Aug 2025 06:18:14 GMT, Prasanta Sadhukhan <psadhukhan at openjdk.org> wrote:

>> When trying to call 'icon.setImage(null);' where 'icon' is an instance of ImageIcon, a null pointer exception is thrown at runtime.
>> The code tried to get the `id` for that image and instantiates `MediaTracker` to associate the null image to that `id` and checks the status of loading this null image, removes the null image from the tracker and then tries to get the image width where it throws NPE as image is null.
>> 
>> It's better to not go through all MediaTracker usage and bail out initially itself for null image..
>
> Prasanta Sadhukhan has updated the pull request incrementally with one additional commit since the last revision:
> 
>   class javadoc modification

Changes requested by aivanov (Reviewer).

src/java.desktop/share/classes/javax/swing/ImageIcon.java line 71:

> 69:  * even though {@link #getImage()} will return a non-null value,
> 70:  * as the image will have no dimensions
> 71:  * and {@link #getImageLoadStatus()} will report {@code MediaTracker.ERRORED}.

What do you think about this version?
Suggestion:

 * If the image source parameter provided to a constructor or method is non-{@code null}
 * but does not reference valid or accessible image data, no exceptions will be thrown.
 * In this case, {@link #getImage()} will return a non-{@code null} value, but the image
 * will have no dimensions, and nothing will be rendered.
 * Additionally, {@link #getImageLoadStatus()} will report {@code MediaTracker.ERRORED}.

Shorter sentences are easier to read, the statements are clearer.

test/jdk/javax/swing/ImageIcon/ImageIconTest.java line 74:

> 72:                            } else if (v == ArgVal.INVALID_DATA) {
> 73:                                expected = false;
> 74:                                new ImageIcon("file://" + imgName, "gif");

There's a discrepancy: `null` is tested with one-arg constructor whereas invalid data is tested with two-arg constructor which accepts the description in addition to the URL.

More over, `new ImageIcon("file://" + imgName, "gif")` does not call `ImageIcon(URL, String)` constructor, it calls `ImageIcon(String, String)`.

test/jdk/javax/swing/ImageIcon/ImageIconTest.java line 84:

> 82:                            } else if (v == ArgVal.INVALID_DATA) {
> 83:                                expected = false;
> 84:                                new ImageIcon(new byte[0], "gif");

Why not `new ImageIcon(new byte[0])`?

Why is it `new byte[0]` instead of `invalidData` that you seemingly created for the purpose of having invalid data?

test/jdk/javax/swing/ImageIcon/ImageIconTest.java line 90:

> 88:                        case IMAGE :
> 89:                            if (v == ArgVal.NULL) {
> 90:                                new ImageIcon((Image)null);

Suggestion:

                               new ImageIcon((Image) null);

I guess you advocated for having a space between the type cast operator and its argument.

test/jdk/javax/swing/ImageIcon/ImageIconTest.java line 116:

> 114:                    System.err.println("Did not receive expected exception for : " + a);
> 115:                    throw new RuntimeException("Test failed");
> 116:                 }

Suggestion:

                if (expected && !passed) {
                   throw new RuntimeException("Did not receive expected exception for: " + a);
                }

Why not throw a more meaningful error message, this would make analysing test failure easier. At the very least, you will be able to distinguish different types of failures without looking into the test log file.

test/jdk/javax/swing/ImageIcon/ImageIconTest.java line 120:

> 118:                    System.err.println("Received unexpected exception for : " + a);
> 119:                    throw new RuntimeException("Test failed");
> 120:                 }

Suggestion:

                if (!expected && !passed) {
                   throw new RuntimeException("Received unexpected exception for: " + a);
                }

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

PR Review: https://git.openjdk.org/jdk/pull/25767#pullrequestreview-3087840636
PR Review Comment: https://git.openjdk.org/jdk/pull/25767#discussion_r2254034417
PR Review Comment: https://git.openjdk.org/jdk/pull/25767#discussion_r2254046393
PR Review Comment: https://git.openjdk.org/jdk/pull/25767#discussion_r2254052610
PR Review Comment: https://git.openjdk.org/jdk/pull/25767#discussion_r2254054860
PR Review Comment: https://git.openjdk.org/jdk/pull/25767#discussion_r2254063647
PR Review Comment: https://git.openjdk.org/jdk/pull/25767#discussion_r2254065010