8159055: Clarify handling of null and invalid image data for ImageIcon constructors and setImage method #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: Clarify handling of null and invalid image data for ImageIcon constructors and setImage method (Bug - P4)
JDK-8359398: Clarify handling of null and invalid image data for ImageIcon constructors and setImage method (CSR)

Reviewers

Abhishek Kumar (@kumarabhi006 - Reviewer) Review applies to 8e1938b6
Alexander Zuev (@azuev-java - Reviewer) Review applies to c6be2a91
Alexey Ivanov (@aivanov-jdk - Reviewer)
Sergey Bylokhov (@mrserb - Reviewer) Review applies to 6f6320ac
Phil Race (@prrace - Reviewer)

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 now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8159055: Clarify handling of null and invalid image data for ImageIcon constructors and setImage method

Reviewed-by: aivanov, prr, abhiscxk, kizune, serb

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 1099 new commits pushed to the master branch:

e7d2a52: 8344333: Spurious System.err.flush() in LWCToolkit.java
0f7c0e9: 8302057: Wrong BeanProperty description for JTable.setShowGrid
e507766: 8361610: Avoid wasted work in ImageIcon(Image) for setting description
... and 1096 more: https://git.openjdk.org/jdk/compare/20005511e3612d6a5f12fa83066f02c88c628e8b...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

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

prrace · 2025-07-29T16:56:16Z

Currently, JDK-8159055 is an enhancement which made sense while the goal of this PR was to change Image.setImage so that it accepts null as parameter.

After a month of discussions, that solution has been abandoned, instead it is stated that Image.setImage will throw NullPointerException if image parameter is null.

I think the type of JDK-8159055 should be changed to bug.

I agree. Not saying how null is handled is a bug. So fixing it is a bug fix, not an enhancement.

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

aivanov-jdk · 2025-08-05T11:24:43Z

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

+                               new ImageIcon((URL)null);
+                           } else if (v == ArgVal.INVALID_DATA) {
+                               expected = false;
+                               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).

aivanov-jdk · 2025-08-05T11:26:59Z

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

