GP-2295 - fixes and debug code for a history actions stack trace

Ghidra/Features/Base/src/main/java/ghidra/app/nav/Navigatable.java

@@ -35,9 +35,9 @@ public interface Navigatable {
 
 	/**
 	 * Commands this navigatable to goto (display) the given program and location
-	 * 
+	 *
 	 * @param program the program
-	 * 
+	 *
 	 * @param location the location in that program to display
 	 * @return true if the goto was successful
 	 */
@@ -45,35 +45,35 @@ public interface Navigatable {
 
 	/**
 	 * Returns the current location of this Navigatable
-	 * 
+	 *
 	 * @return the current location of this Navigatable
 	 */
 	public ProgramLocation getLocation();
 
 	/**
 	 * Returns the current Program of this Navigatable
-	 * 
+	 *
 	 * @return the current Program of this Navigatable
 	 */
 	public Program getProgram();
 
 	/**
 	 * Returns the view state for this navigatable
-	 * 
+	 *
 	 * @return the view state for this navigatable
 	 */
 	public LocationMemento getMemento();
 
 	/**
 	 * Sets the view state for this navigatable. This is used later to restore the view state.
-	 * 
+	 *
 	 * @param memento the state of this navigatable
 	 */
 	public void setMemento(LocationMemento memento);
 
 	/**
 	 * Returns an icon that represents this Navigatable
-	 * 
+	 *
 	 * @return the icon
 	 */
 	public Icon getNavigatableIcon();
@@ -81,7 +81,7 @@ public interface Navigatable {
 	/**
 	 * Returns true if this Navigatable is "connected". Navigatables are connected if they produce
 	 * and consume location and selection events.
-	 * 
+	 *
 	 * @return true if this Navigatable is "connected"
 	 */
 	public boolean isConnected();
@@ -89,8 +89,8 @@ public interface Navigatable {
 	/**
 	 * Return true if this Navigatable is part of the "dynamic analysis" or "debugger" user
 	 * interface.
-	 * 
+	 *
-	 * @return tre if this Navigatable is "dynamic"
+	 * @return true if this Navigatable is "dynamic"
 	 */
 	default public boolean isDynamic() {
 		return false;
@@ -98,7 +98,7 @@ public interface Navigatable {
 
 	/**
 	 * Currently only the 'connected' windows support markers
-	 * 
+	 *
 	 * @return true if this navigatable supports markers
 	 */
 	public boolean supportsMarkers();
@@ -110,78 +110,78 @@ public interface Navigatable {
 
 	/**
 	 * Returns true if this provider is visible
-	 * 
+	 *
 	 * @return true if visible
 	 */
 	public boolean isVisible();
 
 	/**
 	 * Tells this Navigatable to set its selection to the given selection
-	 * 
+	 *
 	 * @param selection the selection to set.
 	 */
 	public void setSelection(ProgramSelection selection);
 
 	/**
 	 * Tells this Navigatable to set its highlight to the given highlight
-	 * 
+	 *
 	 * @param highlight the highlight to set.
 	 */
 	public void setHighlight(ProgramSelection highlight);
 
 	/**
 	 * Returns the current selection of this Navigatable
-	 * 
+	 *
 	 * @return the current selection of this Navigatable
 	 */
 	public ProgramSelection getSelection();
 
 	/**
 	 * Returns the current highlight of this Navigatable
-	 * 
+	 *
 	 * @return the current highlight of this Navigatable
 	 */
 	public ProgramSelection getHighlight();
 
 	/**
 	 * Returns the current text selection or null
-	 * 
+	 *
 	 * @return the text selection
 	 */
 	public String getTextSelection();
 
 	/**
 	 * Adds a listener to be notified if this Navigatable is terminated
-	 * 
+	 *
 	 * @param listener the listener to be notified when this Navigatable is closed
 	 */
 	public void addNavigatableListener(NavigatableRemovalListener listener);
 
 	/**
 	 * Removes a listener to be notified if this Navigatable is terminated.
-	 * 
+	 *
 	 * @param listener the listener that no longer should be notified when this Navigatable is
 	 *            closed.
 	 */
 	public void removeNavigatableListener(NavigatableRemovalListener listener);
 
 	/**
-	 * Returns true if this navigatable is no longer valid, false if it is still good
+	 * Returns true if this navigatable is no longer valid
-	 * 
+	 *
-	 * @return true if this navigatable is no longer valid, false if it is still good
+	 * @return true if this navigatable is no longer valid
 	 */
 	public boolean isDisposed();
 
 	/**
 	 * Returns true if this navigatable supports highlighting
-	 * 
+	 *
 	 * @return true if this navigatable supports highlighting
 	 */
 	public boolean supportsHighlight();
 
 	/**
 	 * Set the highlight provider for the given program
-	 * 
+	 *
 	 * @param highlightProvider the provider
 	 * @param program the program
 	 */
@@ -189,7 +189,7 @@ public interface Navigatable {
 
 	/**
 	 * Removes the given highlight provider for the given program
-	 * 
+	 *
 	 * @param highlightProvider the provider
 	 * @param program the program
 	 */

Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/navigation/NavigationHistoryPlugin.java

@@ -41,7 +41,7 @@ import ghidra.util.bean.opteditor.OptionsVetoException;
  * viewer plugins to change their focus to a certain address. As viewer plugins are directed to one
  * or more addresses it maintains information about where the viewers have been to support ability
  * for the viewers to go back to a previous "focus" point.
- * 
+ *
  * Services Provided: NavigationHistoryService Events Consumed: ProgramLocationPluginEvent,
  * ProgramPluginEvent Event Produced: HistoryChangePluginEvent Actions: None.
  */
@@ -447,7 +447,7 @@ public class NavigationHistoryPlugin extends Plugin
 
 //==================================================================================================
 // Inner Classes
-//==================================================================================================    
+//==================================================================================================
 
 	private static class HistoryList {
 		private List<LocationMemento> list = new ArrayList<>();
@@ -551,7 +551,7 @@ public class NavigationHistoryPlugin extends Plugin
 		/**
 		 * Find the next history LocationMemento that contains a different function. If no such
 		 * LocationMemento is found, null is returned.
-		 * 
+		 *
 		 * @param navigatable the navigatable being navigated
 		 * @param moveTo true means after finding, get current location to it. false to just find
 		 *            and do nothing
@@ -595,7 +595,7 @@ public class NavigationHistoryPlugin extends Plugin
 		/**
 		 * Find the previous history LocationMemento that contains a different function. If no such
 		 * LocationMemento is found, null is returned.
-		 * 
+		 *
 		 * @param navigatable the navigatable being navigated
 		 * @param moveTo true means after finding, get current location to it. false to just find
 		 *            and do nothing
@@ -627,35 +627,71 @@ public class NavigationHistoryPlugin extends Plugin
 		}
 
 		private Function getPreviousStartFunction(Navigatable navigatable) {
-			ProgramLocation location = navigatable.getLocation();
+			ProgramLocation location = getProgramLocation(navigatable);
 			if (location == null) {
 				return null;
 			}
 
 			Address currentAddress = location.getAddress();
+			Program program = location.getProgram();
+			FunctionManager functionManager = program.getFunctionManager();
+			return functionManager.getFunctionContaining(currentAddress);
+		}
+
+		private ProgramLocation getProgramLocation(Navigatable navigatable) {
 
 			//
 			// The active component may still be showing the previously loaded function, instead
-			// of the current location when that location is not in a function.  In that case, 
+			// of the current location when that location is not in a function.  In that case,
 			// when that provider is focused, prefer its notion of the current function so that
 			// users navigating from that view will go to the function before the one that is
 			// on the history stack.  This should feel more intuitive to the user, with the risk
 			// that the navigation actions will sometimes feel inconsistent, depending upon
 			// what view is focused.
 			//
-			DockingWindowManager manager = DockingWindowManager.getActiveInstance();
+			DockingWindowManager dwm = DockingWindowManager.getActiveInstance();
-			ComponentProvider provider = manager.getActiveComponentProvider();
+			ComponentProvider activeProvider = dwm.getActiveComponentProvider();
-			if (provider instanceof Navigatable) {
+			if (activeProvider instanceof Navigatable) {
-				LocationMemento memento = ((Navigatable) provider).getMemento();
+				Navigatable activeNavigatable = (Navigatable) activeProvider;
-				ProgramLocation otherLocation = memento.getProgramLocation();
+				LocationMemento memento = activeNavigatable.getMemento();
-				if (otherLocation != null) {
+				ProgramLocation location =
-					currentAddress = otherLocation.getAddress();
+					validateProgramLocation(activeNavigatable, memento.getProgramLocation());
+				if (location != null) {
+					// prefer the active natigatable's location
+					return location;
 				}
 			}
 
+			ProgramLocation location = navigatable.getLocation();
+			return validateProgramLocation(navigatable, location);
+		}
+
+		private ProgramLocation validateProgramLocation(Navigatable navigatable,
+				ProgramLocation location) {
+			if (location == null) {
+				return null;
+			}
+			if (isClosedProgram(navigatable, location)) {
+				return null;
+			}
+			return location;
+		}
+
+		// TODO debug code that can go away if we find the source of the lingering program location
+		// being used that causes the program closed exception
+		private boolean isClosedProgram(Navigatable navigatable, ProgramLocation location) {
 			Program program = location.getProgram();
-			FunctionManager functionManager = program.getFunctionManager();
+			if (program.isClosed()) {
-			return functionManager.getFunctionContaining(currentAddress);
+
+				Msg.showError(this, null, "Closed Program",
+					"The Navigation History Plugin is using a closed program.\n" + "Program: " +
+						program.getName() + "Navigatable: " +
+						navigatable.getClass().getSimpleName() + "\n" + "Location: " +
+						location.getClass().getSimpleName() + " @ " + location.getAddress());
+				return true;
+			}
+
+			return false;
 		}
 
 		void remove(LocationMemento location) {

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

@@ -30,28 +30,17 @@ import ghidra.util.SystemUtilities;
 /**
  * Visible Plugin to show ByteBlock data in various formats.
  */
-@PluginInfo(
+@PluginInfo(status = PluginStatus.RELEASED, packageName = CorePluginPackage.NAME, category = PluginCategoryNames.BYTE_VIEWER, shortDescription = "Displays bytes in memory", description = "Provides a component for showing the bytes in memory.  " +
-	status = PluginStatus.RELEASED,
+	"Additional plugins provide capabilites for this plugin" +
-	packageName = CorePluginPackage.NAME,
+	" to show the bytes in various formats (e.g., hex, octal, decimal)." +
-	category = PluginCategoryNames.BYTE_VIEWER,
+	"  The hex format plugin is loaded by default when this plugin is loaded.", servicesRequired = {
-	shortDescription = "Displays bytes in memory",
-	description = "Provides a component for showing the bytes in memory.  " +
-		"Additional plugins provide capabilites for this plugin" +
-		" to show the bytes in various formats (e.g., hex, octal, decimal)." +
-		"  The hex format plugin is loaded by default when this plugin is loaded.",
-	servicesRequired = {
 		ProgramManager.class, GoToService.class, NavigationHistoryService.class,
-		ClipboardService.class,
+		ClipboardService.class, }, eventsConsumed = { ProgramLocationPluginEvent.class,
-	},
+			ProgramActivatedPluginEvent.class, ProgramSelectionPluginEvent.class,
-	eventsConsumed = {
+			ProgramHighlightPluginEvent.class, ProgramClosedPluginEvent.class,
-		ProgramLocationPluginEvent.class, ProgramActivatedPluginEvent.class,
+			ByteBlockChangePluginEvent.class, }, eventsProduced = {
-		ProgramSelectionPluginEvent.class, ProgramHighlightPluginEvent.class,
+				ProgramLocationPluginEvent.class, ProgramSelectionPluginEvent.class,
-		ProgramClosedPluginEvent.class, ByteBlockChangePluginEvent.class,
+				ByteBlockChangePluginEvent.class, })
-	},
-	eventsProduced = {
-		ProgramLocationPluginEvent.class, ProgramSelectionPluginEvent.class,
-		ByteBlockChangePluginEvent.class,
-	})
 public class ByteViewerPlugin extends AbstractByteViewerPlugin<ProgramByteViewerComponentProvider> {
 
 	public ByteViewerPlugin(PluginTool tool) {
@@ -73,6 +62,7 @@ public class ByteViewerPlugin extends AbstractByteViewerPlugin<ProgramByteViewer
 
 		if (event instanceof ProgramActivatedPluginEvent) {
 			currentProgram = ((ProgramActivatedPluginEvent) event).getActiveProgram();
+			currentLocation = null;
 		}
 		else if (event instanceof ProgramLocationPluginEvent) {
 			currentLocation = ((ProgramLocationPluginEvent) event).getLocation();
@@ -131,8 +121,7 @@ public class ByteViewerPlugin extends AbstractByteViewerPlugin<ProgramByteViewer
 	}
 
 	@Override
-	public void highlightChanged(ByteViewerComponentProvider provider,
+	public void highlightChanged(ByteViewerComponentProvider provider, ProgramSelection highlight) {
-			ProgramSelection highlight) {
 		if (provider == connectedProvider) {
 			tool.firePluginEvent(new ProgramHighlightPluginEvent(getName(), highlight,
 				connectedProvider.getProgram()));

Ghidra/Features/Decompiler/src/main/java/ghidra/app/plugin/core/decompile/DecompilePlugin.java

@@ -74,9 +74,15 @@ public class DecompilePlugin extends Plugin {
 	 * or when switching program tabs.
 	 */
 	SwingUpdateManager delayedLocationUpdateMgr = new SwingUpdateManager(200, 200, () -> {
-		if (currentLocation != null) {
+		if (currentLocation == null) {
-			connectedProvider.setLocation(currentLocation, null);
+			return;
 		}
+
+		Program locationProgram = currentLocation.getProgram();
+		if (locationProgram.isClosed()) {
+			return; // not sure if this can happen
+		}
+		connectedProvider.setLocation(currentLocation, null);
 	});
 
 	public DecompilePlugin(PluginTool tool) {

Ghidra/Features/Decompiler/src/main/java/ghidra/app/plugin/core/decompile/DecompilerProvider.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.

Ghidra/Features/FunctionGraph/src/main/java/ghidra/app/plugin/core/functiongraph/FGProvider.java

@@ -135,7 +135,7 @@ public class FGProvider extends VisualGraphComponentProvider<FGVertex, FGEdge, F
 
 	@Override
 	public boolean isSnapshot() {
-		// we are a snapshot when we are 'disconnected' 
+		// we are a snapshot when we are 'disconnected'
 		return !isConnected();
 	}
 
@@ -241,7 +241,7 @@ public class FGProvider extends VisualGraphComponentProvider<FGVertex, FGEdge, F
 	/**
 	 * Gives to the clipboard of this provider the given string.  This will prime the clipboard
 	 * such that a copy action will copy the given string.
-	 * 
+	 *
 	 * @param string the string to set
 	 */
 	public void setClipboardStringContent(String string) {
@@ -351,7 +351,7 @@ public class FGProvider extends VisualGraphComponentProvider<FGVertex, FGEdge, F
 
 	/**
 	 * Called to signal to this provider that it should update its state due to a new function
-	 * being graphed.  The UI is updated by the controller without this provider's knowledge. 
+	 * being graphed.  The UI is updated by the controller without this provider's knowledge.
 	 * This call here is to signal that the provider needs to update its metadata.
 	 */
 	public void functionGraphDataChanged() {
@@ -413,15 +413,15 @@ public class FGProvider extends VisualGraphComponentProvider<FGVertex, FGEdge, F
 	/**
 	 * Called from within the FunctionGraph when locations are changed (e.g., if a user clicks
 	 * inside of a vertex)
-	 * 
+	 *
-	 * @param newLocation the new location 
+	 * @param newLocation the new location
 	 */
 	public void graphLocationChanged(ProgramLocation newLocation) {
 		storeLocation(newLocation);
 
 		if (isFocusedProvider()) {
 
-			// Note: this is the easy way to avoid odd event bouncing--only send events out if 
+			// Note: this is the easy way to avoid odd event bouncing--only send events out if
 			//       we are focused, as this implies the user is driving the events.  A better
 			//       metaphor for handling external and internal program locations is needed to
 			//       simplify the logic of when to broadcast location changes.
@@ -434,8 +434,8 @@ public class FGProvider extends VisualGraphComponentProvider<FGVertex, FGEdge, F
 	/**
 	 * Called from within the FunctionGraph when selections are changed (e.g., if a user clicks
 	 * inside of a vertex)
-	 * 
+	 *
-	 * @param selection the new selection 
+	 * @param selection the new selection
 	 */
 	public void graphSelectionChanged(ProgramSelection selection) {
 		storeSelection(selection);
@@ -476,10 +476,10 @@ public class FGProvider extends VisualGraphComponentProvider<FGVertex, FGEdge, F
 	}
 
 	/**
-	 * Called when for location changes that are <b>external</b> to the function graph (e.g., 
+	 * Called when for location changes that are <b>external</b> to the function graph (e.g.,
 	 * when the user clicks in Ghidra's Listing window)
-	 * 
+	 *
-	 * @param newLocation the new location 
+	 * @param newLocation the new location
 	 */
 	void setLocation(ProgramLocation newLocation) {
 		pendingLocation = newLocation;
@@ -497,6 +497,13 @@ public class FGProvider extends VisualGraphComponentProvider<FGVertex, FGEdge, F
 			return;
 		}
 
+		Program program = newLocation.getProgram();
+		if (program.isClosed()) {
+			// this method is called from an update manager, which means that the callback may
+			// happen after the notification that the program was closed
+			return;
+		}
+
 		setLocationNow(newLocation);
 	}
 
@@ -522,7 +529,7 @@ public class FGProvider extends VisualGraphComponentProvider<FGVertex, FGEdge, F
 	}
 
 	/**
-	 * Tells this provider to refresh, which means to rebuild the graph and relayout the 
+	 * Tells this provider to refresh, which means to rebuild the graph and relayout the
 	 * vertices.
 	 */
 	private void refresh(boolean keepPerspective) {
@@ -537,7 +544,7 @@ public class FGProvider extends VisualGraphComponentProvider<FGVertex, FGEdge, F
 			Address address = function.getEntryPoint();
 			Address currentAddress = currentLocation.getAddress();
 			if (function.getBody().contains(currentAddress)) {
-				// prefer the current address if it is within the current function (i.e., the 
+				// prefer the current address if it is within the current function (i.e., the
 				// location hasn't changed out from under the graph due to threading issues)
 				address = currentAddress;
 			}
@@ -568,7 +575,7 @@ public class FGProvider extends VisualGraphComponentProvider<FGVertex, FGEdge, F
 	}
 
 	/**
-	 * Tells the graph that some display data may have changed, but the changes are not worth 
+	 * Tells the graph that some display data may have changed, but the changes are not worth
 	 * performing a full rebuild
 	 */
 	public void refreshDisplayWithoutRebuilding() {
@@ -597,7 +604,7 @@ public class FGProvider extends VisualGraphComponentProvider<FGVertex, FGEdge, F
 
 		//
 		// Note: since we are not looping and we are using 'else if's, order is important!
-		// 
+		//
 
 		if (ev.containsEvent(DomainObject.DO_OBJECT_RESTORED) ||
 			ev.containsEvent(ChangeManager.DOCR_FUNCTION_BODY_CHANGED)) {
@@ -716,7 +723,7 @@ public class FGProvider extends VisualGraphComponentProvider<FGVertex, FGEdge, F
 	}
 
 	private void handleReferenceRemoved(DomainObjectChangeRecord record) {
-		// 
+		//
 		// Get the affected vertex (if any).  Determine if we have to combine the vertex with
 		// the vertex below it (adding a reference creates a new basic block, which creates a new
 		// vertex--we may need to reverse that process)
@@ -730,7 +737,7 @@ public class FGProvider extends VisualGraphComponentProvider<FGVertex, FGEdge, F
 			return; // this particular removal doesn't affect our graph
 		}
 
-		// 
+		//
 		// How do we know if we can combine this vertex with its parent?  Well, we have some
 		// tests that must hold true:
 		// -There must be only a fallthrough edge to the affected vertex
@@ -774,7 +781,7 @@ public class FGProvider extends VisualGraphComponentProvider<FGVertex, FGEdge, F
 
 	private void handleReferenceAdded(DomainObjectChangeRecord record) {
 
-		// 
+		//
 		// Get the affected vertex (if any).  Determine if we have to split the vertex.
 		//
 		FGData functionGraphData = controller.getFunctionGraphData();
@@ -786,10 +793,10 @@ public class FGProvider extends VisualGraphComponentProvider<FGVertex, FGEdge, F
 			return; // this particular removal doesn't affect our graph
 		}
 
-		// 
+		//
 		// How do we know if we need to split this vertex?  Well, we have some
 		// tests that must hold true:
-		// -The 'to' address for the reference must not be to the minimum address for that vertex		
+		// -The 'to' address for the reference must not be to the minimum address for that vertex
 		//
 		AddressSetView addresses = destinationVertex.getAddresses();
 		Address minAddress = addresses.getMinAddress();
@@ -824,7 +831,7 @@ public class FGProvider extends VisualGraphComponentProvider<FGVertex, FGEdge, F
 	}
 
 	private void handleSymbolRemoved(DomainObjectChangeRecord record) {
-		// 
+		//
 		// Get the affected vertex (if any).  Determine if we have to combine the vertex with
 		// the vertex below it (adding a symbol creates a new basic block, which creates a new
 		// vertex--we may need to reverse that process)
@@ -837,13 +844,13 @@ public class FGProvider extends VisualGraphComponentProvider<FGVertex, FGEdge, F
 			return; // this particular removal doesn't affect our graph
 		}
 
-		// 
+		//
 		// How do we know if we can combine this vertex with its parent?  Well, we have some
 		// tests that must hold true:
 		// -There must be only a fallthrough edge to the affected vertex
 		// -The parent vertex must have only one flow--FallThrough
 		// -There must not be any other references to the entry of the vertex
-		// -There must not be any non-dynamic labels on the vertex		
+		// -There must not be any non-dynamic labels on the vertex
 		//
 		Graph<FGVertex, FGEdge> graph = functionGraph;
 		Collection<FGEdge> inEdgesForDestination = graph.getInEdges(destinationVertex);
@@ -895,7 +902,7 @@ public class FGProvider extends VisualGraphComponentProvider<FGVertex, FGEdge, F
 	}
 
 	private void handleSymbolAdded(DomainObjectChangeRecord record) {
-		// 
+		//
 		// Get the affected vertex (if any).  Determine if we have to split the vertex.
 		//
 		FGData functionGraphData = controller.getFunctionGraphData();
@@ -906,10 +913,10 @@ public class FGProvider extends VisualGraphComponentProvider<FGVertex, FGEdge, F
 			return; // this particular removal doesn't affect our graph
 		}
 
-		// 
+		//
 		// How do we know if we need to split this vertex?  Well, we have some
 		// tests that must hold true:
-		// -The address for the symbol must not be to the minimum address for that vertex		
+		// -The address for the symbol must not be to the minimum address for that vertex
 		//
 		AddressSetView addresses = destinationVertex.getAddresses();
 		Address minAddress = addresses.getMinAddress();
@@ -1155,7 +1162,7 @@ public class FGProvider extends VisualGraphComponentProvider<FGVertex, FGEdge, F
 			return null;
 		}
 
-		// we want to limit the selections we return here to that which is inside of our 
+		// we want to limit the selections we return here to that which is inside of our
 		// graph (the current selection of this provider is that for the entire program)
 		Function function = currentData.getFunction();
 		AddressSetView functionBody = function.getBody();
@@ -1174,7 +1181,7 @@ public class FGProvider extends VisualGraphComponentProvider<FGVertex, FGEdge, F
 			return null;
 		}
 
-		// we want to limit the selections we return here to that which is inside of our 
+		// we want to limit the selections we return here to that which is inside of our
 		// graph (the current selection of this provider is that for the entire program)
 		Function function = currentData.getFunction();
 		AddressSetView functionBody = function.getBody();