21 bugs in 21st version of Apache NetBeans

Apache NetBeans is one of the first IDEs for Java. It has been supported for nearly 30 years. Recently, Apache NetBeans 21 was released. We've decided to check the source code of such a long-lived project and have chosen the most curious errors. Let's follow them up in this article.

What are we checking and how?

One of the ways to check the program source code is to use a static analyzer. This is a special tool that takes source code files as input and outputs the result of the check — warnings about code fragments that may contain errors. Our company develops such a tool called PVS-Studio.

Using the analyzer on large projects with a vast code base is particularly interesting, especially when we can't fully review the code. Apache NetBeans is an open-source IDE for Java that was first released in 1997. The latest version of this IDE, the 21st, was released recently.

So, we've decided to check Apache NetBeans and break down 21 errors that the analyzer found in the source code. We've divided all the errors into small groups based on their specifics.

Copy but check

Let's start with copy-paste errors. When we copy the code, it's easy to forget to change just single character. As a result, it becomes the reason why the program doesn't operate correctly.

private void refresh(Node[] newSelection) {
  ....
  if(entry1 != null && entry2 != null && file1 != null && file2 != null) {
    if(entry1.getDateTime().getTime() > entry1.getDateTime().getTime()) {
      refreshRevisionDiffPanel(entry1, entry2, file1, file2);
    } else {
      refreshRevisionDiffPanel(entry2, entry1, file2, file1);
    }
    return;
  }
  ....
}

Look at the second if condition: it compares two identical objects with each other:

entry1.getDateTime().getTime() > entry1.getDateTime().getTime().

The analyzer issues the following warning for the code fragment:

V6001 There are identical sub-expressions 'entry1.getDateTime().getTime()' to the left and to the right of the '>' operator. HistoryDiffView.java(130)

Here's another error of the same type:

private RevisionInterval getResortedRevisionInterval(SVNRevision revision1, 
                                                     SVNRevision revision2) {
  RevisionInterval ret; 
  if(revision1.equals(SVNRevision.HEAD) && 
     revision1.equals(SVNRevision.HEAD)) {
    ....
  }
  return ret;
}

The method is used with the SVN version control system to evaluate the interval between two revisions: revision1 *and *revision2. The logical condition checks if the revision is one of the most recent (SVNRevision.HEAD). However, developers may mess up and compare only the first revision instead of both revisions.

The PVS-Studio warning:

V6001 There are identical sub-expressions 'revision1.equals(SVNRevision.HEAD)' to the left and to the right of the '&&' operator. RevertModifications.java(387), RevertModifications.java(387)

Let's move on:

public boolean paintDragFeedback(....)
{
  ....
  if (x1 >= x2) {
    x1 = contInsets.left;
    x2 = contSize.width - contInsets.right;
    if (x1 >= x2) return true; // container is too small
  }
  if (y1 >= y2) {
    y1 = contInsets.top;
    x2 = contSize.height - contInsets.bottom;                // <=
    if (y1 >= y2) return true; // container is too small
  }
  ....
}

Take a closer look at the two if blocks: the first handles the x coordinate, the second handles the y coordinate. However, in the second block, devs forgot to change x2 to y2. As a result, the entire block operates incorrectly. The analyzer issues the following warning:

V6080 Consider checking for misprints. It's possible that an assigned variable should be checked in the next condition. BorderLayoutSupport.java(264), BorderLayoutSupport.java(263)

And the next one:

public void preferenceChange(PreferenceChangeEvent evt) {
  String k = evt == null ? null : evt.getKey();  
  ....
  try {
    if (k == null || k.equals(FoldUtilitiesImpl.PREF_OVERRIDE_DEFAULTS)) {
     ....
    }
    if (k == null || k.equals(SimpleValueNames.CODE_FOLDING_ENABLE)) {
     ....
    } 
    if (k == null || FoldUtilitiesImpl.PREF_CONTENT_PREVIEW.equals
                              (FoldUtilitiesImpl.PREF_CONTENT_PREVIEW)) {
      ....
    }
    if (k == null || FoldUtilitiesImpl.PREF_CONTENT_SUMMARY.equals
                              (FoldUtilitiesImpl.PREF_CONTENT_SUMMARY)) {
      ....
    } 
    ....
  } 
  ....
}

