GP-5258 - Fixed a Symbol Tree bug that caused an empty tree when showing the tree with a filter applied

Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/symboltree/SymbolTreeProvider.java

@@ -150,10 +150,7 @@ public class SymbolTreeProvider extends ComponentProviderAdapter {
 
 	private SymbolGTree createTree(SymbolTreeRootNode rootNode) {
 		if (tree != null) {
-			GTreeNode oldRootNode = tree.getModelRoot();
 			tree.setRootNode(rootNode);
-
-			oldRootNode.removeAll();// assist in cleanup a bit
 			return tree;
 		}
 

Ghidra/Framework/Docking/src/main/java/docking/widgets/tree/CoreGTreeNode.java

@@ -253,6 +253,10 @@ abstract class CoreGTreeNode implements Cloneable {
 	}
 
 	public void dispose() {
+		disconnect(true);
+	}
+
+	void disconnect(boolean dispose) {
 		List<GTreeNode> oldChildren;
 		synchronized (this) {
 			oldChildren = children;
@@ -262,7 +266,10 @@ abstract class CoreGTreeNode implements Cloneable {
 
 		if (oldChildren != null) {
 			for (GTreeNode node : oldChildren) {
-				node.dispose();
+				node.disconnect(dispose);
+				if (dispose) {
+					node.dispose();
+				}
 			}
 			oldChildren.clear();
 		}
@@ -270,23 +277,13 @@ abstract class CoreGTreeNode implements Cloneable {
 
 	/**
 	 * This is used to dispose filtered "clone" nodes. When a filter is applied to the tree,
-	 * the nodes that matched are "shallow" cloned, so when the filter is removed, we don't
-	 * want to do a full dispose on the nodes, just clean up the parent-child references.
+	 * the nodes that matched are "shallow" cloned.  This is effectively a shallow dispose that will
+	 * clean up any children and disconnect this node from the true, but will *not* call dispose(),
+	 * which would affect the original node from which this node was cloned.
 	 */
-	final void disposeClones() {
-		List<GTreeNode> oldChildren;
-		synchronized (this) {
-			oldChildren = children;
-			children = null;
-			parent = null;
-		}
-
-		if (oldChildren != null) {
-			for (GTreeNode node : oldChildren) {
-				node.disposeClones();
-			}
-			oldChildren.clear();
-		}
+	final void disposeClone() {
+		// Do not dispose, as that will affect the clone's source too
+		disconnect(false);
 	}
 
 	/**

Ghidra/Framework/Docking/src/main/java/docking/widgets/tree/GTree.java

@@ -285,12 +285,12 @@ public class GTree extends JPanel implements BusyListener {
 			realModelRootNode.dispose();
 		}
 
-		// if there is a filter applied, clean up the filtered nodes. Note that filtered nodes
+		// If there is a filter applied, clean up the filtered nodes. Note that filtered nodes
 		// are expected to be shallow clones of the model nodes, so we don't want to call full
 		// dispose on the filtered nodes because internal clean-up should happen when the
-		// model nodes are disposed. The disposeClones just breaks the child-parent ties.
+		// model nodes are disposed. The disposeClone() just breaks the child-parent ties.
 		if (realViewRootNode != null && realViewRootNode != realModelRootNode) {
-			realViewRootNode.disposeClones();
+			realViewRootNode.disposeClone();
 		}
 
 		filterProvider.dispose();
@@ -847,23 +847,40 @@ public class GTree extends JPanel implements BusyListener {
 		Swing.runIfSwingOrRunLater(() -> {
 			worker.clearAllJobs();
 			rootNode.setParent(rootParent);
+			GTreeNode oldModelRoot = realModelRootNode;
+			GTreeNode oldViewRoot = realViewRootNode;
 			realModelRootNode = rootNode;
 			realViewRootNode = rootNode;
-			GTreeNode oldRoot;
-			oldRoot = swingSetModelRootNode(rootNode);
-			oldRoot.dispose();
+			swingSetModelRootNode(rootNode);
+
+			disposeOldRoots(oldModelRoot, oldViewRoot);
+
 			if (filter != null) {
 				filterUpdateManager.update();
 			}
 		});
 	}
 
+	private void disposeOldRoots(GTreeNode oldModelRoot, GTreeNode oldViewRoot) {
+		if (oldModelRoot != null && oldModelRoot != realModelRootNode) {
+			oldModelRoot.dispose();
+		}
+
+		// If there is a filter applied, clean up the filtered nodes. Note that filtered nodes
+		// are expected to be shallow clones of the model nodes, so we don't want to call full
+		// dispose on the filtered nodes because internal clean-up should happen when the
+		// model nodes are disposed. The disposeClone() just breaks the child-parent ties.
+		if (oldViewRoot != null && oldViewRoot != realViewRootNode) {
+			oldViewRoot.disposeClone(); // safe to call even if we disposed the same node above
+		}
+	}
+
 	void swingSetFilteredRootNode(GTreeNode filteredRootNode) {
 		filteredRootNode.setParent(rootParent);
 		realViewRootNode = filteredRootNode;
 		GTreeNode currentRoot = swingSetModelRootNode(filteredRootNode);
 		if (currentRoot != realModelRootNode) {
-			currentRoot.disposeClones();
+			currentRoot.disposeClone();
 		}
 	}
 
@@ -871,7 +888,7 @@ public class GTree extends JPanel implements BusyListener {
 		realViewRootNode = realModelRootNode;
 		GTreeNode currentRoot = swingSetModelRootNode(realModelRootNode);
 		if (currentRoot != realModelRootNode && currentRoot != null) {
-			currentRoot.disposeClones();
+			currentRoot.disposeClone();
 		}
 	}
 

Ghidra/Framework/Docking/src/main/java/docking/widgets/tree/GTreeRootParentNode.java

@@ -53,4 +53,9 @@ class GTreeRootParentNode extends GTreeNode {
 	public boolean isLeaf() {
 		return false;
 	}
+
+	@Override
+	public String toString() {
+		return "GTree Root Parent Node";
+	}
 }

Ghidra/Framework/Docking/src/main/java/docking/widgets/tree/internal/GTreeModel.java

@@ -125,20 +125,25 @@ public class GTreeModel implements TreeModel {
 				"GTreeModel.fireNodeStructuredChanged() must be " + "called from the AWT thread");
 
 			// If the tree is filtered and this is called on the original node, we have to
-			// translate the node to a view node (one the jtree knows).
+			// translate the node to a view node (one the tree knows).
 			GTreeNode viewNode = convertToViewNode(changedNode);
 			if (viewNode == null) {
 				return;
 			}
 
 			if (viewNode != changedNode) {
-				// This means we are filtered and since the original node's children are invalid,
-				// then the filtered children are invalid also. So clear out the children by
-				// setting an empty list as we don't want to trigger the node to regenerate its
-				// children which happens if you set the children to null.
+				// The only time this can happen is when the tree is filtered.  In this case, the
+				// view node will be a clone of the changed node.   We need to update the cloned 
+				// node to signal that the children have changed.  If we set the children to null,
+				// then they will get reloaded when we fire the event.  Instead, if we set the 
+				// children to the empty list, the node will simply think there are no children and
+				// it will not get reloaded.
 				//
-				// This won't cause a second event to the jtree because we are protected
-				// by the isFiringNodeStructureChanged variable
+				// After the events have been fired, there will eventually be a refilter operation 
+				// to update the view node with the correct children for the active filter.
+				//
+				// Note: this won't cause a second event to the tree because we are protected by 
+				// the isFiringNodeStructureChanged flag at the top of this method.
 				viewNode.setChildren(Collections.emptyList());
 			}