Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8231041: Spotbugs: Null check of value previously dereferenced in java.desktop #1869

Conversation

@turbanoff
Copy link
Contributor

@turbanoff turbanoff commented Dec 22, 2020

I checked JDK16 repository in SpotBugs 4.2.0
изображение
I fixed only places in java.desktop module.
I didn't fixed places, where dereferencing is done inside method.


Progress

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

Integration blocker

 ⚠️ Title mismatch between PR and JBS for issue JDK-8231041

Issue

  • JDK-8231041: Spotbugs: Null check of value previously dereferenced ⚠️ Title mismatch between PR and JBS.

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1869/head:pull/1869
$ git checkout pull/1869

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Dec 22, 2020

👋 Welcome back turbanoff! 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 openjdk bot added the rfr label Dec 22, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 22, 2020

@turbanoff The following labels will be automatically applied to this pull request:

  • 2d
  • swing

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

@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 22, 2020

Webrevs

Copy link
Contributor

@prrace prrace left a comment

Did it occur that maybe the previous de-reference without a null check is the real problem ?
The proposed Raster change actually needs to be addressed as discussed inhttps://bugs.openjdk.java.net/browse/JDK-8255800 and so should not be part of this proposed change. DataBufferUShort may be the same.

But all of them need to be re-examined rather than just blindly updating them as some tool suggests.

@mrserb
Copy link
Member

@mrserb mrserb commented Dec 22, 2020

I had a similar fix in past, but the problem here is that most of these exceptions are not specified, or specified differently than actually works. So I postponed the fix since the spec clarification is required. I suggest postponing this PR until then.

@turbanoff
Copy link
Contributor Author

@turbanoff turbanoff commented Dec 23, 2020

But all of them need to be re-examined rather than just blindly updating them as some tool suggests.

I agree. I actually did careful investigation of all my changes. As you can see SpotBugs reported much more than I actually propose to fix. I reverted most controversal changes from this PR. I hoped that reviewers could help me to clarify controversal changes and start discussion about unclear places.
I will go through my changes and my thoughts about it:

1. DataBufferUShort explicit NPE in constructors

изображение

Behaviour of DataBufferUShort constructors in case if dataArray == null is the same as in original code, as in the changed code, as in hypothetical code where we removed dereference before null check. This behavior - is NullPointerException
This code is in original form, as it was uploaded into open source under https://bugs.openjdk.java.net/browse/JDK-6662775 So there is no chance that there are consumers of this code which expect NPE with this specific message dataArray is null.

2. Dereference of parameter JComponent c in method getPreferredSize in classes BasicToolTipUI, SynthToolTipUI

изображение

This dereference was there from original upload of OpenJDK.
While NPE is not specified in javadoc of method javax.swing.plaf.ComponentUI#getPreferredSize, most of implementations do not check nullness of passed parameter. As I know it matches with general Swing behaviour about null tolerance.

3. Dereference of field stack in javax.swing.text.html.parser.Parser#ignoreElement

изображение

This dereference was there from original upload of OpenJDK.
Method ignoreElement is called only from method legalElementContext which has null check at the start:

        // Deal with the empty stack
        if (stack == null) {
            // System.out.println("-- stack is empty");
            if (elem != dtd.html) {
                // System.out.println("-- pushing html");
                startTag(makeTag(dtd.html, true));
                return legalElementContext(elem);
            }
            return true;
        }

4. Null checks of paragraph javax.swing.text.Element after for cycle in 3 methods: javax.swing.text.html.AccessibleHTML.TextElementInfo.TextAccessibleContext#getParagraphElement, javax.swing.text.DefaultStyledDocument#getParagraphElement, javax.swing.text.JTextComponent.AccessibleJTextComponent#getParagraphElement

изображение

This methods have the same cycle before null check

                Element para;
                for (para = model.getDefaultRootElement(); ! para.isLeaf(); ) {
                    int pos = para.getElementIndex(index);
                    para = para.getElement(pos);
                }