Here, in the third and fourth if blocks in the condition, the FoldUtilitiesImpl.PREF_CONTENT_PREVIEW and FoldUtilitiesImpl.PREF_CONTENT_SUMMARY strings are compared to themselves, not to the k string. It means that the written actions in these blocks will always be performed, even when not required. There are analyzer warnings for these two lines:

V6007 Expression 'FoldUtilitiesImpl.PREF_CONTENT_PREVIEW.equals(FoldUtilitiesImpl.PREF_CONTENT_PREVIEW)' is always true. FoldOptionsPanel.java(177)

V6007 Expression 'FoldUtilitiesImpl.PREF_CONTENT_SUMMARY.equals(FoldUtilitiesImpl.PREF_CONTENT_SUMMARY)' is always true. FoldOptionsPanel.java(180)

Let's take a look at another error:

static void createJfxExtension(Project p, FileObject dirFO, 
                               WizardType type) throws IOException {
  FileObject templateFO = FileUtil.getConfigFile("Templates/JFX/jfx-impl.xml"); 
  if (templateFO != null) {
    ....
    if (type == JavaFXProjectWizardIterator.WizardType.SWING) {
      ....
      FileObject swingTemplateFO1 = FileUtil.getConfigFile(....); 
      if (swingTemplateFO1 != null) {
        FileUtil.copyFile(swingTemplateFO1, 
                          templatesFO, "FXSwingTemplate"); 
      }
      FileObjectswingTemplateFO2=FileUtil.getConfigFile(....); 
      if (swingTemplateFO1 != null) {
        FileUtil.copyFile(swingTemplateFO2, 
                          templatesFO, "FXSwingTemplateApplet"); 
      }
      FileObject swingTemplateFO3 = FileUtil.getConfigFile(....); // NOI18N
      if (swingTemplateFO1 != null) {
        FileUtil.copyFile(swingTemplateFO3, 
                          templatesFO,"FXSwingTemplateApplication"); 
      }
    }
    JFXProjectUtils.addExtension(p);
  }
}

In this example, the method describes instructions for opening three different configuration files and then copying them into one. It may seem like everything is OK: before each copy, the file is checked to see whether it's opened or not. But only swingTemplateF01 is checked all the time. If swingTemplateF02 or swingTemplateF03 can't be opened, it may cause an error and the program crash.

Here's the analyzer warning:

V6080 Consider checking for misprints. It's possible that an assigned variable should be checked in the next condition. JFXProjectUtils.java(768), JFXProjectUtils.java(769)

That's not all.

public Object getElementAt(int index) {
  ....
  int myMinIndex = getExternal (minIndex) + 1; 
  // one after the index of the first non-1 index
  int myMaxIndex = getExternal (maxIndex);

  assert myMaxIndex >= myMaxIndex : "Must be greater"; // NOI18N
  ....
}

A copy-paste error may impede not only program operation but also the debugging process at the development stage. In this part of the code, the myMaxIndex >= myMaxIndex condition is written in the assert instead of myMaxIndex >= myMinIndex.

These indexes are used to fill the array range with a specific value. Since the wrong expression is written in assert, an error in the program logic that causes myMaxIndex to be less than myMinIndex won't be detected in advance*.* Correctly written assert conditions reduce the time needed to find and debug errors.

The PVS-Studio warning:

V6001 There are identical sub-expressions 'myMaxIndex' to the left and to the right of the '>=' operator. FilteredListModel.java(319)

Let's go to the next fragment:

private void mergeParallelInclusions(List<IncludeDesc> inclusions, 
                                     IncludeDesc original,
                                     boolean preserveOriginal) {
  IncludeDesc best = null; 
  ....
  // 2nd remove incompatible inclusions, move compatible ones to same level
  for (Iterator it=inclusions.iterator(); it.hasNext(); ) {
    IncludeDesc iDesc = (IncludeDesc) it.next();
    if (iDesc != best) {
      if (!compatibleInclusions(iDesc, best, dimension)) {
        it.remove();
      } else if (iDesc.parent == best.parent && 
                 iDesc.neighbor == best.neighbor && 
                (iDesc.neighbor != null || iDesc.index == iDesc.index)) {  // <=
        it.remove(); // same inclusion twice (detect for better robustness)
      }
      ....
    }
    ....
  }
  ....
  if (unifyGaps != null) {
    // unify resizability of the border gaps collected for individual inclusions
    for (LayoutInterval[] gaps : unifyGaps) {
      int preferredFixedSide = 
                         fixedSideGaps[LEADING] >= fixedSideGaps[TRAILING] ? 
                                                   LEADING : TRAILING;
      for (int i=LEADING; i <= TRAILING; i++) {
        if (LayoutInterval.canResize(gaps[i]) && !anyResizingNeighbor[i]
            && (anyResizingNeighbor[i^1] || preferredFixedSide == i)) {
          operations.setIntervalResizing(gaps[i], false);
          if (!LayoutInterval.canResize(gaps[i^1])) {
            operations.setIntervalResizing(gaps[i^i], true);       // <=
          }
          break;
        }
      }
    }
  }
}

