[OpenJDK 2D-Dev] RFR: 8231041: Spotbugs: Null check of value previously dereferenced in java.desktop [v2]

Andrey Turbanov github.com+741251+turbanoff at openjdk.java.net
Wed Dec 23 11:21:09 UTC 2020


On Tue, 22 Dec 2020 20:17:59 GMT, Phil Race <prr at openjdk.org> wrote:

>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
 
![изображение](https://user-images.githubusercontent.com/741251/102990509-8823fb80-4528-11eb-908f-42b90d3ae844.png)

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`
![изображение](https://user-images.githubusercontent.com/741251/102990550-9d008f00-4528-11eb-85c8-2eb5034e1549.png)

 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`
![изображение](https://user-images.githubusercontent.com/741251/102990568-a5f16080-4528-11eb-84cf-faa47160f1a2.png)

 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`
![изображение](https://user-images.githubusercontent.com/741251/102990588-aee23200-4528-11eb-93e3-ada52eafc1a5.png)

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)`
![изображение](https://user-images.githubusercontent.com/741251/102990622-bdc8e480-4528-11eb-9f2d-a89940666e75.png)

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`
![изображение](https://user-images.githubusercontent.com/741251/102991095-9888a600-4529-11eb-9f84-3112a76194ee.png)

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`
![изображение](https://user-images.githubusercontent.com/741251/102990639-c7524c80-4528-11eb-8a10-a59c81bff9bb.png)

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`
![изображение](https://user-images.githubusercontent.com/741251/102991049-7d1d9b00-4529-11eb-8af1-30cc0d2f996e.png)

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;
        }

-------------

PR: https://git.openjdk.java.net/jdk/pull/1869


More information about the 2d-dev mailing list