5015261: NPE may be thrown if JDesktopIcon is set to null on a JInternalFrame

[prsadhuk]

JInternalFrame.getDesktopIcon() can be null as setDesktopIcon() can be called with null value. There are many places in JInternalFrame where getDesktopIcon() is accessed without null check which might cause NPE.
Added null check for those cases.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JDK-5015261: NPE may be thrown if JDesktopIcon is set to null on a JInternalFrame

Reviewers

• Sergey Bylokhov (@mrserb - Reviewer) ⚠️ Review applies to 1351b93
• Phil Race (@prrace - Reviewer) ⚠️ Review applies to 213b235
• Alexey Ivanov (@aivanov-jdk - Reviewer) ⚠️ Review applies to 213b235

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4989/head:pull/4989
$ git checkout pull/4989

Update a local copy of the PR:
$ git checkout pull/4989
$ git pull https://git.openjdk.java.net/jdk pull/4989/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4989

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4989.diff

[bridgekeeper]

👋 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]

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

• swing

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]

Webrevs

• 06: Full - Incremental (7883ce2a)
• 05: Full - Incremental (213b2351)
• 04: Full - Incremental (1351b93c)
• 03: Full - Incremental (3e1a444d)
• 02: Full - Incremental (f69c24a1)
• 01: Full - Incremental (0c96a6e3)
• 00: Full (2a2ab50d)

[mrserb]

Does the null desktop icon is actually supported? I have set it to null in the constructor of JInternalFrame, and check how the SwingSet2 work when jdesktop frame is minimized, under all L&F I got NPE. But even if we fix that npe how the user will be able to "unmimimize" the window? Probably we should state that the null icon is unsupported?

[prsadhuk]

Will just stating null icon is unsupported do? I think in that case should we throw IAE?

[mrserb]

Will just stating null icon is unsupported do? I think in that case should we throw IAE?

This icon field is protected so technically it can be changed w/o constructor and set method. So we have a choice:

1. Specify that the null icon is unsupported and may throw NPE any time after
2. Update the set method to throw NPE(since this is an exception already used by various parts) if a null value is passed.

We can do only the first one, or both.

[prsadhuk]

Any more comment on the modification?

[prrace]

JInternalFrame.JDesktopIcon seems to be a bit of a mess overall.

The class doc :
https://docs.oracle.com/en/java/javase/11/docs/api/java.desktop/javax/swing/JInternalFrame.JDesktopIcon.html
says :

This component represents an iconified version of a JInternalFrame. This API should NOT BE USED by Swing applications,
as it will go away in future versions of Swing as its functionality is moved into JInternalFrame. ...
If an application wants to display a desktop icon, it should create a JInternalFrame instance and iconify it.

I am not sure what that implies -iconification of a JInternalFrame isn't the same thing as setting it.

Are the setter and getter there only for the benefit of a L&F and really should not be public at all ?

The words "may" throw an NPE are not clear about when - I guess we aren't throwing an NPE right
at the moment you set it but aybe if you iconify the JInternalFrame it'll happen later ?
That's not great.

We should either prevent null being set or catch all those nulls.

And what if it is null some action needs to be taken - as Shannon wrote - "whatever that is"

On the basis that we need an icon else more problems occur I think we should just prevent it
and take the incompatibility of throwing NPE immediately you try to set null.
Folks who are doing it are likely seeing random exceptions anyway at a later time !

[mrserb]

Folks who are doing it are likely seeing random exceptions anyway at a later time !

What we will do if the field will be set to null directly? Throw NPE at any point later when we will found null?

[prrace]

Folks who are doing it are likely seeing random exceptions anyway at a later time !

What we will do if the field will be set to null directly? Throw NPE at any point later when we will found null?

The field is protected. If a subclasser wants to destroy the functionality of a class they are subclassing there are lots of ways they can do that. Just add advisory doc on the protected field too.

[mrserb]

Just add advisory doc on the protected field too.

ok, this is part of what this change do, probably the text should be rephrased there?

[prsadhuk]

We should either prevent null being set or catch all those nulls.

I did that in 1st iteration of this PR..https://github.com/openjdk/jdk/pull/4989/files/2a2ab50ded3305f3a9e0f01e90417b484a0b2db4

