<Swing 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
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;
}
-------------
PR: https://git.openjdk.java.net/jdk/pull/1869
More information about the swing-dev
mailing list