This method has almost 1500 lines of code. This size spells out why some bugs haven't been detected and fixed right away.

First, let's look at the conditions described in the first marked loop on the collection. Inside the body, the received iDesk object is compared with the best object. In one of the comparison conditions, all fields of the object are used. However, a developer was slightly wrong and wrote iDesc.index == iDesc.index instead of iDesc.index == best.index.

Now let's look at the code fragments from the second loop. Pay attention to the index definition of the array element in the if (!LayoutInterval.canResize(gaps[i^1])) line. As an index in the gaps array, the result of the exclusive OR operation of entities with the i index is used. However, the next line contains an error in using this binary operation: operations.setIntervalResizing(gaps[i^i], true). Here, the expression results in zero because the exclusive OR operator is performed on the same element. If zero was meant, it would have been more logical to write it immediately.

The analyzer warning:

V6001 There are identical sub-expressions 'iDesc.index' to the left and to the right of the '==' operator. LayoutFeeder.java(3660)

V6001 There are identical sub-expressions 'i' to the left and to the right of the '^' operator. LayoutFeeder.java(3897)

Too idle to write methods from scratch

All previous examples were about the copy-paste of code lines one by one. In the next fragments, devs copied whole methods!

public final class EditorMimeTypesImpl 
             implements EditorMimeTypesImplementation {

  private final PropertyChangeSupport listeners;

  @Override
  public void addPropertyChangeListener(@NonNull final 
                                    PropertyChangeListener listener) {
    Parameters.notNull("listener", listener);   //NOI18N
    listeners.addPropertyChangeListener(listener);
  }

  @Override
  public void removePropertyChangeListener(@NonNull final 
                                    PropertyChangeListener listener) {
    Parameters.notNull("listener", listener);   //NOI18N
    listeners.addPropertyChangeListener(listener);
  }
}

Take a look at these two methods. They interact with the listeners object, and inside it, a collection is organized. The first method is used to add an item and the second method is used to delete an item. However, inside the removePropertyChangeListener method, its body completely duplicates the addPropertyChangeListener method. Deleting items from this collection is impossible in this case.

The PVS-Studio analyzer issues the following warning:

V6032 It is odd that the body of method 'addPropertyChangeListener' is fully equivalent to the body of another method 'removePropertyChangeListener'. EditorMimeTypesImpl.java(63), EditorMimeTypesImpl.java(69)

This is not the first case:

public class MethodParamsTipPaintComponent extends JToolTip {
  protected int getWidth(String s, Font font) {
    if (font == null) return fontMetrics.stringWidth(s);
      return getFontMetrics(font).stringWidth(s);
  }

  protected int getHeight(String s, Font font) {
    if (font == null) return fontMetrics.stringWidth(s);
      return getFontMetrics(font).stringWidth(s);
  }
}

There are two other methods, and one of them has been completely copied. In the getHeight method, we get the same value as in the getWeight method.

The PVS-Studio warning:

V6032 It is odd that the body of method 'getWidth' is fully equivalent to the body of another method 'getHeight'. MethodParamsTipPaintComponent.java(121), MethodParamsTipPaintComponent.java(126)

Shoot yourself in foot with null reference

Here we'll wrap copy-paste errors up and move on to another type of errors: incorrect reference handling.

Sometimes a dev accesses a reference before checking it for null. Look at this case:

public void setAllData(String key, String[] data) {
  ....
  // remove superfluous data
  if (getEntryCount() > data.length/3) {
    for (int j=0; j < getEntryCount(); j++) {
      PropertiesFileEntry pfe = getNthEntry(j);
      PropertiesStructure ps = pfe.getHandler().getStructure();
      if (pfe == null || ps == null) continue;
      ....
    }
  }
}

