RFR: 8159055: ImageIcon.setImage can't handle null parameter [v22]
Alexey Ivanov
aivanov at openjdk.org
Mon Jul 28 19:47:04 UTC 2025
On Sun, 27 Jul 2025 07:08:47 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:
>
> ArgType name change
At what point did we decide not to modify `Image.setImage` to accept `null` as the parameter? It made perfect sense.
I wonder if it's possible to use JUnit a test framework in jtreg?
src/java.desktop/share/classes/javax/swing/ImageIcon.java line 64:
> 62: * are preloaded using MediaTracker to monitor the loaded state
> 63: * of the image.
> 64: * If the image source parameter to a constructor or method is non-null,
Suggestion:
* of the image.
*
* <p>
* If the image source parameter to a constructor or method is non-null,
I suggest putting the discussion on non-null parameters in its own paragraph for ease of reading the javadoc.
src/java.desktop/share/classes/javax/swing/ImageIcon.java line 68:
> 66: * no exceptions will be thrown but the image will be unset,
> 67: * as it will have no dimensions and never be drawn, and
> 68: * {@code getImageLoadStatus()} will report {@see java.awt.MediaTracker#ERRORED}.
Suggestion:
* {@code getImageLoadStatus()} will report {@link java.awt.MediaTracker#ERRORED}.
You shouldn't use `@see` here, it should either be `@code` or `@link`. I think both references should use the same tag: either `@code` or `@link`; the latter allows quickly navigating to the *method* or constant.
Perhaps, `@link` for the `getImageLoadStatus` method would work better: it allows viewing the description of the method and its purpose; in its turn, `getImageLoadStatus` references possible return values including `MediaTracker.ERRORED` with `@see` tag.
src/java.desktop/share/classes/javax/swing/ImageIcon.java line 186:
> 184: * @param location the URL for the image
> 185: * @param description a brief textual description of the image
> 186: * @throws {@code NullPointerException} if (@code null) URL is passed.
Suggestion:
* @throws NullPointerException if a {@code null} URL is passed
The exception type is automatically rendered in monospaced font with a link to its description.
You meant to use braces instead of parentheses.
Usually, there's no full stop for `@throws`.
Alternatively, specify the parameter name for more clarity:
Suggestion:
* @throws NullPointerException if {@code location} is {@code null}
src/java.desktop/share/classes/javax/swing/ImageIcon.java line 206:
> 204: * a string representation of the URL.
> 205: * @param location the URL for the image
> 206: * @throws {@code NullPointerException} if (@code null) URL is passed.
Suggestion:
* @throws NullPointerException if a {@code null} URL is passed
src/java.desktop/share/classes/javax/swing/ImageIcon.java line 217:
> 215: * @param image the image
> 216: * @param description a brief textual description of the image
> 217: * @throws {@code NullPointerException} if (@code null) Image is passed.
Suggestion:
* @throws NullPointerException if a {@code null} image is passed
Here, image shouldn't be capitalised, you mean it as the literal _“image”_ rather than a type.
I still prefer specifying the parameter name:
Suggestion:
* @throws NullPointerException if {@code image} is {@code null}
src/java.desktop/share/classes/javax/swing/ImageIcon.java line 229:
> 227: * then the string is used as the description of this icon.
> 228: * @param image the image
> 229: * @throws {@code NullPointerException} if (@code null) Image is passed.
Suggestion:
* @throws NullPointerException if {@code image} is {@code null}
src/java.desktop/share/classes/javax/swing/ImageIcon.java line 253:
> 251: * by the AWT Toolkit, such as GIF, JPEG, or (as of 1.3) PNG
> 252: * @param description a brief textual description of the image
> 253: * @throws {@code NullPointerException} if (@code null) imageData is passed.
Suggestion:
* @throws NullPointerException if {@code imageData} is {@code null}
src/java.desktop/share/classes/javax/swing/ImageIcon.java line 277:
> 275: * @param imageData an array of pixels in an image format supported by
> 276: * the AWT Toolkit, such as GIF, JPEG, or (as of 1.3) PNG
> 277: * @throws {@code NullPointerException} if (@code null) imageData is passed.
Suggestion:
* @throws NullPointerException if {@code imageData} is {@code null}
src/java.desktop/share/classes/javax/swing/ImageIcon.java line 382:
> 380: * Sets the image displayed by this icon.
> 381: * @param image the image
> 382: * @throws {@code NullPointerException} if (@code null) Image is passed.
Suggestion:
* @throws NullPointerException if {@code image} is {@code null}
test/jdk/javax/swing/ImageIcon/ImageIconTest.java line 39:
> 37: import javax.swing.ImageIcon;
> 38:
> 39: public class ImageIconTest {
Suggestion:
public class ImageIconNullParametersTest {
Be more specific?
test/jdk/javax/swing/ImageIcon/ImageIconTest.java line 52:
> 50: fos.write(invalidData);
> 51: }
> 52: String fileName = (new File(System.getProperty("test.src", "."), imgName)).getName();
Why do we need jumping over the hoops?
You're creating an image, put it into the current directory that defaults to `scratch` directory in jtreg. The `scratch` directory is automatically cleaned by jtreg between tests, thus you won't even leave files in the source tree of JDK.
test/jdk/javax/swing/ImageIcon/ImageIconTest.java line 66:
> 64: new ImageIcon(s);
> 65: passed = true; // no exception expected for this case
> 66: break;
Does it make sense to create richer enum values which combine the expected state?
-------------
Changes requested by aivanov (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/25767#pullrequestreview-3064243188
PR Review Comment: https://git.openjdk.org/jdk/pull/25767#discussion_r2237595098
PR Review Comment: https://git.openjdk.org/jdk/pull/25767#discussion_r2237623088
PR Review Comment: https://git.openjdk.org/jdk/pull/25767#discussion_r2237628892
PR Review Comment: https://git.openjdk.org/jdk/pull/25767#discussion_r2237634552
PR Review Comment: https://git.openjdk.org/jdk/pull/25767#discussion_r2237651047
PR Review Comment: https://git.openjdk.org/jdk/pull/25767#discussion_r2237652775
PR Review Comment: https://git.openjdk.org/jdk/pull/25767#discussion_r2237657503
PR Review Comment: https://git.openjdk.org/jdk/pull/25767#discussion_r2237660636
PR Review Comment: https://git.openjdk.org/jdk/pull/25767#discussion_r2237662773
PR Review Comment: https://git.openjdk.org/jdk/pull/25767#discussion_r2237696278
PR Review Comment: https://git.openjdk.org/jdk/pull/25767#discussion_r2237685414
PR Review Comment: https://git.openjdk.org/jdk/pull/25767#discussion_r2237693876
More information about the client-libs-dev
mailing list