@prrace Should I bring that fix back?

Just add advisory doc on the protected field too.

ok, this is part of what this change do, probably the text should be rephrased there?
What should be the advisory phrase? Will the text I have added in the protected field doc not suffice?

[prrace]

We should either prevent null being set or catch all those nulls.

I did that in 1st iteration of this PR..https://github.com/openjdk/jdk/pull/4989/files/2a2ab50ded3305f3a9e0f01e90417b484a0b2db4

@prrace Should I bring that fix back?

You are reading my "there are two options text" and the code you propose to bring back is NOT the option
I recommended.

Just add advisory doc on the protected field too.

ok, this is part of what this change do, probably the text should be rephrased there?
What should be the advisory phrase? Will the text I have added in the protected field doc not suffice?

Something like "subclassers must ensure this is set to a non-null value during construction and not set this to null"

[prsadhuk]

@prrace Any comment on this? I am just preventing "null being set" as was recommended.

[mrserb]

/csr

[openjdk]

@mrserb has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@prsadhuk please create a CSR request for issue JDK-5015261. This pull request cannot be integrated until the CSR request is approved.

[prsadhuk]

Please review CSR https://bugs.openjdk.java.net/browse/JDK-8272748

[prrace]

Could you change the bug and CSR summary, and of course the title of this PR ?
I think JinternalFrame does not cause the NPE. It is caused by setting a null desktop icon
Something like
"NPE may be thrown if JDesktopIcon is set to null on a JInternalFrame"

[prrace]

nor->not (looks like a typo I made in the CSR) as r and t keys are adjacent. I fixed the CSR but you'll have to to fix it here, otherwise seems fine

[openjdk]

@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:

5015261: NPE may be thrown if JDesktopIcon is set to null on a JInternalFrame

Reviewed-by: serb, prr, aivanov

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 302 new commits pushed to the master branch:

• 9bc0232: 8269223: -Xcheck:jni WARNINGs working with fonts on Linux
• 2ff4c01: 8271600: C2: CheckCastPP which should closely follow Allocate is sunk of a loop
• ad92033: 8272736: [JVMCI] Add API for reading and writing JVMCI thread locals
• 709b591: 8272553: several hotspot runtime/CommandLine tests don't check exit code
• 1884072: 8265253: javac -Xdoclint:all gives "no comment" warning for code that can't be commented
• 594e516: 8272778: Consolidate is_instance and is_instance_inlined in java_lang_String
• d542745: 8267894: Skip work for empty regions in G1 Full GC
• 741f58c: 8272417: ZGC: fastdebug build crashes when printing ClassLoaderData
• b7f75c0: 8271142: package help is not displayed for missing X11/extensions/Xrandr.h
• e8a289e: 8272609: Add string deduplication support to SerialGC
• ... and 292 more: https://git.openjdk.java.net/jdk/compare/e4295ccfcdb16041d6f18fd64f7df3f740bf258f...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.

[prsadhuk]

/integrate

[openjdk]

Going to push as commit 22ef4f0.
Since your change was applied there have been 302 commits pushed to the master branch:

• 9bc0232: 8269223: -Xcheck:jni WARNINGs working with fonts on Linux
• 2ff4c01: 8271600: C2: CheckCastPP which should closely follow Allocate is sunk of a loop
• ad92033: 8272736: [JVMCI] Add API for reading and writing JVMCI thread locals
• 709b591: 8272553: several hotspot runtime/CommandLine tests don't check exit code
• 1884072: 8265253: javac -Xdoclint:all gives "no comment" warning for code that can't be commented
• 594e516: 8272778: Consolidate is_instance and is_instance_inlined in java_lang_String
• d542745: 8267894: Skip work for empty regions in G1 Full GC
• 741f58c: 8272417: ZGC: fastdebug build crashes when printing ClassLoaderData
• b7f75c0: 8271142: package help is not displayed for missing X11/extensions/Xrandr.h
• e8a289e: 8272609: Add string deduplication support to SerialGC
• ... and 292 more: https://git.openjdk.java.net/jdk/compare/e4295ccfcdb16041d6f18fd64f7df3f740bf258f...master

Your commit was automatically rebased without conflicts.

[openjdk]

@prsadhuk Pushed as commit 22ef4f0.

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