Here, the pfe object is obtained, its method is called, and only then we see the check for null.

The analyzer issues the following warning:

V6060 The 'pfe' reference was utilized before it was verified against null. BundleStructure.java(423), BundleStructure.java(424)

Let's continue:

public void run() {
  for (SourceJavadocAttacherImplementation.Definer d : plugins) {
    ....
    List<? extends URL> s = source ?
      d.getSources(root, this) : d.getJavadoc(root, this);
    if (s != null || s.isEmpty()) {
    ....
    }
  }
}

Here, a programmer incorrectly used logical operations, using || instead of &&. The s list will be checked for missing items in it only if there is no reference to the list. Unlike the previous example, the static analyzer identifies the error as the following warning:

V6008 Null dereference of 's'. SourceJavadocAttacherUtil.java(141).

Here's another similar error in the reference logic:

public void propertyChange(PropertyChangeEvent evt) {
  ....
  synchronized (this) {
    artifacts = null;
    if (listeners == null && listeners.isEmpty()) {
      return;
    }
    ....
  }
}

If the reference to listeners is null, the listeners list is checked to ensure that there are no items inside.

The analyzer issues the warning:

V6008 Null dereference of 'listeners'. MavenArtifactsImplementation.java(613)

Let's keep going:

private SourcesModel getModel () {
  SourcesModel tm = model.get ();
  if (tm == null) {
    tm.sourcePath.removePropertyChangeListener (this);
    tm.debugger.getSmartSteppingFilter ().
    removePropertyChangeListener (this);
  }
  return tm;
}

In this code fragment, the null reference error reaches a new level. The null reference is dereferenced when we're sure that the reference is exactly null. Moreover, this function is used in 136 places! Obviously, the developer didn't plan to implement exactly this behavior of the program. If such an error isn't noticed immediately, the tm reference is almost never null. However, we can't deny that some planned functionality isn't still executed.

The PVS-Studio warning:

V6008 Null dereference of 'tm'. SourcesModel.java(713)

The project has another exactly the same error:

void unsetStepSuspendedByBpIn(JPDAThreadImpl thread) {
  JPDABreakpoint oldBp;
  synchronized (stepBreakpointLock) {
  ....
    if (this.suspendedSteppingThreads == null) {
      this.suspendedSteppingThreads.remove(thread);
      ....
    }
  }
  ....
}

In this fragment, the thread reference is deleted from a collection that doesn't exist.

The analyzer issues the warning:

V6008 Null dereference of 'this.suspendedSteppingThreads'. JPDAThreadImpl.java(2329)

Null dereference errors are pretty common in this project. So, let's break down a few more.

public @Override Element getElement() {
  if (view != null) {
    return view.getElement();
  } else {
    return view.getDocument().getDefaultRootElement();
  }
}

Something odd occurs here: the view reference will be dereferenced, no matter if it's null or not.

The analyzer warning:

V6008 Null dereference of 'view'. CollapsedView.java(627)

The next error is more riveting. Here, devs didn't forget to check for null, but unfortunately, they made another mistake:

public ModulePathsProblemsProvider(@NonNull final Lookup baseLkp) {
  this.project = baseLkp.lookup(J2SEProject.class);
  if (this.project == null) {
    throw new IllegalArgumentException(String.format(
       "Unsupported project: %s of type: %s",  //NOI18N
        project,
        project.getClass()
       ));
  }
  this.moduleInfoListeners = new HashSet<>();
  this.listeners = new PropertyChangeSupport(this);
}

The project reference is dereferenced when we try to get the information to throw an exception that the project reference is null. They wanted the best of their code but not today.

The PVS-Studio warning:

V6008 Null dereference of 'project'. ModulePathsProblemsProvider.java(93)

The code fragments that guaranteeing to throw a NullPointerException do not end here.

public static void main(String args[]) throws Exception {
  ....
  Bridge lex = null; //new XMLSyntaxTokenManager(input);
  int i = 25; //token count
  int id;
  int toks = 0;
  long time = System.currentTimeMillis();
  while (i/*--*/>0) {

    lex.next();
    ....
  }
}

A null reference to an object of the Bridge class is created, and then it's dereferenced. The most surprising thing is that the code snippet that initializes this reference has been commented out. As we can see by the flood of comments, the method has damaged a lot after the edits.