+                               new ImageIcon(bytes);
+                           } else if (v == ArgVal.INVALID_DATA) {
+                               expected = false;
+                               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?

aivanov-jdk · 2025-08-05T11:28:02Z

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

+                           break;
+                       case IMAGE :
+                           if (v == ArgVal.NULL) {
+                               new ImageIcon((Image)null);


Suggested change

new ImageIcon((Image)null);

new ImageIcon((Image) null);

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

Personally, I do not like having a space there.

aivanov-jdk · 2025-08-05T11:31:00Z

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

+                if (expected && !passed) {
+                   System.err.println("Did not receive expected exception for : " + a);
+                   throw new RuntimeException("Test failed");
+                }


Suggested change

if (expected && !passed) {

System.err.println("Did not receive expected exception for : " + a);

throw new RuntimeException("Test failed");

}

if (expected && !passed) {

throw new RuntimeException("Did not receive expected exception for: " + a);

}

Why not throw a detailed 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.

aivanov-jdk · 2025-08-05T11:31:32Z

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

+                if (!expected && !passed) {
+                   System.err.println("Received unexpected exception for : " + a);
+                   throw new RuntimeException("Test failed");
+                }


Suggested change

if (!expected && !passed) {

System.err.println("Received unexpected exception for : " + a);

throw new RuntimeException("Test failed");

}

if (!expected && !passed) {

throw new RuntimeException("Received unexpected exception for: " + a);

}

prrace · 2025-08-05T18:44:18Z

This PR seems to be back waiting on the CSR ?

aivanov-jdk · 2025-08-05T19:54:46Z

This PR seems to be back waiting on the CSR ?

Yes, because the text was updated.

Additionally, the test needs updating because it doesn't test ImageIcon(URL) constructor, it tests ImageIcon(String) twice.

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

Lines 69 to 76 in c6be2a9

    
           case URL : 
        
               if (v == ArgVal.NULL) { 
        
                   new ImageIcon((URL)null); 
        
               } else if (v == ArgVal.INVALID_DATA) { 
        
                   expected = false; 
        
                   new ImageIcon("file://" + imgName, "gif"); 
        
                   passed = true; // no exception expected for this case 
        
               }

prsadhuk · 2025-08-06T02:51:57Z

Test updated

aivanov-jdk

Looks good… except for lots of unexpected blank lines in the test code.

aivanov-jdk · 2025-08-06T15:00:09Z

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

+                    switch (a) {
+
+                       case FILE :
+
+                           expected = false;
+                           String s = (v == ArgVal.NULL) ? null : imgName;
+                           new ImageIcon(s);
+                           passed = true; // no exception expected for this case
+                           break;
+
+                       case URL :


Was adding so many blank lines intentional?

aivanov-jdk · 2025-08-18T19:10:37Z

Don't you want to use JUnit for test/jdk/javax/swing/ImageIcon/ImageIconTest.java?

I think using separate small methods to test ImageIcon constructors is much clearer and readable than switching over a set of enums. Moreover, a JUnit test will run all the test methods, which means you'll get the full picture instead of failing the entire test when the a first assertion fails.

I posted ImageIconConstructorsTest.java that uses JUnit. To run this test with jtreg add the following tags:

/*
 * @test
 * @summary Verifies behavior of ImageIcon constructors with null parameters
 * @run junit ImageIconConstructorsTest
 */
public final class ImageIconConstructorsTest {
}

You'll have to add test methods for invalid files and invalid data, though.

The jtreg report looks like this:

----------System.out:(0/0)----------
----------System.err:(31/2237)----------
STARTED    ImageIconConstructorsTest::stringNull 'stringNull()'
SUCCESSFUL ImageIconConstructorsTest::stringNull 'stringNull()' [15ms]
STARTED    ImageIconConstructorsTest::noArgs 'noArgs()'
SUCCESSFUL ImageIconConstructorsTest::noArgs 'noArgs()' [0ms]
STARTED    ImageIconConstructorsTest::imageNull 'imageNull()'
SUCCESSFUL ImageIconConstructorsTest::imageNull 'imageNull()' [0ms]
...

prrace · 2025-08-21T20:38:52Z

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

+                           break;
+                       case IMAGE :
+                           if (v == ArgVal.NULL) {
+                               new ImageIcon((Image)null);


Personally, I do not like having a space there.

prsadhuk · 2025-08-26T05:21:25Z

/touch

openjdk · 2025-08-26T05:22:14Z

@prsadhuk The pull request is being re-evaluated and the inactivity timeout has been reset.

prsadhuk · 2025-08-26T05:26:44Z

/integrate

openjdk · 2025-08-26T05:29:16Z

Going to push as commit 98e64cf.
Since your change was applied there have been 1099 commits pushed to the master branch:

e7d2a52: 8344333: Spurious System.err.flush() in LWCToolkit.java
0f7c0e9: 8302057: Wrong BeanProperty description for JTable.setShowGrid
e507766: 8361610: Avoid wasted work in ImageIcon(Image) for setting description
... and 1096 more: https://git.openjdk.org/jdk/compare/20005511e3612d6a5f12fa83066f02c88c628e8b...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-08-26T05:29:22Z

@prsadhuk Pushed as commit 98e64cf.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

aivanov-jdk · 2025-08-26T10:56:12Z

Personally, I do not like having a space there.

Neither do I.

There should be no space between a case label and the colon.

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 client-libs-dev@openjdk.org 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 Outdated 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 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 Show resolved Hide resolved

test/jdk/javax/swing/ImageIcon/ImageIconNullImageTest.java Outdated 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

prsadhuk changed the title ~~8159055: ImageIcon.setImage can't handle null parameter~~ 8159055: Clarify handling of null and invalid image data for ImageIcon constructors and setImage method Jul 29, 2025

prrace reviewed Jul 29, 2025

View reviewed changes

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

class javadoc modification

c6be2a9

prrace approved these changes Aug 4, 2025

View reviewed changes

azuev-java approved these changes Aug 5, 2025

View reviewed changes

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

aivanov-jdk suggested changes Aug 5, 2025

View reviewed changes

Test update

5b38886

aivanov-jdk approved these changes Aug 6, 2025

View reviewed changes

openjdk bot removed the csr Pull request needs approved CSR before integration label Aug 7, 2025

prrace approved these changes Aug 21, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Aug 21, 2025

openjdk bot added the integrated Pull request has been integrated label Aug 26, 2025

openjdk bot closed this Aug 26, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Aug 26, 2025

prsadhuk deleted the JDK-8159055 branch August 26, 2025 05:29

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

8159055: Clarify handling of null and invalid image data for ImageIcon constructors and setImage method #25767

8159055: Clarify handling of null and invalid image data for ImageIcon constructors and setImage method #25767

Uh oh!

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!

prrace commented Jul 29, 2025

Uh oh!

Uh oh!

Uh oh!

aivanov-jdk Aug 5, 2025

Choose a reason for hiding this comment

Uh oh!

aivanov-jdk Aug 5, 2025

Choose a reason for hiding this comment

Uh oh!

aivanov-jdk Aug 5, 2025

Choose a reason for hiding this comment

Uh oh!

prrace Aug 21, 2025

Choose a reason for hiding this comment

Uh oh!

aivanov-jdk Aug 5, 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!

aivanov-jdk Aug 5, 2025

Choose a reason for hiding this comment

Uh oh!

prrace commented Aug 5, 2025

Uh oh!

aivanov-jdk commented Aug 5, 2025

Uh oh!

prsadhuk commented Aug 6, 2025

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

aivanov-jdk Aug 5, 2025 •

edited

Loading

aivanov-jdk commented Aug 26, 2025 •

edited

Loading