8159055: ImageIcon.setImage can't handle null parameter #25767

prsadhuk · 2025-06-12T03:23:30Z

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..

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change requires CSR request JDK-8359398 to be approved
Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)

Issues

JDK-8159055: ImageIcon.setImage can't handle null parameter (Enhancement - P4)
JDK-8359398: ImageIcon.setImage can't handle null parameter (CSR)

Reviewers

Abhishek Kumar (@kumarabhi006 - Reviewer) 🔄 Re-review required (review applies to 8e1938b6)
Alexander Zuev (@azuev-java - Reviewer) 🔄 Re-review required (review applies to bcdef9b4)
Alexey Ivanov (@aivanov-jdk - Reviewer) 🔄 Re-review required (review applies to 340254ec)
Sergey Bylokhov (@mrserb - Reviewer) 🔄 Re-review required (review applies to 6f6320ac)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25767/head:pull/25767
$ git checkout pull/25767

Update a local copy of the PR:
$ git checkout pull/25767
$ git pull https://git.openjdk.org/jdk.git pull/25767/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25767

View PR using the GUI difftool:
$ git pr show -t 25767

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25767.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-06-12T03:24:20Z

👋 Welcome back psadhukhan! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-06-12T03:24:54Z

@prsadhuk This change is no longer ready for integration - check the PR body for details.

openjdk · 2025-06-12T03:25:30Z

@prsadhuk The following label will be automatically applied to this pull request:

client

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2025-06-12T03:28:40Z

Webrevs

kumarabhi006 · 2025-06-12T06:02:05Z

Does it require to update the doc as well to explicitly mention about NULL parameter ?
Otherwise fix looks ok to me.

prsadhuk · 2025-06-12T06:31:04Z

I feel it's better to inform the user in the spec otherwise application may not know what will be done with NULL image

kumarabhi006 · 2025-06-12T06:53:10Z

I feel it's better to inform the user in the spec otherwise application may not know what will be done with NULL image

CSR needed ?

prsadhuk · 2025-06-12T06:55:12Z

yes..i will raise it once we are done with this PR approval..

kumarabhi006 · 2025-06-12T06:57:20Z

yes..i will raise it once we are done with this PR approval..

Ok...
Can't we just mention in the javadoc that null image will throw an NPE and then the code changes are not required?

prsadhuk · 2025-06-12T07:00:34Z

yes..i will raise it once we are done with this PR approval..

Ok... Can't we just mention in the javadoc that null image will throw an NPE and then the code changes are not required?

No, as I mentioned in the description the MediaTracker creation and id addition, tracking and removal is supposedly not required for null image.

kumarabhi006

LGTM

azuev-java

Looks good.

src/java.desktop/share/classes/javax/swing/ImageIcon.java

prsadhuk · 2025-06-13T03:42:15Z

Raised CSR https://bugs.openjdk.org/browse/JDK-8359398
@azuev-java @kumarabhi006 Please review.

src/java.desktop/share/classes/javax/swing/ImageIcon.java

aivanov-jdk

The ImageIcon constructor that accepts Image has the same problem, or a similar problem.

jdk/src/java.desktop/share/classes/javax/swing/ImageIcon.java

