Merge remote-tracking branch 'origin/GT-3599-dragonmacher-analysis-rollback-on-exception'

Ghidra/Features/Base/src/test.slow/java/ghidra/framework/data/PairedTransactionTest.java

@@ -22,7 +22,6 @@ import java.util.*;
 
 import org.junit.*;
 
-import db.DBHandle;
 import generic.test.AbstractGenericTest;
 import ghidra.framework.model.*;
 import ghidra.framework.options.Options;
@@ -82,23 +81,6 @@ public class PairedTransactionTest extends AbstractGenericTest {
 		}
 	}
 
-	class DummyDomainObject extends DomainObjectAdapterDB {
-
-		protected DummyDomainObject(String name, Object consumer) throws IOException {
-			super(new DBHandle(), name, 10, 1, consumer);
-		}
-
-		@Override
-		public String getDescription() {
-			return "Test object: " + testName.getMethodName();
-		}
-
-		@Override
-		public boolean isChangeable() {
-			return true;
-		}
-	}
-
 	private static String START = "Start";
 	private static String END = "End";
 	private static String UNDO_STATE_CHANGE1 = "UndoRedo1";
@@ -106,7 +88,7 @@ public class PairedTransactionTest extends AbstractGenericTest {
 
 	class MyListener implements TransactionListener {
 
-		private List<String> events = new ArrayList<String>();
+		private List<String> events = new ArrayList<>();
 		private Transaction lastTransaction;
 		private DomainObjectAdapterDB obj;
 
@@ -122,7 +104,8 @@ public class PairedTransactionTest extends AbstractGenericTest {
 		}
 
 		@Override
-		public synchronized void transactionStarted(DomainObjectAdapterDB domainObj, Transaction tx) {
+		public synchronized void transactionStarted(DomainObjectAdapterDB domainObj,
+				Transaction tx) {
 			assertEquals(obj, domainObj);
 			events.add(START);
 			lastTransaction = tx;
@@ -176,8 +159,8 @@ public class PairedTransactionTest extends AbstractGenericTest {
 
 		assertTrue(obj1.canUndo());
 		assertTrue(obj2.canUndo());
-		assertTrue(!obj1.canRedo());
+		assertFalse(obj1.canRedo());
-		assertTrue(!obj2.canRedo());
+		assertFalse(obj2.canRedo());
 
 		Transaction tx = obj1Listener.getLastTransaction();
 		obj1Listener.getEvents();
@@ -204,15 +187,15 @@ public class PairedTransactionTest extends AbstractGenericTest {
 		assertEquals(obj1, synchronizedDomainObjects[0]);
 		assertEquals(obj2, synchronizedDomainObjects[1]);
 
-		assertEquals(synchronizedDomainObjects, obj2.getSynchronizedDomainObjects());
+		assertArrayEquals(synchronizedDomainObjects, obj2.getSynchronizedDomainObjects());
 
 		assertEquals(0, obj1.getUndoStackDepth());
 		assertEquals(0, obj2.getUndoStackDepth());
 
-		assertTrue(!obj1.canUndo());
+		assertFalse(obj1.canUndo());
-		assertTrue(!obj2.canUndo());
+		assertFalse(obj2.canUndo());
-		assertTrue(!obj1.canRedo());
+		assertFalse(obj1.canRedo());
-		assertTrue(!obj2.canRedo());
+		assertFalse(obj2.canRedo());
 
 		String[] events1 = obj1Listener.getEvents();
 		assertEquals(UNDO_STATE_CHANGE1, events1[events1.length - 1]);
@@ -269,10 +252,10 @@ public class PairedTransactionTest extends AbstractGenericTest {
 		assertEquals(0, obj1.getUndoStackDepth());
 		assertEquals(0, obj2.getUndoStackDepth());
 
-		assertTrue(!obj1.canUndo());
+		assertFalse(obj1.canUndo());
-		assertTrue(!obj2.canUndo());
+		assertFalse(obj2.canUndo());
-		assertTrue(!obj1.canRedo());
+		assertFalse(obj1.canRedo());
-		assertTrue(!obj2.canRedo());
+		assertFalse(obj2.canRedo());
 
 		events1 = obj1Listener.getEvents();
 		events2 = obj2Listener.getEvents();
@@ -329,8 +312,8 @@ public class PairedTransactionTest extends AbstractGenericTest {
 
 		assertTrue(obj1.canUndo());
 		assertTrue(obj2.canUndo());
-		assertTrue(!obj1.canRedo());
+		assertFalse(obj1.canRedo());
-		assertTrue(!obj2.canRedo());
+		assertFalse(obj2.canRedo());
 
 		events1 = obj1Listener.getEvents();
 		events2 = obj2Listener.getEvents();
@@ -344,8 +327,8 @@ public class PairedTransactionTest extends AbstractGenericTest {
 
 		obj1.undo();
 
-		assertTrue(!obj1.canUndo());
+		assertFalse(obj1.canUndo());
-		assertTrue(!obj2.canUndo());
+		assertFalse(obj2.canUndo());
 		assertTrue(obj1.canRedo());
 		assertTrue(obj2.canRedo());
 
@@ -366,8 +349,8 @@ public class PairedTransactionTest extends AbstractGenericTest {
 
 		assertTrue(obj1.canUndo());
 		assertTrue(obj2.canUndo());
-		assertTrue(!obj1.canRedo());
+		assertFalse(obj1.canRedo());
-		assertTrue(!obj2.canRedo());
+		assertFalse(obj2.canRedo());
 
 		events1 = obj1Listener.getEvents();
 		events2 = obj2Listener.getEvents();
@@ -388,10 +371,10 @@ public class PairedTransactionTest extends AbstractGenericTest {
 		assertEquals(0, obj1.getUndoStackDepth());
 		assertEquals(0, obj2.getUndoStackDepth());
 
-		assertTrue(!obj1.canUndo());
+		assertFalse(obj1.canUndo());
-		assertTrue(!obj2.canUndo());
+		assertFalse(obj2.canUndo());
-		assertTrue(!obj1.canRedo());
+		assertFalse(obj1.canRedo());
-		assertTrue(!obj2.canRedo());
+		assertFalse(obj2.canRedo());
 
 		events1 = obj1Listener.getEvents();
 		events2 = obj2Listener.getEvents();
@@ -449,10 +432,10 @@ public class PairedTransactionTest extends AbstractGenericTest {
 		assertEquals(0, obj1.getUndoStackDepth());
 		assertEquals(1, obj2.getUndoStackDepth());
 
-		assertTrue(!obj1.canUndo());
+		assertFalse(obj1.canUndo());
 		assertTrue(obj2.canUndo());
-		assertTrue(!obj1.canRedo());
+		assertFalse(obj1.canRedo());
-		assertTrue(!obj2.canRedo());
+		assertFalse(obj2.canRedo());
 
 		events1 = obj1Listener.getEvents();
 		events2 = obj2Listener.getEvents();

Ghidra/Features/Base/src/test/java/ghidra/framework/plugintool/mgr/BackgroundCommandTaskTest.java

@@ -0,0 +1,184 @@
+/* ###
+ * IP: GHIDRA
+ *
+ * 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.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package ghidra.framework.plugintool.mgr;
+
+import static org.junit.Assert.*;
+
+import java.io.IOException;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import generic.test.AbstractGenericTest;
+import ghidra.framework.cmd.BackgroundCommand;
+import ghidra.framework.data.DummyDomainObject;
+import ghidra.framework.model.*;
+import ghidra.test.DummyTool;
+import ghidra.util.UniversalIdGenerator;
+import ghidra.util.exception.RollbackException;
+import ghidra.util.task.TaskMonitor;
+
+public class BackgroundCommandTaskTest extends AbstractGenericTest {
+
+	@Before
+	public void setUp() {
+		UniversalIdGenerator.initialize();
+	}
+
+	@Test
+	public void testSuccessfulCommand() throws Exception {
+
+		DummyTool tool = new DummyTool();
+		ToolTaskManager taskManager = new ToolTaskManager(tool);
+		SpyDomainObject domainObject = new SpyDomainObject(this);
+		SuccessfulDummyCommand cmd = new SuccessfulDummyCommand();
+		taskManager.executeCommand(cmd, domainObject);
+
+		waitFor(taskManager);
+		assertTrue(domainObject.wasCommitted());
+	}
+
+	@Test
+	public void testExceptionalCommand_NonRollbackException() throws Exception {
+
+		DummyTool tool = new DummyTool();
+		ToolTaskManager taskManager = new ToolTaskManager(tool);
+		SpyDomainObject domainObject = new SpyDomainObject(this);
+		NullPointerExceptionCommand cmd = new NullPointerExceptionCommand();
+
+		setErrorsExpected(true);
+		taskManager.executeCommand(cmd, domainObject);
+		waitFor(taskManager);
+		setErrorsExpected(false);
+
+		assertTrue(domainObject.wasCommitted());
+	}
+
+	@Test
+	public void testExceptionalCommand_RollbackException() throws Exception {
+
+		DummyTool tool = new DummyTool();
+		ToolTaskManager taskManager = new ToolTaskManager(tool);
+		SpyDomainObject domainObject = new SpyDomainObject(this);
+		RollbackExceptionCommand cmd = new RollbackExceptionCommand();
+
+		setErrorsExpected(true);
+		taskManager.executeCommand(cmd, domainObject);
+		waitFor(taskManager);
+		setErrorsExpected(false);
+
+		assertFalse(domainObject.wasCommitted());
+	}
+
+	@Test
+	public void testExceptionalCommand_DomainObjectLockedException() throws Exception {
+
+		DummyTool tool = new DummyTool();
+		ToolTaskManager taskManager = new ToolTaskManager(tool);
+		SpyDomainObject domainObject = new SpyDomainObject(this);
+		DomainObjectLockedExceptionCommand cmd = new DomainObjectLockedExceptionCommand();
+
+		setErrorsExpected(true);
+		taskManager.executeCommand(cmd, domainObject);
+		waitFor(taskManager);
+		setErrorsExpected(false);
+
+		assertFalse(domainObject.wasCommitted());
+	}
+
+	private void waitFor(ToolTaskManager taskManager) {
+		waitFor(() -> !taskManager.isBusy());
+	}
+
+	private class SpyDomainObject extends DummyDomainObject {
+
+		private static final int ID = 1;
+		private boolean transactionCommited;
+
+		protected SpyDomainObject(Object consumer) throws IOException {
+			super(consumer);
+		}
+
+		@Override
+		public int startTransaction(String description) {
+			return startTransaction(description, null);
+		}
+
+		@Override
+		public int startTransaction(String description, AbortedTransactionListener listener) {
+			return ID;
+		}
+
+		@Override
+		public void endTransaction(int transactionID, boolean commit) {
+
+			assertEquals(ID, transactionID);
+			transactionCommited = commit;
+		}
+
+		boolean wasCommitted() {
+			return transactionCommited;
+		}
+	}
+
+	private class SuccessfulDummyCommand extends BackgroundCommand {
+
+		SuccessfulDummyCommand() {
+			super("Dummy", true, true, false);
+		}
+
+		@Override
+		public boolean applyTo(DomainObject obj, TaskMonitor monitor) {
+			return true;
+		}
+
+	}
+
+	private class NullPointerExceptionCommand extends BackgroundCommand {
+
+		NullPointerExceptionCommand() {
+			super("Dummy", true, true, false);
+		}
+
+		@Override
+		public boolean applyTo(DomainObject obj, TaskMonitor monitor) {
+			throw new NullPointerException();
+		}
+
+	}
+
+	private class RollbackExceptionCommand extends BackgroundCommand {
+		RollbackExceptionCommand() {
+			super("Dummy", true, true, false);
+		}
+
+		@Override
+		public boolean applyTo(DomainObject obj, TaskMonitor monitor) {
+			throw new RollbackException("This is a rollback exception");
+		}
+	}
+
+	private class DomainObjectLockedExceptionCommand extends BackgroundCommand {
+		DomainObjectLockedExceptionCommand() {
+			super("Dummy", true, true, false);
+		}
+
+		@Override
+		public boolean applyTo(DomainObject obj, TaskMonitor monitor) {
+			throw new DomainObjectLockedException("Unable to connect");
+		}
+	}
+}

Ghidra/Features/VersionTracking/src/test/java/ghidra/feature/vt/gui/filters/TagFilterTest.java

@@ -15,15 +15,13 @@
  */
 package ghidra.feature.vt.gui.filters;
 
-import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.*;
-import static org.junit.Assert.assertTrue;
 
 import java.io.IOException;
 import java.util.*;
 
 import org.junit.*;
 
-import db.DBHandle;
 import ghidra.feature.vt.api.db.VTSessionDB;
 import ghidra.feature.vt.api.impl.VTChangeManager;
 import ghidra.feature.vt.api.impl.VersionTrackingChangeRecord;
@@ -32,7 +30,7 @@ import ghidra.feature.vt.db.DummyTestProgramCorrelator;
 import ghidra.feature.vt.db.VTBaseTestCase;
 import ghidra.feature.vt.gui.plugin.VTController;
 import ghidra.feature.vt.gui.plugin.VTControllerListener;
-import ghidra.framework.data.DomainObjectAdapterDB;
+import ghidra.framework.data.DummyDomainObject;
 import ghidra.framework.model.*;
 import ghidra.framework.plugintool.ServiceProvider;
 import ghidra.program.model.listing.Program;
@@ -433,23 +431,6 @@ public class TagFilterTest extends VTBaseTestCase {
 		}
 	}
 
-	class DummyDomainObject extends DomainObjectAdapterDB {
-
-		protected DummyDomainObject(Object consumer) throws IOException {
-			super(new DBHandle(), "Dummy", 10, 1, consumer);
-		}
-
-		@Override
-		public String getDescription() {
-			return "Test object: " + getName();
-		}
-
-		@Override
-		public boolean isChangeable() {
-			return true;
-		}
-	}
-
 	public static VTProgramCorrelator createProgramCorrelator(ServiceProvider serviceProvider,
 			Program sourceProgram, Program destinationProgram) {
 		return new DummyTestProgramCorrelator(serviceProvider, sourceProgram, destinationProgram);

Ghidra/Framework/Project/src/main/java/ghidra/framework/plugintool/mgr/BackgroundCommandTask.java

@@ -56,6 +56,7 @@ class BackgroundCommandTask extends Task implements AbortedTransactionListener {
 
 	/**
 	 * Returns the Domain Object associated with this Task
+	 * @return the object
 	 */
 	public UndoableDomainObject getDomainObject() {
 		return obj;
@@ -96,13 +97,12 @@ class BackgroundCommandTask extends Task implements AbortedTransactionListener {
 		}
 
 		boolean success = false;
-		boolean error = true;
+		boolean commit = true;
 		try {
 			success = cmd.applyTo(obj, monitor);
 			if (success) {
 				taskMgr.taskCompleted(obj, this, monitor);
 			}
-			error = false;
 		}
 		catch (Throwable t) {
 			synchronized (taskMgr) {
@@ -114,26 +114,28 @@ class BackgroundCommandTask extends Task implements AbortedTransactionListener {
 				t = t.getCause();
 			}
 
-			if (ignoreException(t)) {
+			commit = shouldKeepData(t);
+
+			if (isUnrecoverableException(t)) {
 				monitor.cancel();
 				taskMgr.clearTasks(obj);
 				return;
 			}
 			else if (!(t instanceof RollbackException)) {
-				Msg.showError(this, null, "Command Failure",
+				String message =
-					"An unexpected error occurred while processing the command: " + cmd.getName(),
+					"An unexpected error occurred while processing the command: " + cmd.getName();
-					t);
+				Msg.showError(this, null, "Command Failure", message, t);
 			}
 		}
 		finally {
 			TaskUtilities.removeTrackedTask(this);
 			try {
-				obj.endTransaction(id, !error);
+				obj.endTransaction(id, commit);
 			}
 			catch (DomainObjectException e) {
 				Throwable cause = e.getCause();
-				if (!error && !(cause instanceof ClosedException)) {
+				if (commit && !(cause instanceof ClosedException)) {
-					Msg.showError(this, null, null, null, cause);
+					Msg.error(this, "Transaction error", cause);
 					success = false;
 				}
 			}
@@ -144,7 +146,13 @@ class BackgroundCommandTask extends Task implements AbortedTransactionListener {
 		}
 	}
 
-	private boolean ignoreException(Throwable t) {
+	private boolean shouldKeepData(Throwable t) {
+		// unrecoverable exceptions are really bad; rollback exceptions signal to abort
+		boolean reallyBad = isUnrecoverableException(t) || t instanceof RollbackException;
+		return !reallyBad;
+	}
+
+	private boolean isUnrecoverableException(Throwable t) {
 
 		//@formatter:off
 		return t instanceof ConnectException ||

Ghidra/Framework/Project/src/test/java/ghidra/framework/data/DummyDomainObject.java

@@ -0,0 +1,44 @@
+/* ###
+ * IP: GHIDRA
+ *
+ * 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.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package ghidra.framework.data;
+
+import java.io.IOException;
+
+import db.DBHandle;
+
+/**
+ * Simple dummy version of DomainObjectAdapterDB
+ */
+public class DummyDomainObject extends DomainObjectAdapterDB {
+
+	public DummyDomainObject(Object consumer) throws IOException {
+		this("Dummy", consumer);
+	}
+
+	public DummyDomainObject(String name, Object consumer) throws IOException {
+		super(new DBHandle(), name, 10, 1, consumer);
+	}
+
+	@Override
+	public String getDescription() {
+		return "Test object: " + getName();
+	}
+
+	@Override
+	public boolean isChangeable() {
+		return true;
+	}
+}

Ghidra/Framework/SoftwareModeling/src/test/java/ghidra/test/DummyTool.java

@@ -20,7 +20,7 @@ import java.beans.PropertyChangeListener;
 import java.util.Collections;
 import java.util.Set;
 
-import javax.swing.ImageIcon;
+import javax.swing.*;
 import javax.swing.event.ChangeListener;
 
 import org.jdom.Element;
@@ -436,4 +436,19 @@ public class DummyTool extends PluginTool {
 	public PluginClassManager getPluginClassManager() {
 		return null;
 	}
+
+	@Override
+	public void addStatusComponent(JComponent c, boolean addBorder, boolean rightSide) {
+		//do nothing
+	}
+
+	@Override
+	public void removeStatusComponent(JComponent c) {
+		//do nothing
+	}
+
+	@Override
+	public JFrame getToolFrame() {
+		return null;
+	}
 }