Merge remote-tracking branch 'origin/patch'

Ghidra/Debug/Debugger-api/src/main/java/ghidra/debug/api/target/Target.java

@@ -700,4 +700,15 @@ public interface Target {
 	 * @return true if busy
 	 */
 	boolean isBusy();
+
+	/**
+	 * Forcibly commit all of the back-ends transactions on this target's trace.
+	 * 
+	 * <p>
+	 * This is generally not a recommended course of action, except that sometimes the back-end
+	 * crashes and fails to close a transaction. It should only be invoked by a relatively hidden
+	 * menu option, and mediated by a warning of some sort. Closing a transaction prematurely, when
+	 * the back-end actually <em>does</em> still need it may cause a host of other problems.
+	 */
+	void forciblyCloseTransactions();
 }

Ghidra/Debug/Debugger-api/src/main/java/ghidra/debug/api/tracermi/TraceRmiConnection.java

@@ -171,8 +171,19 @@ public interface TraceRmiConnection extends AutoCloseable {
 	/**
 	 * Check if the given target has a transaction open
 	 * 
-	 * @param target
+	 * @param target the target
 	 * @return true if busy
 	 */
 	boolean isBusy(Target target);
+
+	/**
+	 * Forcibly commit all transactions this connection has on the given trace
+	 * 
+	 * <p>
+	 * This may cause undefined behavior in the back-end, especially if it still needs the
+	 * transaction.
+	 * 
+	 * @param target the the target
+	 */
+	void forciblyCloseTransactions(Target target);
 }

Ghidra/Debug/Debugger-rmi-trace/src/main/help/help/topics/TraceRmiConnectionManagerPlugin/TraceRmiConnectionManagerPlugin.html

@@ -105,5 +105,15 @@
 
     <P>Stop the server. This closes the persistent server. This does not affect pending acceptors
     or established connections.</P>
+
+    <H3><A name="forcibly_close_txes"></A>Forcibly Close Transactions</H3>
+
+    <P>Forcibly close all the back-end's transactions on the target trace. This is generally not a
+    recommended course of action, except that sometimes the back-end crashes and fails to close a
+    transaction. Un-closed transactions from the back-end can leave most, if not all, of the UI in
+    a stale state, since event processing on the trace is disabled. If there is good reason to
+    believe the back-end has forgotten to close a transaction, this action will forcibly close all
+    of them and re-enable event processing. If, however, the back-end was in fact still doing work
+    with that transaction, it may crash and/or corrupt the connection.</P>
   </BODY>
 </HTML>

Ghidra/Debug/Debugger-rmi-trace/src/main/java/ghidra/app/plugin/core/debug/gui/tracermi/connection/TraceRmiConnectionManagerProvider.java

@@ -161,6 +161,24 @@ public class TraceRmiConnectionManagerProvider extends ComponentProviderAdapter
 		}
 	}
 
+	interface ForceCloseTransactionsActions {
+		String NAME = "Forcibly Close Transactions";
+		String DESCRIPTION = "Forcibly commit all remote transactions on the trace";
+		String GROUP = GROUP_MAINTENANCE;
+		String HELP_ANCHOR = "forcibly_close_txes";
+
+		static ActionBuilder builder(Plugin owner) {
+			String ownerName = owner.getName();
+			return new ActionBuilder(NAME, ownerName)
+					.description(DESCRIPTION)
+					.menuPath(NAME)
+					.popupMenuPath(NAME)
+					.menuGroup(GROUP)
+					.popupMenuGroup(GROUP)
+					.helpLocation(new HelpLocation(ownerName, HELP_ANCHOR));
+		}
+	}
+
 	class InjectableGTree extends GTree {
 		public InjectableGTree(GTreeNode root) {
 			super(root);
@@ -200,6 +218,7 @@ public class TraceRmiConnectionManagerProvider extends ComponentProviderAdapter
 	DockingAction actionConnectOutbound;
 	DockingAction actionCloseConnection;
 	DockingAction actionCloseAll;
+	DockingAction actionForceCloseTransactions;
 
 	TraceRmiManagerActionContext myActionContext;
 
@@ -309,6 +328,12 @@ public class TraceRmiConnectionManagerProvider extends ComponentProviderAdapter
 				.enabledWhen(this::isActionCloseAllEnabled)
 				.onAction(this::doActionCloseAllActivated)
 				.buildAndInstallLocal(this);
+
+		actionForceCloseTransactions = ForceCloseTransactionsActions.builder(plugin)
+				.withContext(TraceRmiManagerActionContext.class)
+				.enabledWhen(this::isActionForceCloseTransactionsEnabled)
+				.onAction(this::doActionCloseTransactionsActivated)
+				.buildAndInstallLocal(this);
 	}
 
 	@Override
@@ -482,6 +507,21 @@ public class TraceRmiConnectionManagerProvider extends ComponentProviderAdapter
 		}
 	}
 