Lines 224 to 226 in 9d06057

    
           public ImageIcon (Image image) { 
        
               this.image = image; 
        
               Object o = image.getProperty("comment", imageObserver);

It throws NullPointerException if a null parameter is passed. Other constructors handle null values, so you should unify handling of null images.

test/jdk/javax/swing/ImageIcon/ImageIconNullImageTest.java

src/java.desktop/share/classes/javax/swing/ImageIcon.java

test/jdk/javax/swing/ImageIcon/ImageIconNullImageTest.java

src/java.desktop/share/classes/javax/swing/ImageIcon.java

test/jdk/javax/swing/ImageIcon/ImageIconNullImageTest.java

src/java.desktop/share/classes/javax/swing/ImageIcon.java

aivanov-jdk · 2025-06-18T17:38:22Z

src/java.desktop/share/classes/javax/swing/ImageIcon.java

@@ -367,10 +371,14 @@ public Image getImage() {

    /**
     * Sets the image displayed by this icon.
+     * Setting a {@code null} image will not render any image icon.


Suggested change

* Setting a {@code null} image will not render any image icon.

* Setting a {@code null} image renders no icon.

I think this is better: it's shorter, it uses the present tense because it's fact and it doesn't depend on anything.

I think the existing text was OK. And "will" is better because it hasn't happened yet.
Rendering will happen later. Not when you construct this object.
My personally preferred wording would be something like
Setting a {@code null} image means any existing image will be removed and no image will be rendered.

Now .. what should happen to an existing description when you set a new image ?

Apps can call setDescription() but in what order ?

The only thing I can say here is that I think if the image is null we probably should clear the description.

I have modified the wording as per suggestion and also clearing the description now if image is null..

src/java.desktop/share/classes/javax/swing/ImageIcon.java

test/jdk/javax/swing/ImageIcon/ImageIconNullImageTest.java

prrace · 2025-06-18T22:18:45Z

src/java.desktop/share/classes/javax/swing/ImageIcon.java

@@ -367,10 +371,14 @@ public Image getImage() {

    /**
     * Sets the image displayed by this icon.
+     * Setting a {@code null} image will not render any image icon.


I think the existing text was OK. And "will" is better because it hasn't happened yet.
Rendering will happen later. Not when you construct this object.
My personally preferred wording would be something like
Setting a {@code null} image means any existing image will be removed and no image will be rendered.

Now .. what should happen to an existing description when you set a new image ?

Apps can call setDescription() but in what order ?

The only thing I can say here is that I think if the image is null we probably should clear the description.

prrace · 2025-06-18T22:21:22Z

src/java.desktop/share/classes/javax/swing/ImageIcon.java

@@ -223,6 +224,9 @@ public ImageIcon(Image image, String description) {
     */
    public ImageIcon (Image image) {
        this.image = image;
+        if (image == null) {
+            return;
+        }
        Object o = image.getProperty("comment", imageObserver);
        if (o instanceof String) {
            description = (String) o;


Hmm. This is wasted work if the app calls ImageIcon(Image, String) because that promptly over-writes whatever was obtained via this code.

Also that other constructor (which github won't let me comment on) installs the description even if the image is null.
That seems to be inconsistent with everything else that has a null description for a null image.

Also that other constructor (which github won't let me comment on) installs the description even if the image is null.

I don't see it as a problem.

If the app developer wants to initialise the object with null image and a description, why don't we let them do it?

The image may be auto-generated and while it's generated, the image in ImageIcon remains null; when the image is read the app calls ImageIcon.setImage to the set now available image and expects the description being preserved.

Now .. what should happen to an existing description when you set a new image ?

Apps can call setDescription() but in what order ?

The only thing I can say here is that I think if the image is null we probably should clear the description.

Nothing should happen to the description. These are two fields which can be changed independently now, why would we enforce an order on method calls?

Changing the image shouldn't change the description; similarly, changing description doesn't change the image.

Also that other constructor (which github won't let me comment on) installs the description even if the image is null.

I don't see it as a problem.

If the app developer wants to initialise the object with null image and a description, why don't we let them do it?

Because that would be inconsistent with every other constructor.

See
ImagIcon(String filename)
ImageIcon(String filename, String description)
ImageIcon(String URL)
ImageIcon(String URL, String description)
ImageIcon(byte[] imageData)
ImageIcon(byte[] imageData, String description)

None of these set the description if the image resolves to null.

Also clearly
ImageIcon(Image image)
and
ImageIcon(Image image, String description)
where image == null can't be doing it today
because a NPE is thrown before it gets there.
So we can have that case do whatever we want if we allow a null image.
And consistency seems the best choice.

And consistency seems the best choice.

Then our best choice is not to touch ImageIcon(Image) and leave it to throw NullPointerException.

All the constructors except for ImageIcon((String) null) throw NullPointerException if the first argument is null.

Strangely enough, ImageIcon((String) null) creates a non-null image.

If we leave it to throw NPE, then JCK might crib, if at some point they decide to add test to test null image and since javadoc doesn't mention it can throw NPE, it will result in non-compliance and then we have to modify javadoc to clarify it, so I guess it's best to take preventive action now..

If we leave it to throw NPE, then JCK might crib, if at some point they decide to add test to test null image and since javadoc doesn't mention it can throw NPE, it will result in non-compliance and then we have to modify javadoc to clarify it, so I guess it's best to take preventive action now..

But it is consistent with the current behaviour and it is not specified at the moment.

If we explicitly specify the constructors throw NPE if a null parameter is passed, then JCK will add a test that confirms the specified behaviour. All the constructors come into picture then, which seems out of scope for this issue…

If we don't change the behaviour of constructors and JCK challenges the implementation, then such a JCK test has no foundation in the API specification; yet it also leads to JCK challenge of the clarity of the specification.

ExE-Boss

These are not needed as the initial value of description is null:

src/java.desktop/share/classes/javax/swing/ImageIcon.java

aivanov-jdk · 2025-06-19T10:28:53Z

/reviewers 2 reviewer

openjdk · 2025-06-19T10:29:17Z

@aivanov-jdk
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 2 Reviewers).

prsadhuk · 2025-06-20T03:50:53Z

I have removed resetting "description" from constructor as it is already null there..Retained in setImage and reset width/height also..

src/java.desktop/share/classes/javax/swing/ImageIcon.java

aivanov-jdk · 2025-06-20T11:55:35Z

src/java.desktop/share/classes/javax/swing/ImageIcon.java

+        if (image == null) {
+            return;
+        }


So, are we changing the constructors that accept Image?

Are we changing the constructor to accept null as the Image parameter?

There's a long thread about consistent behaviour of the ImageIcon constructors, yet no decision has been taken.

@prrace mentioned consistency among constructors, and ImageIcon constructors consistently throw NullPointerException for null image. In fact, he referred to setting the description field, but this is moot if a constructor throws.

src/java.desktop/share/classes/javax/swing/ImageIcon.java

aivanov-jdk · 2025-06-20T15:29:56Z

src/java.desktop/share/classes/javax/swing/ImageIcon.java

        Object o = image.getProperty("comment", imageObserver);
        if (o instanceof String) {
            description = (String) o;


To address Phil's comment:

This is wasted work if the app calls ImageIcon(Image, String) because that promptly over-writes whatever was obtained via this code.

If we're going to change the constructors, to avoid this wasted work when ImageIcon(Image, String) constructor is called, I suggest moving the work into ImageIcon(Image, String) and implement ImageIcon like this:

public ImageIcon (Image image) { String description = null; if (image != null) { Object o = image.getProperty("comment", null); if (o instanceof String) { description = (String) o; } } this(image, description);

It is allowed in JDK 22 and later.

Note that this was a preview language feature until JDK 25.

I haven't done my due diligence to verify whether this feature can be used. Even if this particular way can't be used, it's still a viable option, just move the code above into a helper method.

public ImageIcon(Image image, String description) { this.image = image; this.description = description; loadImage(image); } public ImageIcon (Image image) { this(image, getImageComment(image)); } /** * {@return the {@code "comment"} property of the image * if the value of the property is a sting} * @param image the image to get the {@code "comment"} property */ private static String getImageComment(Image image) { if (image == null) { return null; } Object o = image.getProperty("comment", null); return (o instanceof String) ? (String) o : null; }

This should be done separately from this changeset, I think, to have shorter, more specific changes.

Note that this was a preview language feature until JDK 25.

Huh… We're developing JDK 26 now, so the Flexible Constructor Bodies [JEP 513] feature can safely be used.

Yes, but it complicates backports.

Yes, but it complicates backports.

Yes, it complicates backports, but this implies backports are possible. If the specification is updated, the backports will likely be impossible.

prsadhuk · 2025-07-03T06:46:25Z

Both image and description are two independent fields of ImageIcon object, each has its own getter and setter, and each can be changed independently. We should not enforce the order of calls: if an app developer wants to set the image to null, temporarily or not, the value of the description has to be preserved

I guess resetting the description came from the fact that description is mentioned as "Sets the description of the image. This is meant to be a brief textual description of the object." so if image is null, the description of the image logically should be nothing.

But again, as pointed out, description has its own setter and getter so user would expect whatever description is stored either by ImageIcon(Image image, String description) constructor or setDescription would be returned same via getDescription..
If not, the spec for set and getDescription needs to be modified to to incorporate null-image scenario.
But all-in-all I guess this description change can go in separate PR along with above change

aivanov-jdk · 2025-07-03T09:43:34Z

Both image and description are two independent fields of ImageIcon object, each has its own getter and setter, and each can be changed independently. We should not enforce the order of calls: if an app developer wants to set the image to null, temporarily or not, the value of the description has to be preserved

I guess resetting the description came from the fact that description is mentioned as "Sets the description of the image. This is meant to be a brief textual description of the object." so if image is null, the description of the image logically should be nothing.

I agree that a description for a null image doesn't make sense.

Then, it also depends on how it's really handled. If the image in the ImageIcon object is null, the description could be ignored.

But again, as pointed out, description has its own setter and getter so user would expect whatever description is stored either by ImageIcon(Image image, String description) constructor or setDescription would be returned same via getDescription.. If not, the spec for set and getDescription needs to be modified to to incorporate null-image scenario.

Exactly.

Updating the specification for setDescription and getDescription to indicate the description will be set to null if the image is set to null will make the API cumbersome and less clear.

I prefer leaving the description intact.

But all-in-all I guess this description change can go in separate PR along with above change

Have anyone submit a bug to address this deficiency? If not, I'll submit one.

prrace · 2025-07-07T22:43:21Z

I think we need to have the "whole picture" tested to make sure everything does as we expect.
It isn't so easy to tell even from looking at the source of ImageIcon.
Then spec'ed so developers know what to expect.
When I wrote a little test, I see null args do generate NPEs but invalid args don't result in null images
Toolkit images are installed, even if they can't be used because the source isn't valid.
They are "effectively" null, but not "actually" null.
We should at least specify the null arg behaviours and try to say something about invalid image data.
And as a result, I am now flipping to just documenting the setImage(null) NPE
The protected method loadImage probably needs to say it too.
Invalid image data, maybe we can cover that in a class level statement.
Note: I'd like to see every constructor tested with both null and invalid image data.

prsadhuk · 2025-07-08T06:00:21Z

I think we need to have the "whole picture" tested to make sure everything does as we expect. It isn't so easy to tell even from looking at the source of ImageIcon. Then spec'ed so developers know what to expect. When I wrote a little test, I see null args do generate NPEs but invalid args don't result in null images Toolkit images are installed, even if they can't be used because the source isn't valid. They are "effectively" null, but not "actually" null. We should at least specify the null arg behaviours and try to say something about invalid image data. And as a result, I am now flipping to just documenting the setImage(null) NPE The protected method loadImage probably needs to say it too. Invalid image data, maybe we can cover that in a class level statement. Note: I'd like to see every constructor tested with both null and invalid image data.

I tested with null image/imagedata and invalid imagedata and it seems only these constructors throw NPE (current JDK) for null image (invalid image doesnt throw any exception)

ImageIcon(image, desc)
ImageIcon(image)
ImageIcon(byte[], desc)
ImageIcon(byte[])

Current PR checks for null in ImageIcon(image) so it will not throw NPE but ImageIcon(byte[]) will still throw..
Should we handle those in separate PR and let this only be for setImage(null)?

aivanov-jdk · 2025-07-08T12:56:07Z

test/jdk/javax/swing/ImageIcon/ImageIconNullImageTest.java

+import java.awt.Image;
+import javax.swing.ImageIcon;
+
+public class ImageIconNullImageTest {


I guess we're going to update the test later with follow-up fixes…

However, I suggest dropping ImageIcon- from the test class name — the test is in ImageIcon folder, therefore it tests ImageIcon.

aivanov-jdk · 2025-07-08T16:40:37Z

I think we need to have the "whole picture" tested to make sure everything does as we expect. …
When I wrote a little test, I see null args do generate NPEs but invalid args don't result in null images
Toolkit images are installed, even if they can't be used because the source isn't valid. They are "effectively" null, but not "actually" null.

I tested with null image/imagedata and invalid imagedata and it seems only these constructors throw NPE (current JDK) for null image (invalid image doesnt throw any exception)

I wrote a JUnit test, it's more convenient than a jtreg test:

ImageIconConstructorsTest.java

import java.awt.Image;
import java.net.URL;

import javax.swing.ImageIcon;

import org.junit.jupiter.api.Test;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertThrows;

public class ImageIconConstructorsTest {
    @Test
    public void noArgs() {
        ImageIcon im = new ImageIcon();
        assertNull(im.getImage());
        assertNull(im.getDescription());
    }

    @Test
    public void stringNull() {
        ImageIcon im = new ImageIcon((String) null);
        assertNotNull(im.getImage());
        assertNull(im.getDescription());
    }

    @Test
    public void stringNullNull() {
        ImageIcon im = new ImageIcon((String) null, null);
        assertNotNull(im.getImage());
        assertNull(im.getDescription());
    }

    @Test
    public void stringNullWithDescription() {
        ImageIcon im = new ImageIcon((String) null, "Description");
        assertNotNull(im.getImage());
        assertEquals("Description", im.getDescription());
    }

    @Test
    public void byteArrayNull() {
        assertThrows(NullPointerException.class,
                     () -> new ImageIcon((byte[]) null));
//        ImageIcon im = new ImageIcon((byte[]) null);
//        assertNull(im.getImage());
//        assertNull(im.getDescription());
    }

    @Test
    public void byteArrayNullNull() {
        assertThrows(NullPointerException.class,
                     () -> new ImageIcon((byte[]) null, null));
//        ImageIcon im = new ImageIcon((byte[]) null, null);
//        assertNull(im.getImage());
//        assertNull(im.getDescription());
    }

    @Test
    public void byteArrayNullWithDescription() {
        assertThrows(NullPointerException.class,
                     () -> new ImageIcon((byte[]) null, "Description"));
//        ImageIcon im = new ImageIcon((byte[]) null, "Description");
//        assertNull(im.getImage());
//        assertNull(im.getDescription());
    }

    @Test
    public void urlNull() {
        assertThrows(NullPointerException.class,
                     () -> new ImageIcon((URL) null));
//        ImageIcon im = new ImageIcon((URL) null);
//        assertNull(im.getImage());
//        assertNull(im.getDescription());
    }

    @Test
    public void urlNullNull() {
        assertThrows(NullPointerException.class,
                     () -> new ImageIcon((URL) null, null));
//        ImageIcon im = new ImageIcon((URL) null, null);
//        assertNull(im.getImage());
//        assertNull(im.getDescription());
    }

    @Test
    public void urlNullWithDescription() {
        assertThrows(NullPointerException.class,
                     () -> new ImageIcon((URL) null, "Description"));
//        ImageIcon im = new ImageIcon((URL) null, "Description");
//        assertNull(im.getImage());
//        assertNull(im.getDescription());
    }

    @Test
    public void imageNull() {
        assertThrows(NullPointerException.class,
                     () -> new ImageIcon((Image) null));
//        ImageIcon im = new ImageIcon((Image) null);
//        assertNull(im.getImage());
//        assertNull(im.getDescription());
    }

    @Test
    public void imageNullNull() {
        assertThrows(NullPointerException.class,
                     () -> new ImageIcon((Image) null, null));
//        ImageIcon im = new ImageIcon((Image) null, null);
//        assertNull(im.getImage());
//        assertNull(im.getDescription());
    }

    @Test
    public void imageNullWithDescription() {
        assertThrows(NullPointerException.class,
                     () -> new ImageIcon((Image) null, "Description"));
//        ImageIcon im = new ImageIcon((Image) null, "Description");
//        assertNull(im.getImage());
//        assertNull(im.getDescription());
    }

}

All the test cases currently pass, and most constructors throw NPE for null. I haven't tested invalid arguments.

aivanov-jdk · 2025-07-08T17:18:49Z

But all-in-all I guess this description change can go in separate PR along with above change

Have anyone submit a bug to address this deficiency? If not, I'll submit one.

I submitted JDK-8361610: Avoid wasted work in ImageIcon(Image) for setting description for updating ImageIcon(Image) and ImageIcon(Image, String) constructors.

prrace · 2025-07-11T18:50:45Z

I think we need to have the "whole picture" tested to make sure everything does as we expect. It isn't so easy to tell even from looking at the source of ImageIcon. Then spec'ed so developers know what to expect. When I wrote a little test, I see null args do generate NPEs but invalid args don't result in null images Toolkit images are installed, even if they can't be used because the source isn't valid. They are "effectively" null, but not "actually" null. We should at least specify the null arg behaviours and try to say something about invalid image data. And as a result, I am now flipping to just documenting the setImage(null) NPE The protected method loadImage probably needs to say it too. Invalid image data, maybe we can cover that in a class level statement. Note: I'd like to see every constructor tested with both null and invalid image data.

I tested with null image/imagedata and invalid imagedata and it seems only these constructors throw NPE (current JDK) for null image (invalid image doesnt throw any exception)

ImageIcon(image, desc) ImageIcon(image) ImageIcon(byte[], desc) ImageIcon(byte[])

Current PR checks for null in ImageIcon(image) so it will not throw NPE but ImageIcon(byte[]) will still throw.. Should we handle those in separate PR and let this only be for setImage(null)?

You told me (off-line) that null URL also throws NPE and I see that too.

This class is a bit of a mess of accidental behaviours and scant documentation.

Let's document all the NPE behaviours and include a test that verifies it.

And invalid data like "new byte[0]" or null for a file name, or pointing to something that isn't an image file, or an invalid URL .. etc .. results in an Image instance being present, but it will never be completed so can never be drawn.

I think we need a class level statement like
"If the image source parameter to a constructor is non-null, but does not reference valid accessible image data, no exceptions will be thrown but the image will be 'effectively' null, as it will have no dimensions and never be drawn, and
getImageLoadStatus() will report ERRORED" - you should verify that last part but I think it is right.

We probably should add the similar text on setImage().

We can also test the invalid image data cases too.

prsadhuk · 2025-07-13T09:11:49Z

I think we need to have the "whole picture" tested to make sure everything does as we expect. It isn't so easy to tell even from looking at the source of ImageIcon. Then spec'ed so developers know what to expect. When I wrote a little test, I see null args do generate NPEs but invalid args don't result in null images Toolkit images are installed, even if they can't be used because the source isn't valid. They are "effectively" null, but not "actually" null. We should at least specify the null arg behaviours and try to say something about invalid image data. And as a result, I am now flipping to just documenting the setImage(null) NPE The protected method loadImage probably needs to say it too. Invalid image data, maybe we can cover that in a class level statement. Note: I'd like to see every constructor tested with both null and invalid image data.

I tested with null image/imagedata and invalid imagedata and it seems only these constructors throw NPE (current JDK) for null image (invalid image doesnt throw any exception)
ImageIcon(image, desc) ImageIcon(image) ImageIcon(byte[], desc) ImageIcon(byte[])
Current PR checks for null in ImageIcon(image) so it will not throw NPE but ImageIcon(byte[]) will still throw.. Should we handle those in separate PR and let this only be for setImage(null)?

You told me (off-line) that null URL also throws NPE and I see that too.

This class is a bit of a mess of accidental behaviours and scant documentation.

Let's document all the NPE behaviours and include a test that verifies it.

And invalid data like "new byte[0]" or null for a file name, or pointing to something that isn't an image file, or an invalid URL .. etc .. results in an Image instance being present, but it will never be completed so can never be drawn.

I think we need a class level statement like "If the image source parameter to a constructor is non-null, but does not reference valid accessible image data, no exceptions will be thrown but the image will be 'effectively' null, as it will have no dimensions and never be drawn, and getImageLoadStatus() will report ERRORED" - you should verify that last part but I think it is right.

We probably should add the similar text on setImage().

We can also test the invalid image data cases too.

Documented all NPE behavour and added class-level statement.
Updated test to verify it.
getImageLoadStatus() does return 4 which is ERRORED for invalid image, updated test reflects that.

prsadhuk added 2 commits June 12, 2025 04:14

ImageIcon.setImage can't handle null parameter

710a710

COpyright

4be55ab

openjdk bot added the rfr Pull request is ready for review label Jun 12, 2025

openjdk bot added the client [email protected] label Jun 12, 2025

javadoc

bcdef9b

kumarabhi006 approved these changes Jun 12, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 12, 2025

azuev-java approved these changes Jun 12, 2025

View reviewed changes

mrserb reviewed Jun 12, 2025

View reviewed changes

src/java.desktop/share/classes/javax/swing/ImageIcon.java Show resolved Hide resolved

openjdk bot added csr Pull request needs approved CSR before integration and removed ready Pull request is ready to be integrated labels Jun 13, 2025

Update image instance before returning

43d43c6

ExE-Boss reviewed Jun 15, 2025

View reviewed changes

src/java.desktop/share/classes/javax/swing/ImageIcon.java Outdated Show resolved Hide resolved

javadoc

d082a35

aivanov-jdk suggested changes Jun 16, 2025

View reviewed changes

test/jdk/javax/swing/ImageIcon/ImageIconNullImageTest.java Outdated Show resolved Hide resolved

Fix for constructor

5d32ca5

aivanov-jdk suggested changes Jun 16, 2025

View reviewed changes

src/java.desktop/share/classes/javax/swing/ImageIcon.java Outdated Show resolved Hide resolved

test/jdk/javax/swing/ImageIcon/ImageIconNullImageTest.java Show resolved Hide resolved

aivanov-jdk reviewed Jun 16, 2025

View reviewed changes

src/java.desktop/share/classes/javax/swing/ImageIcon.java Outdated Show resolved Hide resolved

Fix

fb4a0e2

aivanov-jdk suggested changes Jun 18, 2025

View reviewed changes

aivanov-jdk reviewed Jun 18, 2025

View reviewed changes

test/jdk/javax/swing/ImageIcon/ImageIconNullImageTest.java Outdated Show resolved Hide resolved

prrace reviewed Jun 18, 2025

View reviewed changes

prsadhuk changed the title ~~8159055: ImageIcon setImage and constructor can't handle null parameter~~ 8159055: ImageIcon setImage and ImageIcon(Image) can't handle null parameter Jun 19, 2025

prsadhuk added 2 commits June 19, 2025 03:51

javadoc wording..clear image desscription if image is null

eb8f29f

Test fix

f623774

prsadhuk changed the title ~~8159055: ImageIcon setImage and ImageIcon(Image) can't handle null parameter~~ 8159055: ImageIcon.setImage and ImageIcon(Image) constructor can't handle null parameter Jun 19, 2025

ExE-Boss reviewed Jun 19, 2025

View reviewed changes

aivanov-jdk suggested changes Jun 19, 2025

View reviewed changes

Remove resetting description from constructor as it is already null

e76c4ff

aivanov-jdk suggested changes Jun 20, 2025

View reviewed changes

aivanov-jdk reviewed Jun 20, 2025

View reviewed changes

javadoc change, description setting removed

c263419

prsadhuk added 2 commits July 8, 2025 07:13

Revert ImageIcon constructor behavior

f6cfd3a

constructor test removal

340254e

prsadhuk changed the title ~~8159055: ImageIcon.setImage and ImageIcon(Image) constructor can't handle null parameter~~ 8159055: ImageIcon.setImage can't handle null parameter Jul 8, 2025

aivanov-jdk approved these changes Jul 8, 2025

View reviewed changes

prsadhuk added 2 commits July 13, 2025 10:08

Document NPE behaviour and update test and add class-level statement

a1acc18

Document NPE behaviour and update test and add class-level statement

dbf7edb

	public ImageIcon (Image image) {
	this.image = image;
	Object o = image.getProperty("comment", imageObserver);

	* Setting a {@code null} image will not render any image icon.
	* Setting a {@code null} image renders no icon.

8159055: ImageIcon.setImage can't handle null parameter #25767

Are you sure you want to change the base?

8159055: ImageIcon.setImage can't handle null parameter #25767

Conversation

prsadhuk commented Jun 12, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issues

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Jun 12, 2025

Uh oh!

openjdk bot commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Jun 12, 2025

Uh oh!

mlbridge bot commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

kumarabhi006 commented Jun 12, 2025

Uh oh!

prsadhuk commented Jun 12, 2025

Uh oh!

kumarabhi006 commented Jun 12, 2025

Uh oh!

prsadhuk commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kumarabhi006 commented Jun 12, 2025

Uh oh!

prsadhuk commented Jun 12, 2025

Uh oh!

kumarabhi006 left a comment

Choose a reason for hiding this comment

Uh oh!

azuev-java left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

prsadhuk commented Jun 13, 2025

Uh oh!

Uh oh!

aivanov-jdk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prrace Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prsadhuk commented Jun 12, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Jun 12, 2025 •

edited

Loading

mlbridge bot commented Jun 12, 2025 •

edited

Loading

prsadhuk commented Jun 12, 2025 •

edited

Loading

prrace Jun 19, 2025 •

edited

Loading

aivanov-jdk Jun 20, 2025 •

edited

Loading