SpotBugs reported redundant null check of para, because dereference is alredy happened in for cycle condition: ! para.isLeaf()
This dereference and null check were there from original upload of OpenJDK.
para is a result of Element.getElement method call. Since parameter pos is result of previous call to Element.getElementIndex - and this method, according to its javadoc, must return valid child element index for non-leafs.
It means Element.getElement shouldn't return null values according to contracts. So even if some custom implementation returns null - it violates Element contract and NPE is expected.
OpenJDK implementations of Element

5. Null check of in method javax.swing.text.html.StyleSheet#getRule(HTML.Tag, Element)

изображение

Variable AttributeSet attr is result of method javax.swing.text.Element#getAttributes. It is dereferenced and then null checked.
This dereference was there from original upload of OpenJDK.
Javadoc of method javax.swing.text.Element#getAttributes doesn't specify that it can return null.
OpenJDK implementations never return null. Actually there is only one implementation javax.swing.text.AbstractDocument.AbstractElement#getAttributes:

        public AttributeSet getAttributes() {
            return this;
        }

6. Component c = javax.swing.MenuElement.getComponent() is null-checked in javax.swing.JMenuBar#processBindingForKeyStrokeRecursive

изображение

This dereference was there from original upload of OpenJDK.
Variable c is checked before another condition c instanceof JComponent in the same if statement. instanceof JComponent do not return true for null values. So it's safe to remove.
Javadoc of method javax.swing.MenuElement.getComponent() doesn't specify null as possible return value. All implementations inside OpenJDK return non-null value: this

7. Null check of File fontFile inside sun.font.FileFont.CreatedFontFileDisposerRecord#dispose

изображение

This field of sun.font.FileFont.CreatedFontFileDisposerRecord class initialized in constructor, which is called in one place sun.font.FileFont#setFileToRemove. File file is passed there from the caller method sun.font.SunFontManager#createFont2D.
At the start method sun.font.SunFontManager#createFont2D there is dererefence:

        String fontFilePath = fontFile.getPath();

This internal method sun.font.FontManager#createFont2D never accepted null values since its introduction.
PS. I tracked where original fontFile value could came from and found 2 public methods:
a. java.awt.Font#createFont(int, java.io.File) - it does specify NullPointerException in it's javadoc
b. java.awt.Font#createFonts(java.io.File) - it does NOT specify NullPointerException in it's javadoc. Perphaps it should be specified to be consistent.
Anyway this fact is not directly related to removing redundant null check in sun.font.FileFont.CreatedFontFileDisposerRecord#dispose. Variable is dereferenced multiple times in code flow, before calling this method.

8. Null check of bufferedImage in sun.print.PathGraphics#hasTransparentPixels

изображение

This dereference was there from original upload of OpenJDK.
Variable is dereferenced at the start of method. This method is called from 2 places: sun.print.PSPathGraphics#drawImageToPlatform and in sun.awt.windows.WPathGraphics#drawImageToPlatform. Both of this methods handle null values by themselves

        BufferedImage img = getBufferedImage(image);
        if (img == null) {
            return true;
        }
@turbanoff
Copy link
Contributor Author

@turbanoff turbanoff commented Dec 23, 2020

So I postponed the fix since the spec clarification is required. I suggest postponing this PR until then.

Do you know if this clarification is in progress now? As I see from @prrace comment there is https://bugs.openjdk.java.net/browse/JDK-8255800 Do you know any more similar issues?

@mrserb
Copy link
Member

@mrserb mrserb commented Dec 31, 2020

So I postponed the fix since the spec clarification is required. I suggest postponing this PR until then.

Do you know if this clarification is in progress now? As I see from @prrace comment there is https://bugs.openjdk.java.net/browse/JDK-8255800 Do you know any more similar issues?

I do not know any other issues except this one. If you wish to continue to work on this bug, then it will be simpler to only change the code where exceptions are already specified, for example, drop the changes in the DataBufferUShort(), etc.

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jan 28, 2021

@turbanoff This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Feb 25, 2021

@turbanoff This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it!

@bridgekeeper bridgekeeper bot closed this Feb 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants