GP-5724 - Fixed potential deadlock with options registration

Ghidra/Features/Base/src/main/java/ghidra/app/script/GhidraScript.java

@@ -1790,13 +1790,13 @@ public abstract class GhidraScript extends FlatProgramAPI {
 	 * This format object may be used to format any code unit (instruction/data) using
 	 * the same option settings.
 	 *
-	 *  @return code unit format when in GUI mode, default format in headless
+	 * @return code unit format when in GUI mode, default format in headless
 	 */
 	public CodeUnitFormat getCodeUnitFormat() {
 		PluginTool tool = state.getTool();
 		if (cuFormat == null) {
 			if (tool != null) {
-				cuFormat = new BrowserCodeUnitFormat(state.getTool());
+				cuFormat = new BrowserCodeUnitFormat(tool);
 			}
 			else {
 				cuFormat = new CodeUnitFormat(ShowBlockName.NEVER, ShowNamespace.NON_LOCAL);
@@ -3855,7 +3855,7 @@ public abstract class GhidraScript extends FlatProgramAPI {
 	 * @see #getPlateComment(Address)
 	 */
 	public String getPlateCommentAsRendered(Address address) {
-		String comment = currentProgram.getListing().getComment(CodeUnit.PLATE_COMMENT, address);
+		String comment = currentProgram.getListing().getComment(CommentType.PLATE, address);
 		PluginTool tool = state.getTool();
 		if (tool != null) {
 			comment = CommentUtils.getDisplayString(comment, currentProgram);
@@ -3874,7 +3874,7 @@ public abstract class GhidraScript extends FlatProgramAPI {
 	 * @see #getPreComment(Address)
 	 */
 	public String getPreCommentAsRendered(Address address) {
-		String comment = currentProgram.getListing().getComment(CodeUnit.PRE_COMMENT, address);
+		String comment = currentProgram.getListing().getComment(CommentType.PRE, address);
 		PluginTool tool = state.getTool();
 		if (tool != null) {
 			comment = CommentUtils.getDisplayString(comment, currentProgram);
@@ -3892,7 +3892,7 @@ public abstract class GhidraScript extends FlatProgramAPI {
 	 * @see #getPostComment(Address)
 	 */
 	public String getPostCommentAsRendered(Address address) {
-		String comment = currentProgram.getListing().getComment(CodeUnit.POST_COMMENT, address);
+		String comment = currentProgram.getListing().getComment(CommentType.POST, address);
 		PluginTool tool = state.getTool();
 		if (tool != null) {
 			comment = CommentUtils.getDisplayString(comment, currentProgram);
@@ -3910,7 +3910,7 @@ public abstract class GhidraScript extends FlatProgramAPI {
 	 * @see #getEOLComment(Address)
 	 */
 	public String getEOLCommentAsRendered(Address address) {
-		String comment = currentProgram.getListing().getComment(CodeUnit.EOL_COMMENT, address);
+		String comment = currentProgram.getListing().getComment(CommentType.EOL, address);
 		PluginTool tool = state.getTool();
 		if (tool != null) {
 			comment = CommentUtils.getDisplayString(comment, currentProgram);
@@ -3929,7 +3929,7 @@ public abstract class GhidraScript extends FlatProgramAPI {
 	 */
 	public String getRepeatableCommentAsRendered(Address address) {
 		String comment =
-			currentProgram.getListing().getComment(CodeUnit.REPEATABLE_COMMENT, address);
+			currentProgram.getListing().getComment(CommentType.REPEATABLE, address);
 		PluginTool tool = state.getTool();
 		if (tool != null) {
 			comment = CommentUtils.getDisplayString(comment, currentProgram);

Ghidra/Features/Base/src/main/java/ghidra/app/util/viewer/field/BrowserCodeUnitFormatOptions.java

@@ -4,9 +4,9 @@
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
  * You may obtain a copy of the License at
- * 
+ *
  *      http://www.apache.org/licenses/LICENSE-2.0
- * 
+ *
  * Unless required by applicable law or agreed to in writing, software
  * distributed under the License is distributed on an "AS IS" BASIS,
  * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
@@ -105,67 +105,74 @@ public class BrowserCodeUnitFormatOptions extends CodeUnitFormatOptions
 		this.fieldOptions = fieldOptions;
 		this.displayOptions = new OptionsBasedDataTypeDisplayOptions(fieldOptions);
 		templateSimplifier = new TemplateSimplifier(fieldOptions);
+
 		boolean exists = fieldOptions.isRegistered(NAMESPACE_OPTIONS);
-
 		if (!exists) {
-			fieldOptions.registerOption(NAMESPACE_OPTIONS, OptionType.CUSTOM_TYPE,
+			registerOptions();
-				new NamespaceWrappedOption(), null, NAMESPACE_OPTIONS_DESCRIPTIONS,
-				() -> new NamespacePropertyEditor());
-
-			HelpLocation hl = new HelpLocation("CodeBrowserPlugin", "Operands_Field");
-			fieldOptions.getOptions(GhidraOptions.OPERAND_GROUP_TITLE).setOptionsHelpLocation(hl);
-
-			fieldOptions.registerOption(GhidraOptions.SHOW_BLOCK_NAME_OPTION, false, hl,
-				"Prepends memory block names to labels in the operands field.");
-			fieldOptions.registerOption(REGISTER_VARIABLE_MARKUP_OPTION, true, hl,
-				"Markup function register variable references");
-			fieldOptions.registerOption(STACK_VARIABLE_MARKUP_OPTION, true, hl,
-				"Markup function stack variable references");
-			fieldOptions.registerOption(INFERRED_VARIABLE_MARKUP_OPTION, true, hl,
-				"Include INFERRED variable references in markup");
-			fieldOptions.registerOption(ALWAYS_SHOW_PRIMARY_REFERENCE_MARKUP_OPTION, true, hl,
-				"Forces the primary reference to be rendered with the operand, using the => separator if necessary");
-			fieldOptions.registerOption(FOLLOW_POINTER_REFERENCE_MARKUP_OPTION, true, hl,
-				"Markup pointer READ/INDIRECT reference with symbol referenced by pointer.  " +
-					"An indirectly referenced symbol name will be prefixed with -> .");
-			fieldOptions.registerOption(SCALAR_ADJUSTMENT_OPTION, false, hl,
-				"Include scalar adjustment of certain reference offsets to maintain replaced scalar value");
-			fieldOptions.registerOption(SHOW_MUTABILITY_OPTION, false, hl,
-				"Include data mnemonic prefix of 'const' or 'volatile' based upon data setting");
-			fieldOptions.registerOption(SHOW_OFFCUT_INFO_OPTION, true, hl,
-				"Include trailing offcut address + offset data when showing offcut data");
 		}
-		updateFormat();
+
+		loadOptions();
 
 		if (autoUpdate) {
 			fieldOptions.addOptionsChangeListener(this);
 		}
 	}
 
+	private void registerOptions() {
+		fieldOptions.registerOption(NAMESPACE_OPTIONS, OptionType.CUSTOM_TYPE,
+			new NamespaceWrappedOption(), null, NAMESPACE_OPTIONS_DESCRIPTIONS,
+			() -> new NamespacePropertyEditor());
+
+		HelpLocation hl = new HelpLocation("CodeBrowserPlugin", "Operands_Field");
+		fieldOptions.getOptions(GhidraOptions.OPERAND_GROUP_TITLE).setOptionsHelpLocation(hl);
+
+		fieldOptions.registerOption(GhidraOptions.SHOW_BLOCK_NAME_OPTION, false, hl,
+			"Prepends memory block names to labels in the operands field.");
+		fieldOptions.registerOption(REGISTER_VARIABLE_MARKUP_OPTION, true, hl,
+			"Markup function register variable references");
+		fieldOptions.registerOption(STACK_VARIABLE_MARKUP_OPTION, true, hl,
+			"Markup function stack variable references");
+		fieldOptions.registerOption(INFERRED_VARIABLE_MARKUP_OPTION, true, hl,
+			"Include INFERRED variable references in markup");
+		fieldOptions.registerOption(ALWAYS_SHOW_PRIMARY_REFERENCE_MARKUP_OPTION, true, hl,
+			"Forces the primary reference to be rendered with the operand, using the => separator if necessary");
+		fieldOptions.registerOption(FOLLOW_POINTER_REFERENCE_MARKUP_OPTION, true, hl,
+			"Markup pointer READ/INDIRECT reference with symbol referenced by pointer.  " +
+				"An indirectly referenced symbol name will be prefixed with -> .");
+		fieldOptions.registerOption(SCALAR_ADJUSTMENT_OPTION, false, hl,
+			"Include scalar adjustment of certain reference offsets to maintain replaced scalar value");
+		fieldOptions.registerOption(SHOW_MUTABILITY_OPTION, false, hl,
+			"Include data mnemonic prefix of 'const' or 'volatile' based upon data setting");
+		fieldOptions.registerOption(SHOW_OFFCUT_INFO_OPTION, true, hl,
+			"Include trailing offcut address + offset data when showing offcut data");
+	}
+
 	@Override
 	public void optionsChanged(ToolOptions options, String optionName, Object oldValue,
 			Object newValue) {
 		if (templateSimplifier.fieldOptionsChanged(options, optionName, oldValue, newValue)) {
 			notifyListeners();
 		}
-		else if (optionName.equals(GhidraOptions.SHOW_BLOCK_NAME_OPTION) ||
+
-			optionName.equals(REGISTER_VARIABLE_MARKUP_OPTION) ||
+		switch (optionName) {
-			optionName.equals(STACK_VARIABLE_MARKUP_OPTION) ||
+			case GhidraOptions.SHOW_BLOCK_NAME_OPTION:
-			optionName.equals(INFERRED_VARIABLE_MARKUP_OPTION) ||
+			case REGISTER_VARIABLE_MARKUP_OPTION:
-			optionName.equals(ALWAYS_SHOW_PRIMARY_REFERENCE_MARKUP_OPTION) ||
+			case STACK_VARIABLE_MARKUP_OPTION:
-			optionName.equals(FOLLOW_POINTER_REFERENCE_MARKUP_OPTION) ||
+			case INFERRED_VARIABLE_MARKUP_OPTION:
-			optionName.equals(SCALAR_ADJUSTMENT_OPTION) || optionName.equals(NAMESPACE_OPTIONS) ||
+			case ALWAYS_SHOW_PRIMARY_REFERENCE_MARKUP_OPTION:
-			optionName.equals(SHOW_MUTABILITY_OPTION) ||
+			case FOLLOW_POINTER_REFERENCE_MARKUP_OPTION:
-			optionName.equals(SHOW_OFFCUT_INFO_OPTION)) {
+			case SCALAR_ADJUSTMENT_OPTION:
-			updateFormat();
+			case NAMESPACE_OPTIONS:
-			notifyListeners();
+			case SHOW_MUTABILITY_OPTION:
+			case SHOW_OFFCUT_INFO_OPTION:
+				loadOptions();
+				notifyListeners();
+				break;
 		}
 	}
 
-	private void updateFormat() {
+	private void loadOptions() {
-		fieldOptions.registerOption(NAMESPACE_OPTIONS, OptionType.CUSTOM_TYPE,
+
-			new NamespaceWrappedOption(), null, NAMESPACE_OPTIONS_DESCRIPTIONS,
-			() -> new NamespacePropertyEditor());
 		CustomOption customOption =
 			fieldOptions.getCustomOption(NAMESPACE_OPTIONS, new NamespaceWrappedOption());
 		if (!(customOption instanceof NamespaceWrappedOption)) {
@@ -173,8 +180,8 @@ public class BrowserCodeUnitFormatOptions extends CodeUnitFormatOptions
 				"Someone set an option for " + NAMESPACE_OPTIONS + " that is not the expected " +
 					"ghidra.app.util.viewer.field.NamespaceWrappedOption type.");
 		}
-		NamespaceWrappedOption namespaceOption = (NamespaceWrappedOption) customOption;
 
+		NamespaceWrappedOption namespaceOption = (NamespaceWrappedOption) customOption;
 		showBlockName = fieldOptions.getBoolean(GhidraOptions.SHOW_BLOCK_NAME_OPTION, false)
 				? CodeUnitFormatOptions.ShowBlockName.NON_LOCAL
 				: CodeUnitFormatOptions.ShowBlockName.NEVER;
@@ -198,8 +205,8 @@ public class BrowserCodeUnitFormatOptions extends CodeUnitFormatOptions
 		else if (namespaceOption.isShowNonLocalNamespace()) {
 			showNamespace = CodeUnitFormatOptions.ShowNamespace.NON_LOCAL;
 		}
-		showLibraryInNamespace = namespaceOption.isShowLibraryInNamespace();
 
+		showLibraryInNamespace = namespaceOption.isShowLibraryInNamespace();
 		doRegVariableMarkup = fieldOptions.getBoolean(REGISTER_VARIABLE_MARKUP_OPTION, true);
 		doStackVariableMarkup = fieldOptions.getBoolean(STACK_VARIABLE_MARKUP_OPTION, true);
 		includeInferredVariableMarkup =

Ghidra/Features/Base/src/main/java/ghidra/app/util/viewer/format/FormatManager.java

@@ -235,8 +235,8 @@ public class FormatManager implements OptionsChangeListener {
 	/**
 	 * Returns the format model to use for the internals of open structures.
 	 * 
-	 * @param data
+	 * @param data the data code unit to get the format model for.
-	 *            the data code unit to get the format model for.
+	 * @return the format model to use for the internals of open structures.
 	 */
 	public FieldFormatModel getOpenDataFormat(Data data) {
 
@@ -312,6 +312,7 @@ public class FormatManager implements OptionsChangeListener {
 
 	/**
 	 * Returns the width of the widest model in this manager.
+	 * @return the width of the widest model in this manager.
 	 */
 	public int getMaxWidth() {
 		int maxWidth = 0;
@@ -811,6 +812,7 @@ public class FormatManager implements OptionsChangeListener {
 	/**
 	 * Returns the maximum number of possible rows in a layout. This would only
 	 * occur if some address had every possible type of information to be displayed.
+	 * @return the maximum number of possible rows in a layout.
 	 */
 	public int getMaxNumRows() {
 		return maxNumRows;
@@ -875,8 +877,9 @@ public class FormatManager implements OptionsChangeListener {
 	}
 
 	/**
-	 * Returns the {@link ListingHighlightProvider} that should be used when creating {@link FieldFactory}
+	 * Returns the {@link ListingHighlightProvider} that should be used when creating 
-	 * objects.
+	 * {@link FieldFactory} objects.
+	 * @return the provider
 	 */
 	public ListingHighlightProvider getFormatHighlightProvider() {
 		return highlightProvider;

Ghidra/Framework/Gui/src/main/java/ghidra/framework/options/AbstractOptions.java

@@ -4,9 +4,9 @@
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
  * You may obtain a copy of the License at
- * 
+ *
  *      http://www.apache.org/licenses/LICENSE-2.0
- * 
+ *
  * Unless required by applicable law or agreed to in writing, software
  * distributed under the License is distributed on an "AS IS" BASIS,
  * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
@@ -141,7 +141,7 @@ public abstract class AbstractOptions implements Options {
 	}
 
 	@Override
-	public synchronized void registerOption(String optionName, OptionType type, Object defaultValue,
+	public void registerOption(String optionName, OptionType type, Object defaultValue,
 			HelpLocation help, String description, Supplier<PropertyEditor> editorSupplier) {
 
 		if (type == OptionType.NO_TYPE) {
@@ -192,16 +192,18 @@ public abstract class AbstractOptions implements Options {
 				ReflectionUtilities.createJavaFilteredThrowable());
 		}
 
-		Option currentOption = getExistingComptibleOption(optionName, type);
+		synchronized (this) {
-		if (currentOption != null) {
+
-			currentOption.updateRegistration(description, help, defaultValue, editor);
+			Option currentOption = getExistingComptibleOption(optionName, type);
-			return;
+			if (currentOption != null) {
+				currentOption.updateRegistration(description, help, defaultValue, editor);
+				return;
+			}
+
+			Option option =
+				createRegisteredOption(optionName, type, description, help, defaultValue, editor);
+			valueMap.put(optionName, option);
 		}
-
-		Option option =
-			createRegisteredOption(optionName, type, description, help, defaultValue, editor);
-
-		valueMap.put(optionName, option);
 	}
 
 	private void warnShouldUseTheme(String optionType) {

Ghidra/Framework/Gui/src/main/java/ghidra/framework/options/Options.java

@@ -4,9 +4,9 @@
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
  * You may obtain a copy of the License at
- * 
+ *
  *      http://www.apache.org/licenses/LICENSE-2.0
- * 
+ *
  * Unless required by applicable law or agreed to in writing, software
  * distributed under the License is distributed on an "AS IS" BASIS,
  * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
@@ -160,12 +160,17 @@ public interface Options {
 	 * {@link #registerThemeColorBinding(String, String, HelpLocation, String)} or
 	 * {@link #registerThemeFontBinding(String, String, HelpLocation, String)}.
 	 * <P>
-	 * Note: we use a <i>supplier</i> of a custom editor, instead of a custom editor, to avoid
+	 * Note 1: we use a <i>supplier</i> of a custom editor, instead of a custom editor, to avoid
 	 * creating {@link PropertyEditor}s until needed.  This allows us to use the same API in both
 	 * GUI mode and headless mode.  If GUI property editors are created in headless mode, exceptions
 	 * may be thrown.  This API will not use the supplier when in headless mode, this avoiding the
 	 * creation of GUI components.  For this to work correctly, clients using custom property
 	 * editors must defer construction of the editor until the supplier is called.
+	 * <P>
+	 * Note 2: most clients are calling this API on the Swing thread, but that is not a requirement.
+	 * Any clients calling this method not on the Swing thread that also have a custom property 
+	 * editor, need to ensure they create that editor on the Swing thread (this is a Java 
+	 * requirement).
 	 *
 	 * @param optionName the name of the option being registered.
 	 * @param type the OptionType for this options.