+	private boolean isActionForceCloseTransactionsEnabled(TraceRmiManagerActionContext context) {
+		TraceRmiManagerNode node = context.getSelectedNode();
+		if (node instanceof TraceRmiTargetNode) {
+			return true;
+		}
+		return false;
+	}
+
+	private void doActionCloseTransactionsActivated(TraceRmiManagerActionContext context) {
+		TraceRmiManagerNode node = context.getSelectedNode();
+		if (node instanceof TraceRmiTargetNode tNode) {
+			tNode.getTarget().forciblyCloseTransactions();
+		}
+	}
+
 	@AutoServiceConsumed
 	private void setTraceRmiService(TraceRmiService traceRmiService) {
 		if (this.traceRmiService != null) {

Ghidra/Debug/Debugger-rmi-trace/src/main/java/ghidra/app/plugin/core/debug/service/tracermi/TraceRmiHandler.java

@@ -950,6 +950,18 @@ public class TraceRmiHandler extends AbstractTraceRmiConnection {
 				.build();
 	}
 
+	protected void checkRestoreEvents(OpenTrace open) {
+		final boolean restoreEvents;
+		synchronized (openTxes) {
+			restoreEvents = openTxes.keySet()
+					.stream()
+					.noneMatch(id -> id.doId.equals(open.doId));
+		}
+		if (restoreEvents) {
+			open.trace.setEventsEnabled(true);
+		}
+	}
+
 	protected ReplyEndTx handleEndTx(RequestEndTx req) {
 		OpenTx tx;
 		synchronized (openTxes) {
@@ -973,16 +985,8 @@ public class TraceRmiHandler extends AbstractTraceRmiConnection {
 		}
 
 		tx.tx.close();
+		checkRestoreEvents(open);
 
-		final boolean restoreEvents;
-		synchronized (openTxes) {
-			restoreEvents = openTxes.keySet()
-					.stream()
-					.noneMatch(id -> id.doId.domObjId == req.getOid().getId());
-		}
-		if (restoreEvents) {
-			open.trace.setEventsEnabled(true);
-		}
 		Swing.runLater(
 			() -> plugin.listeners.invoke().transactionClosed(this, open.target, req.getAbort()));
 		return ReplyEndTx.getDefaultInstance();
@@ -1329,7 +1333,9 @@ public class TraceRmiHandler extends AbstractTraceRmiConnection {
 
 	@Override
 	public boolean isBusy() {
-		return !openTxes.isEmpty();
+		synchronized (openTxes) {
+			return !openTxes.isEmpty();
+		}
 	}
 
 	@Override
@@ -1339,11 +1345,33 @@ public class TraceRmiHandler extends AbstractTraceRmiConnection {
 			return false;
 		}
 
-		for (Tid tid : openTxes.keySet()) {
+		synchronized (openTxes) {
-			if (Objects.equals(openTrace.doId, tid.doId)) {
+			for (Tid tid : openTxes.keySet()) {
-				return true;
+				if (Objects.equals(openTrace.doId, tid.doId)) {
+					return true;
+				}
 			}
 		}
 		return false;
 	}
+
+	@Override
+	public void forciblyCloseTransactions(Target target) {
+		OpenTrace open = openTraces.getByTrace(target.getTrace());
+		if (open == null || open.target != target) {
+			return;
+		}
+		synchronized (openTxes) {
+			for (OpenTx tx : List.copyOf(openTxes.values())) {
+				if (Objects.equals(open.doId, tx.txId.doId)) {
+					openTxes.remove(tx.txId);
+					tx.tx.commit();
+					tx.tx.close();
+					Swing.runLater(
+						() -> plugin.listeners.invoke().transactionClosed(this, target, false));
+				}
+			}
+		}
+		open.trace.setEventsEnabled(true);
+	}
 }

Ghidra/Debug/Debugger-rmi-trace/src/main/java/ghidra/app/plugin/core/debug/service/tracermi/TraceRmiTarget.java

@@ -1716,4 +1716,9 @@ public class TraceRmiTarget extends AbstractTarget {
 	public boolean isBusy() {
 		return connection.isBusy(this);
 	}
+
+	@Override
+	public void forciblyCloseTransactions() {
+		connection.forciblyCloseTransactions(this);
+	}
 }

Ghidra/Debug/Debugger-rmi-trace/src/test/java/ghidra/app/plugin/core/debug/service/tracermi/TestTraceRmiConnection.java

@@ -277,4 +277,8 @@ public abstract class TestTraceRmiConnection extends AbstractTraceRmiConnection
 	public boolean isBusy(Target target) {
 		return false;
 	}
+
+	@Override
+	public void forciblyCloseTransactions(Target target) {
+	}
 }

Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/gui/control/TargetActionTask.java

@@ -136,6 +136,16 @@ public class TargetActionTask extends Task {
 	/**
 	 * Execute an {@link ActionEntry}
 	 * 
+	 * <p>
+	 * If the {@link ProgressService} is available, we will not enforce a timeout, because it should
+	 * be relatively easy for the user to manage the pending tasks. Otherwise, we'll enforce the
+	 * timeout. The rationale here is that some tasks do actually take a good bit of time. For
+	 * example, some targets just have a large module list. Often a GUI component is asking for a
+	 * reason, and if we time it out, that thing doesn't get what it needs. Furthermore, the entry
+	 * disappears from the task list, even though the back-end is likely still working on it. That's
+	 * not good, actually. Since we have a cancel button, let the user decide when it's had enough
+	 * time.
+	 * 
 	 * @param tool the tool in which to execute
 	 * @param title the title, often {@link ActionEntry#display()}
 	 * @param entry the action to execute
@@ -144,11 +154,13 @@ public class TargetActionTask extends Task {
 	 */
 	public static CompletableFuture<Void> runAction(PluginTool tool, String title,
 			ActionEntry entry) {
-		return executeTask(tool, new TargetActionTask(tool, title, entry));
+		return executeTask(tool, new TargetActionTask(tool, title, entry,
+			tool.getService(ProgressService.class) == null));
 	}
 
 	private final PluginTool tool;
 	private final ActionEntry entry;
+	private final boolean timeout;
 
 	/**
 	 * Construct a task fore the given action
@@ -156,17 +168,24 @@ public class TargetActionTask extends Task {
 	 * @param tool the plugin tool
 	 * @param title the title, often {@link ActionEntry#display()}
 	 * @param entry the action to execute
+	 * @param timeout whether or not to enforce the timeout
 	 */
-	public TargetActionTask(PluginTool tool, String title, ActionEntry entry) {
+	public TargetActionTask(PluginTool tool, String title, ActionEntry entry, boolean timeout) {
 		super(title, false, false, false);
 		this.tool = tool;
 		this.entry = entry;
+		this.timeout = timeout;
 	}
 
 	@Override
 	public void run(TaskMonitor monitor) throws CancelledException {
 		try {
-			entry.run(entry.requiresPrompt());
+			if (timeout) {
+				entry.run(entry.requiresPrompt());
+			}
+			else {
+				entry.invokeAsyncWithoutTimeout(entry.requiresPrompt()).get();
+			}
 		}
 		catch (Throwable e) {
 			reportError(e);

Ghidra/Debug/Debugger/src/test/java/ghidra/app/plugin/core/debug/service/MockTarget.java

@@ -277,4 +277,8 @@ public class MockTarget implements Target {
 	public boolean isBusy() {
 		return false;
 	}
+
+	@Override
+	public void forciblyCloseTransactions() {
+	}
 }