The analyzer resents and issues a warning:

V6008 Null dereference of 'lex'. XMLSyntaxTokenManager.java(216)

Error in reverse

Now let's move on to the more unusual bugs for this project.

private void changeIgnoreStatus (File f) throws IOException {
  File parent = f;
  boolean isDirectory = f.isDirectory() && (! Files.isSymbolicLink(f.toPath()));
  StringBuilder sb = new StringBuilder('/');
  if (isDirectory) {
    sb.append('/');
  }
  boolean cont = true;
  while (cont) {
    sb.insert(0, parent.getName()).insert(0, '/');
    parent = parent.getParentFile();
    String path = sb.toString();
    if (parent.equals(getRepository().getWorkTree())) {
      if (addStatement(new File(parent, Constants.DOT_GIT_IGNORE), 
          path, isDirectory, false) &&
          handleAdditionalIgnores(path, isDirectory)) {
        addStatement(new File(parent, Constants.DOT_GIT_IGNORE),
                              path, isDirectory, true);
      }
      cont = false;
    } else {
      cont = addStatement(new File(parent, Constants.DOT_GIT_IGNORE), 
                                   path, isDirectory, false);
   }
}

The '/' character is passed to the StringBuilder constructor. There doesn't seem to be anything wrong here: devs create the StringBuilder instance and immediately add a character to it. However, Java sees this character as a number. As a result, instead of receiving the initial character as a slash, sb *reserves memory space for 47 characters, and 47 corresponds to the '/'* character in the ASCII table.

Now we're coming to the best part. This code generates the absolute path to the f file. If the slash is specified as a string in the sb constructor, it'll be the initial character. Then the path is written to the beginning of this string, thus leaving a slash at the end. Based on this algorithm, we'd get the following paths:

• "/folder1/folder2/f/" for file;
• "/folder1/folder2/f//" for folder.

It isn't hard to see that in both cases there would be an extra slash at the end. However, this doesn't happen, because the slash doesn't influence anything in the StringBuilder constructor. It turns out that the code isn't properly executing, and that's why the string is generated correctly! The PVS-Studio analyzer issues the following warning:

V6009 Buffer capacity is set to '47' using a char value. Most likely, the '/' symbol was supposed to be placed in the buffer. IgnoreUnignoreCommand.java(107)

Danger code. Crash is inevitable

I suggest you look at the "unstoppable" code:

private class BrowserOutlineView extends OutlineView {
  ....
  public void startEditingAtPath(TreePath path) {
    startEditingAtPath(path);
  }
  ....
}

If you want the program to crash all the time, write recursion without the exit condition.

The analyzer warning:

V6062 Possible infinite recursion inside the 'startEditingAtPath' method. BrowserPanel.java(165), BrowserPanel.java(166)

Besides, such recursion occurs more than once in the source code:

public class SchemaReferenceProxy extends SchemaReference 
                                  implements AXIComponentProxy {
  ....
  private SchemaReference getShared() {
    return (SchemaReference)getShared();
  }
  ....
}

If you examine the source code more closely, you'll find that the getShared method is called in all methods of the SchemaReferenceProxy class. The class is used when working with the XML files. So, if you're actively working in Apache NetBeans with the XML files, and you suddenly have a program crash, you know the reason.

The analyzer issues the warning:

V6062 Possible infinite recursion inside the 'getShared' method. SchemaReferenceProxy.java(40), SchemaReferenceProxy.java(41)

Sum it up

We've checked 21 bugs in the Apache NetBeans source code. We stop here, but many suspicious places are still left unexplored. There were some simply weird things happened. For example, there are many places where triple negation has been used, like in !!!it.hasNext(). It's not clear why this would be done in Java :)

You should understand that Apache NetBeans is a project with the huge source code base — it's simply impossible to find and fix all the errors manually. That's why when developing a project at such a high level, it'd be better resort to the help of various tools, such as a static analyzer.

If you're interested in checking other IDEs, we recommend reading the related articles:

• Operation K. Looking for bugs in the IntelliJ IDEA code
• Qt Creator* in search of Qt Creator bugs
• PVS-Studio vs CodeLite: a battle for the perfect code
• Any bugs in your IDE? Checking AvalonStudio with PVS-Studio

If you're interested in the PVS-Studio static analyzer and would like to check your project, here's a special trial version for you. Try it here.