GT-2698 - code-review fixes, javadocs

Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/function/FunctionPlugin.java

@@ -388,23 +388,7 @@ public class FunctionPlugin extends Plugin implements DataService {
 			Address loc = location.getAddress();
 			return program.getFunctionManager().getFunctionsOverlapping(new AddressSet(loc, loc));
 		}
-		//return an empty iterator....
+		return Collections.emptyIterator();
-		return new Iterator<>() {
-			@Override
-			public void remove() {
-				// not supported
-			}
-
-			@Override
-			public boolean hasNext() {
-				return false;
-			}
-
-			@Override
-			public Function next() {
-				return null;
-			}
-		};
 	}
 
 	Function getFunction(ListingActionContext context) {

Ghidra/Features/Base/src/main/java/ghidra/app/util/OptionsEditorPanel.java

@@ -247,6 +247,8 @@ public class OptionsEditorPanel extends JPanel {
 			combo.setSelectedItem(space);
 		}
 		combo.addActionListener(e -> {
+			// called whenever the combobox changes to push the value back to the Option that is
+			// our 'model'
 			option.setValue(combo.getSelectedItem());
 		});
 		return combo;

Ghidra/Features/Base/src/main/java/ghidra/app/util/dialog/AskAddrDialog.java

@@ -26,7 +26,6 @@ import docking.widgets.label.GLabel;
 import ghidra.app.util.AddressInput;
 import ghidra.program.model.address.Address;
 import ghidra.program.model.address.AddressFactory;
-import ghidra.util.SystemUtilities;
 
 public class AskAddrDialog extends DialogComponentProvider {
 	private boolean isCanceled;
@@ -54,8 +53,7 @@ public class AskAddrDialog extends DialogComponentProvider {
 		addCancelButton();
 		setDefaultButton(okButton);
 
-		SystemUtilities.runSwingNow(
+		DockingWindowManager.showDialog(null, this);
-			() -> DockingWindowManager.showDialog(null, AskAddrDialog.this));
 	}
 
 	@Override

Ghidra/Features/Base/src/test/java/ghidra/app/util/html/DataTypeDifferTest.java

@@ -26,7 +26,7 @@ import javax.swing.*;
 import org.junit.Assert;
 import org.junit.Test;
 
-import docking.widgets.label.GDLabel;
+import docking.widgets.label.GDHtmlLabel;
 import ghidra.app.util.html.diff.*;
 
 public class DataTypeDifferTest {
@@ -491,7 +491,7 @@ public class DataTypeDifferTest {
 
 		JPanel rightPanel = new JPanel(new BorderLayout());
 		StringBuffer buffy1 = new StringBuffer(htmlLeft);
-		JLabel rightLabel = new GDLabel();
+		JLabel rightLabel = new GDHtmlLabel();
 		rightLabel.setOpaque(true);
 		rightLabel.setBackground(Color.WHITE);
 		rightLabel.setVerticalAlignment(SwingConstants.TOP);
@@ -499,7 +499,7 @@ public class DataTypeDifferTest {
 
 		JPanel leftPanel = new JPanel(new BorderLayout());
 		StringBuffer buffy2 = new StringBuffer(htmlRight);
-		JLabel leftLabel = new GDLabel();
+		JLabel leftLabel = new GDHtmlLabel();
 		leftLabel.setOpaque(true);
 		leftLabel.setBackground(Color.WHITE);
 		leftLabel.setVerticalAlignment(SwingConstants.TOP);

Ghidra/Features/ByteViewer/src/main/java/ghidra/app/plugin/core/byteviewer/ByteViewerPlugin.java

@@ -67,8 +67,7 @@ public class ByteViewerPlugin extends Plugin {
 
 	private ProgramByteViewerComponentProvider connectedProvider;
 
-	private List<ProgramByteViewerComponentProvider> disconnectedProviders =
+	private List<ProgramByteViewerComponentProvider> disconnectedProviders = new ArrayList<>();
-		new ArrayList<>();
 
 	public ByteViewerPlugin(PluginTool tool) {
 		super(tool);
@@ -336,7 +335,7 @@ public class ByteViewerPlugin extends Plugin {
 	/**
 	 * Set the status info on the tool.
 	 * 
-	 * @param msg non-html text to display
+	 * @param msg text to display
 	 * @param provider not used
 	 */
 	void setStatusMessage(String msg, ComponentProvider provider) {

Ghidra/Framework/Docking/src/main/java/docking/DockingUtils.java

@@ -20,13 +20,76 @@ import java.awt.event.*;
 
 import javax.swing.*;
 import javax.swing.plaf.basic.BasicSeparatorUI;
+import javax.swing.table.TableCellRenderer;
 import javax.swing.text.Document;
 import javax.swing.text.JTextComponent;
+import javax.swing.tree.DefaultTreeCellRenderer;
+import javax.swing.tree.TreeCellRenderer;
 import javax.swing.undo.UndoableEdit;
 
+import docking.widgets.checkbox.GCheckBox;
+import docking.widgets.checkbox.GHtmlCheckBox;
+import docking.widgets.combobox.GComboBox;
+import docking.widgets.combobox.GhidraComboBox;
+import docking.widgets.label.*;
+import docking.widgets.list.GList;
+import docking.widgets.list.GListCellRenderer;
+import docking.widgets.table.GTableCellRenderer;
+import docking.widgets.tree.support.GTreeRenderer;
 import ghidra.docking.util.DockingWindowsLookAndFeelUtils;
+import ghidra.util.HTMLUtilities;
 import resources.ResourceManager;
 
+/**
+ * <h1>Notes about how to use HTML safely:</h1>
+ * Java's built-in HTML rendering in UI components is very useful, but can also introduce security
+ * issues when a hostile actor is providing the text strings that are being rendered.
+ * <p>
+ * Before using a native Java UI component, search for a corresponding 'G'hidra component, and
+ * if possible choose the non-HTML version of that component (if available).
+ * <p>
+ * For instance, instead of using {@link JLabel}, use either {@link GLabel} or {@link GHtmlLabel}
+ * (and their variants).
+ * <p>
+ * (native JLabel, JCheckbox, etc, usage is actually disallowed in the Ghidra project)
+ * <p>
+ * When using a UI component that is HTML enabled, care must be used when constructing the text
+ * that is being rendered.
+ * <p>
+ * During string-building or concatenation, appending a non-literal string value (ie. 
+ * {@code "Hello " + getFoo();} ), the non-literal string value should be escaped using 
+ * {@link HTMLUtilities#escapeHTML(String)} (ie. {@code "Hello " + HTMLUtilities.escapeHTML(getFoo());}.
+ * <p>
+ * Of course, there are exceptions to every rule, and if the string value can be definitely be 
+ * traced to its source and there are no user-supplied origins, the HTML escaping can be skipped.
+ * <p>
+ * Note: just using a UI component that is HTML enabled does not mean that it will treat its
+ * text as HTML text.  If you need to HTML escape any values that are being fed to the component, you
+ * need to force the HTML mode 'on' by pre-pending a "&lt;HTML&gt;" at the beginning of the string.
+ * If you fail to do this, the escaped substrings will look wrong because any '&lt;' and '&gt;' chars
+ * (and others) in the substring will be mangled when rendered in plain-text mode.
+ * <p>
+ * When working with plain text, try to avoid allowing a user supplied string being the first 
+ * value of text that could be fed to a UI component.  This will prevent the possibly hostile 
+ * string from having a leading HTML start tag.  
+ * (ie. when displaying an error to the user about a bad file, don't put the filename
+ * value at the start of the string, but instead put a quote or some other delimiter to prevent
+ * html mode).
+ * <p>
+ * <h1>Recommended Ghidra UI Components:</h1>
+ * <p>
+ * <table border=1>
+ * 	<tr><th>Native Component</th><th>Recommended Component</th></tr>
+ * 	<tr><td>{@link JLabel}</td><td>{@link GLabel}<br>{@link GDLabel}<br>{@link GHtmlLabel}<br>{@link GDHtmlLabel}<br>{@link GIconLabel}</td></tr>
+ * 	<tr><td>{@link JCheckBox}</td><td>{@link GCheckBox}<br>{@link GHtmlCheckBox}</td></tr>
+ * 	<tr><td>{@link JComboBox}</td><td>{@link GComboBox}<br>{@link GhidraComboBox}</td></tr>
+ * 	<tr><td>{@link JList}</td><td>{@link GList}</td></tr>
+ * 	<tr><td>{@link ListCellRenderer}<br>{@link DefaultListCellRenderer}</td><td>{@link GListCellRenderer}</td></tr>
+ * 	<tr><td>{@link TableCellRenderer}</td><td>{@link GTableCellRenderer}</td></tr>
+ * 	<tr><td>{@link TreeCellRenderer}<br>{@link DefaultTreeCellRenderer}</td><td>{@link GTreeRenderer}<br>{@link DnDTreeCellRenderer}</td></tr>
+ * 	<tr><td>{@link JButton}</td><td>???tbd???</td></tr>
+ * </table>
+ */
 public class DockingUtils {
 	// taken from BasicHTML.htmlDisable, which is private
 	public static final String HTML_DISABLE_STRING = "html.disable";

Ghidra/Framework/Docking/src/main/java/docking/DockingWindowManager.java

@@ -1904,7 +1904,7 @@ public class DockingWindowManager implements PropertyChangeListener, Placeholder
 
 	/**
 	 * Set the status text in the active component window.
-	 * @param text non-html status text
+	 * @param text status text
 	 */
 	public void setStatusText(String text) {
 		if (root != null) {

Ghidra/Framework/Docking/src/main/java/docking/widgets/checkbox/GCheckBox.java

@@ -20,7 +20,14 @@ import javax.swing.*;
 import docking.DockingUtils;
 
 /**
- * A {@link JCheckBox} that has HTML rendering disabled
+ * A {@link JCheckBox} that has HTML rendering disabled.
+ * <p>
+ * See also:
+ * <table border=1>
+ * 	<tr><th>Class</th><th>HTML rendering</th><th>Description</th></tr>
+ *  <tr><td>{@link GCheckBox}</td><td>NO</td><td>HTML disabled JCheckBox</td></tr>
+ *  <tr><td>{@link GHtmlCheckBox}</td><td>YES</td><td>HTML allowed JCheckBox</td></tr>
+ * </table>
  */
 public class GCheckBox extends JCheckBox {
 

Ghidra/Framework/Docking/src/main/java/docking/widgets/checkbox/GHtmlCheckBox.java

@@ -19,7 +19,14 @@ import javax.swing.*;
 
 /**
  * A {@link JCheckBox} that allows HTML rendering.
- */
+ * <p>
+ * See also:
+ * <table border=1>
+ * 	<tr><th>Class</th><th>HTML rendering</th><th>Description</th></tr>
+ *  <tr><td>{@link GCheckBox}</td><td>NO</td><td>HTML disabled JCheckBox</td></tr>
+ *  <tr><td>{@link GHtmlCheckBox}</td><td>YES</td><td>HTML allowed JCheckBox</td></tr>
+ * </table>
+*/
 public class GHtmlCheckBox extends JCheckBox {
 
 	/**

Ghidra/Framework/Docking/src/main/java/docking/widgets/label/GDHtmlLabel.java

@@ -17,8 +17,11 @@ package docking.widgets.label;
 
 import javax.swing.*;
 
+import docking.widgets.checkbox.GCheckBox;
+import docking.widgets.checkbox.GHtmlCheckBox;
+
 /**
- * A 'dynamic' label with HTML rendering allowed.
+ * A 'dynamic' label (the text can be changed), with HTML rendering allowed.
  * <p>
  * See also:
  * <table border=1>
@@ -28,6 +31,9 @@ import javax.swing.*;
  *  <tr><td>{@link GHtmlLabel}</td><td>Immutable</td><td>YES</td><td>Html unchangeable label</td></tr>
  *  <tr><td>{@link GDHtmlLabel}</td><td>Mutable</td><td>YES</td><td>Html changeable label</td></tr>
  *  <tr><td>{@link GIconLabel}</td><td>N/A</td><td>NO</td><td>Label that only has an icon image, no text</td></tr>
+ *  <tr><th colspan=4>Other components of note:</th></tr>
+ *  <tr><td>{@link GCheckBox}</td><td></td><td>NO</td><td>Non-html checkbox</td></tr>
+ *  <tr><td>{@link GHtmlCheckBox}</td><td></td><td>YES</td><td>Html checkbox</td></tr>
  * </table>
  */
 public class GDHtmlLabel extends JLabel {

Ghidra/Framework/Docking/src/main/java/docking/widgets/label/GDLabel.java

@@ -18,9 +18,11 @@ package docking.widgets.label;
 import javax.swing.*;
 
 import docking.DockingUtils;
+import docking.widgets.checkbox.GCheckBox;
+import docking.widgets.checkbox.GHtmlCheckBox;
 
 /**
- * A 'dynamic' label with HTML rendering disabled.
+ * A 'dynamic' label (the text can be changed), with HTML rendering disabled.
  * <p>
  * See also:
  * <table border=1>
@@ -30,6 +32,9 @@ import docking.DockingUtils;
  *  <tr><td>{@link GHtmlLabel}</td><td>Immutable</td><td>YES</td><td>Html unchangeable label</td></tr>
  *  <tr><td>{@link GDHtmlLabel}</td><td>Mutable</td><td>YES</td><td>Html changeable label</td></tr>
  *  <tr><td>{@link GIconLabel}</td><td>N/A</td><td>NO</td><td>Label that only has an icon image, no text</td></tr>
+ *  <tr><th colspan=4>Other components of note:</th></tr>
+ *  <tr><td>{@link GCheckBox}</td><td></td><td>NO</td><td>Non-html checkbox</td></tr>
+ *  <tr><td>{@link GHtmlCheckBox}</td><td></td><td>YES</td><td>Html checkbox</td></tr>
  * </table>
  */
 public class GDLabel extends JLabel {

Ghidra/Framework/Docking/src/main/java/docking/widgets/label/GHtmlLabel.java

@@ -17,10 +17,15 @@ package docking.widgets.label;
 
 import javax.swing.*;
 
+import org.apache.commons.lang3.StringUtils;
+
+import docking.widgets.checkbox.GCheckBox;
+import docking.widgets.checkbox.GHtmlCheckBox;
 import ghidra.util.Msg;
+import utilities.util.reflection.ReflectionUtilities;
 
 /**
- * An immutable label with HTML rendering allowed.
+ * An immutable label (the text can NOT be changed), with HTML rendering allowed.
  * <p>
  * See also:
  * <table border=1>
@@ -30,6 +35,9 @@ import ghidra.util.Msg;
  *  <tr><td>{@link GHtmlLabel}</td><td>Immutable</td><td>YES</td><td>Html unchangeable label</td></tr>
  *  <tr><td>{@link GDHtmlLabel}</td><td>Mutable</td><td>YES</td><td>Html changeable label</td></tr>
  *  <tr><td>{@link GIconLabel}</td><td>N/A</td><td>NO</td><td>Label that only has an icon image, no text</td></tr>
+ *  <tr><th colspan=4>Other components of note:</th></tr>
+ *  <tr><td>{@link GCheckBox}</td><td></td><td>NO</td><td>Non-html checkbox</td></tr>
+ *  <tr><td>{@link GHtmlCheckBox}</td><td></td><td>YES</td><td>Html checkbox</td></tr>
  * </table>
  */
 public class GHtmlLabel extends JLabel {
@@ -108,9 +116,11 @@ public class GHtmlLabel extends JLabel {
 	@Deprecated
 	@Override
 	public void setText(String text) {
-		if (getText() != null && !getText().isEmpty()) {
+		if (StringUtils.isEmpty(getText())) {
-			Msg.warn(this, "Trying to set text on an immutable label!  Current text: [" +
+			Msg.warn(this,
-				getText() + "], new text: [" + text + "]", new Throwable());
+				"Trying to set text on an immutable label!  Current text: [" + getText() +
+					"], new text: [" + text + "]",
+				ReflectionUtilities.createJavaFilteredThrowable());
 			return;
 		}
 		super.setText(text);

Ghidra/Framework/Docking/src/main/java/docking/widgets/label/GIconLabel.java

@@ -17,7 +17,12 @@ package docking.widgets.label;
 
 import javax.swing.*;
 
+import org.apache.commons.lang3.StringUtils;
+
+import docking.widgets.checkbox.GCheckBox;
+import docking.widgets.checkbox.GHtmlCheckBox;
 import ghidra.util.Msg;
+import utilities.util.reflection.ReflectionUtilities;
 
 /**
  * A label that only contains an image and no text.
@@ -30,6 +35,9 @@ import ghidra.util.Msg;
  *  <tr><td>{@link GHtmlLabel}</td><td>Immutable</td><td>YES</td><td>Html unchangeable label</td></tr>
  *  <tr><td>{@link GDHtmlLabel}</td><td>Mutable</td><td>YES</td><td>Html changeable label</td></tr>
  *  <tr><td>{@link GIconLabel}</td><td>N/A</td><td>NO</td><td>Label that only has an icon image, no text</td></tr>
+ *  <tr><th colspan=4>Other components of note:</th></tr>
+ *  <tr><td>{@link GCheckBox}</td><td></td><td>NO</td><td>Non-html checkbox</td></tr>
+ *  <tr><td>{@link GHtmlCheckBox}</td><td></td><td>YES</td><td>Html checkbox</td></tr>
  * </table>
  */
 public class GIconLabel extends GLabel {
@@ -88,9 +96,9 @@ public class GIconLabel extends GLabel {
 	@Deprecated
 	@Override
 	public void setText(String text) {
-		if (text != null && !text.isEmpty()) {
+		if (!StringUtils.isEmpty(text)) {
 			Msg.warn(this, "Trying to set text on an icon label! New text: [" + text + "]",
-				new Throwable());
+				ReflectionUtilities.createJavaFilteredThrowable());
 			return;
 		}
 		super.setText(text);

Ghidra/Framework/Docking/src/main/java/docking/widgets/label/GLabel.java

@@ -17,11 +17,16 @@ package docking.widgets.label;
 
 import javax.swing.*;
 
+import org.apache.commons.lang3.StringUtils;
+
 import docking.DockingUtils;
+import docking.widgets.checkbox.GCheckBox;
+import docking.widgets.checkbox.GHtmlCheckBox;
 import ghidra.util.Msg;
+import utilities.util.reflection.ReflectionUtilities;
 
 /**
- * An immutable label with HTML rendering disabled.
+ * An immutable label (the text can NOT be changed), with HTML rendering disabled.
  * <p>
  * See also:
  * <table border=1>
@@ -31,6 +36,9 @@ import ghidra.util.Msg;
  *  <tr><td>{@link GHtmlLabel}</td><td>Immutable</td><td>YES</td><td>Html unchangeable label</td></tr>
  *  <tr><td>{@link GDHtmlLabel}</td><td>Mutable</td><td>YES</td><td>Html changeable label</td></tr>
  *  <tr><td>{@link GIconLabel}</td><td>N/A</td><td>NO</td><td>Label that only has an icon image, no text</td></tr>
+ *  <tr><th colspan=4>Other components of note:</th></tr>
+ *  <tr><td>{@link GCheckBox}</td><td></td><td>NO</td><td>Non-html checkbox</td></tr>
+ *  <tr><td>{@link GHtmlCheckBox}</td><td></td><td>YES</td><td>Html checkbox</td></tr>
  * </table>
  */
 public class GLabel extends JLabel {
@@ -100,9 +108,6 @@ public class GLabel extends JLabel {
 		init();
 	}
 
-	//---------------------------------------------------------------------------------------------
-	// Protected ctors for derived classes
-	//---------------------------------------------------------------------------------------------
 	/**
 	 * 
 	 * @param image icon to display
@@ -126,8 +131,6 @@ public class GLabel extends JLabel {
 		init();
 	}
 
-	//---------------------------------------------------------------------------------------------
-
 	private void init() {
 		DockingUtils.turnOffHTMLRendering(this);
 	}
@@ -145,20 +148,29 @@ public class GLabel extends JLabel {
 	@Deprecated
 	@Override
 	public void setText(String text) {
-		if (getText() != null && !getText().isEmpty()) {
+		if (!StringUtils.isEmpty(getText())) {
-			Msg.warn(this, "Trying to set text on an immutable label!  Current text: [" +
+			Msg.warn(this,
-				getText() + "], new text: [" + text + "]", new Throwable());
+				"Trying to set text on an immutable label!  Current text: [" + getText() +
+					"], new text: [" + text + "]",
+				ReflectionUtilities.createJavaFilteredThrowable());
 			return;
 		}
 		warnAboutHtmlText(text);
 		super.setText(text);
 	}
 
+	/**
+	 * Helper function that logs a warning about a string text that looks like it has HTML text.
+	 * <p>
+	 * Use this when working with a string in a label that has already disabled HTML rendering.
+	 * <p>
+	 * @param text string to test for HTML and warn about
+	 */
 	public static void warnAboutHtmlText(String text) {
 		// #ifdef still_finding_html_labels_in_our_huge_codebase
-		if (text != null && (text.startsWith("<html>") || text.startsWith("<HTML>"))) {
+		if (StringUtils.startsWithIgnoreCase(text, "<html>")) {
 			Msg.warn(GLabel.class, "HTML text detected in non-HTML component: " + text,
-				new Throwable());
+				ReflectionUtilities.createJavaFilteredThrowable());
 		}
 		// #endif
 	}

Ghidra/Framework/Docking/src/main/java/docking/widgets/list/GList.java

@@ -27,8 +27,13 @@ import docking.widgets.table.GTable;
 
 /**
  * A sub-class of JList that provides an auto-lookup feature.
+ * <p>
  * The user can begin typing the first few letters of a desired
  * list element and the selection will automatically navigate to it.
+ * <p>
+ * HTML rendering is disabled by default.
+ * <p>
+ * 
  */
 public class GList<T> extends JList<T> {
 	private static final long serialVersionUID = 1L;

Ghidra/Framework/Docking/src/main/java/docking/widgets/list/GListCellRenderer.java

@@ -24,6 +24,8 @@ import docking.widgets.AbstractGCellRenderer;
 
 /**
  * Provides a common implementation of a list renderer, for use in both JList and JComboBox.
+ * <p>
+ * HTML rendering defaults to disabled.  See {@link #setHTMLRenderingEnabled(boolean)}.
  *
  * @param <E> the element-type this list models.
  */

Ghidra/Framework/Generic/src/main/java/resources/ResourceManager.java

@@ -539,7 +539,9 @@ public class ResourceManager {
 	}
 
 	/**
-	 * Load the image specified by filename; returns null if problems occur trying to load the file
+	 * Load the image specified by filename; returns the default bomb icon
+	 * if problems occur trying to load the file.
+	 * <p>
 	 * 
 	 * @param filename name of file to load, e.g., "images/home.gif"
 	 * @return the image icon stored in the bytes
@@ -574,6 +576,22 @@ public class ResourceManager {
 		return getDefaultIcon();
 	}
 
+	/**
+	 * Load the images specified by filenames; substitutes the default bomb icon
+	 * if problems occur trying to load an individual file.
+	 * <p>
+	 * @param filenames vararg list of string filenames (ie. "images/home.gif")
+	 * @return list of ImageIcons with each image, problem / missing images replaced with
+	 * the default icon.
+	 */
+	public static List<ImageIcon> loadImages(String... filenames) {
+		List<ImageIcon> results = new ArrayList<>(filenames.length);
+		for (String filename : filenames) {
+			results.add(loadImage(filename));
+		}
+		return results;
+	}
+
 	/**
 	 * A convenience method to force the image denoted by <code>filename</code> to be read 
 	 * from disk and to not use the cached version 
@@ -690,12 +708,4 @@ public class ResourceManager {
 		testSearchPaths = results;
 		return testSearchPaths;
 	}
-
-	public static List<ImageIcon> loadImages(String... filenames) {
-		List<ImageIcon> results = new ArrayList<>(filenames.length);
-		for (String filename : filenames) {
-			results.add(loadImage(filename));
-		}
-		return results;
-	}
 }