Merge remote-tracking branch 'origin/GP-6392_Dan_fixSaveOnCloseDeadlock-take4--SQUASHED' into patch

Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/tracemgr/DebuggerTraceManagerServicePlugin.java

@@ -634,7 +634,7 @@ public class DebuggerTraceManagerServicePlugin extends Plugin
 	}
 
 	@Override
-	public synchronized Collection<Trace> getOpenTraces() {
+	public Collection<Trace> getOpenTraces() {
 		synchronized (listenersByTrace) {
 			return Set.copyOf(tracesView);
 		}

Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/DBTrace.java

@@ -15,7 +15,6 @@
  */
 package ghidra.trace.database;
 
-import java.io.File;
 import java.io.IOException;
 import java.util.*;
 import java.util.function.Consumer;
@@ -876,16 +875,10 @@ public class DBTrace extends DBCachedDomainObjectAdapter implements Trace, Trace
 	}
 
 	@Override
-	public void save(String comment, TaskMonitor monitor) throws IOException, CancelledException {
+	protected void prepareToSave() {
-		objectManager.flushWbCaches();
+		try (Transaction tx = openForcedTransaction("flush for save")) {
-		super.save(comment, monitor);
+			objectManager.flushWbCaches();
-	}
+		}
-
-	@Override
-	public void saveToPackedFile(File outputFile, TaskMonitor monitor)
-			throws IOException, CancelledException {
-		objectManager.flushWbCaches();
-		super.saveToPackedFile(outputFile, monitor);
 	}
 
 	public boolean isClosing() {
@@ -895,9 +888,9 @@ public class DBTrace extends DBCachedDomainObjectAdapter implements Trace, Trace
 	@Override
 	protected void close() {
 		closing = true;
-		objectManager.flushWbCaches();
+		// NOTE: Any unsaved changes in the write-back cache are unrecoverable
-		super.close();
 		objectManager.waitWbWorkers();
+		super.close();
 	}
 
 	@Override

Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/DBTraceContentHandler.java

@@ -41,16 +41,16 @@ public class DBTraceContentHandler extends DBWithUserDataContentHandler<DBTrace>
 	static final Class<DBTrace> TRACE_DOMAIN_OBJECT_CLASS = DBTrace.class;
 	static final String TRACE_CONTENT_DEFAULT_TOOL = "Debugger";
 
-	private static final DBTraceLinkContentHandler linkHandler = new DBTraceLinkContentHandler();
+	private static final DBTraceLinkContentHandler LINK_HANDLER = new DBTraceLinkContentHandler();
 
 	@Override
 	public long createFile(FileSystem fs, FileSystem userfs, String path, String name,
 			DomainObject obj, TaskMonitor monitor)
 			throws IOException, InvalidNameException, CancelledException {
-		if (!(obj instanceof DBTrace)) {
+		if (!(obj instanceof DBTrace trace)) {
 			throw new IOException("Unsupported domain object: " + obj.getClass().getName());
 		}
-		return createFile((DBTrace) obj, TRACE_CONTENT_TYPE, fs, path, name, monitor);
+		return createFile(trace, TRACE_CONTENT_TYPE, fs, path, name, monitor);
 	}
 
 	@Override
@@ -340,6 +340,6 @@ public class DBTraceContentHandler extends DBWithUserDataContentHandler<DBTrace>
 
 	@Override
 	public DBTraceLinkContentHandler getLinkHandler() {
-		return linkHandler;
+		return LINK_HANDLER;
 	}
 }

Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/target/DBTraceObjectValueWriteBehindCache.java

@@ -40,8 +40,9 @@ class DBTraceObjectValueWriteBehindCache {
 	private final AsyncReference<Boolean, Void> busy = new AsyncReference<>(false);
 	private volatile boolean flushing = false;
 
-	private final Map<DBTraceObject, Map<String, NavigableMap<Long, DBTraceObjectValueBehind>>> cachedValues =
+	private final Map<DBTraceObject,
-		new HashMap<>();
+		Map<String, NavigableMap<Long, DBTraceObjectValueBehind>>> cachedValues =
+			new HashMap<>();
 
 	public DBTraceObjectValueWriteBehindCache(DBTraceObjectManager manager) {
 		this.manager = manager;
@@ -51,7 +52,7 @@ class DBTraceObjectValueWriteBehindCache {
 	}
 
 	private void workLoop() {
-		while (!manager.trace.isClosed()) {
+		while (!manager.trace.isClosing()) {
 			try {
 				synchronized (cachedValues) {
 					if (cachedValues.isEmpty()) {
@@ -69,7 +70,7 @@ class DBTraceObjectValueWriteBehindCache {
 						cachedValues.wait(left);
 					}
 				}
-				if (manager.trace.isClosed()) {
+				if (manager.trace.isClosing()) {
 					break;
 				}
 				writeBatch();

Ghidra/Features/Base/src/test.slow/java/ghidra/framework/data/TransactionLockingTest.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/Framework/Project/src/main/java/ghidra/framework/data/AbstractTransactionManager.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.
@@ -55,31 +55,36 @@ abstract class AbstractTransactionManager {
 
 		checkLockingTask();
 
+		boolean doPrepare = false;
 		synchronized (this) {
 			if (getCurrentTransactionInfo() != null && !transactionTerminated) {
 				return false;
 			}
 			if (lockCount == 0) {
-				for (DomainObjectAdapterDB domainObj : getDomainObjects()) {
+				doPrepare = true;
-					if (domainObj.isChanged()) {
-						domainObj.prepareToSave();
-					}
-				}
 			}
 			lockReason = reason;
 			++lockCount;
-			return true;
 		}
+		if (doPrepare) {
+			for (DomainObjectAdapterDB domainObj : getDomainObjects()) {
+				if (domainObj.isChanged()) {
+					domainObj.prepareToSave();
+				}
+			}
+		}
+		return true;
 	}
 
 	/**
-	 * Attempt to obtain a modification lock on the domain object when generating a
+	 * Attempt to obtain a modification lock on the domain object when generating a background
-	 * background snapshot.
+	 * snapshot.
+	 * 
 	 * @param domainObj domain object corresponding to snapshot
 	 * @param hasProgress true if monitor has progress indicator
 	 * @param title title to be used for monitor
 	 * @return monitor object if lock obtained successfully, else null which indicates that a
-	 * modification is in process.
+	 *         modification is in process.
 	 */
 	final synchronized LockingTaskMonitor lockForSnapshot(DomainObjectAdapterDB domainObj,
 			boolean hasProgress, String title) {
@@ -106,9 +111,10 @@ abstract class AbstractTransactionManager {
 
 	/**
 	 * Force transaction lock and terminate current transaction.
+	 * 
 	 * @param rollback true if rollback of non-commited changes should occurs, false if commit
-	 * should be done.  NOTE: it can be potentially detrimental to commit an incomplete transaction
+	 *            should be done. NOTE: it can be potentially detrimental to commit an incomplete
-	 * and should be avoided.
+	 *            transaction and should be avoided.
 	 * @param reason very short reason for requesting lock
 	 */
 	final void forceLock(boolean rollback, String reason) {
@@ -131,9 +137,10 @@ abstract class AbstractTransactionManager {
 
 	/**
 	 * Terminate current transaction.
+	 * 
 	 * @param rollback true if rollback of non-commited changes should occurs, false if commit
-	 * should be done.  NOTE: it can be potentially detrimental to commit an incomplete transaction
+	 *            should be done. NOTE: it can be potentially detrimental to commit an incomplete
-	 * and should be avoided.
+	 *            transaction and should be avoided.
 	 * @param notify true for listeners to be notified else false
 	 */
 	abstract void terminateTransaction(boolean rollback, boolean notify);
@@ -159,8 +166,7 @@ abstract class AbstractTransactionManager {
 	}
 
 	/**
-	 * Block on active locking task.
+	 * Block on active locking task. Do not invoke this method from within a synchronized block.
-	 * Do not invoke this method from within a synchronized block.
 	 */
 	final void checkLockingTask() {
 		synchronized (this) {
@@ -173,6 +179,7 @@ abstract class AbstractTransactionManager {
 
 	/**
 	 * Throw lock exception if currently locked
+	 * 
 	 * @throws DomainObjectLockedException if currently locked
 	 */
 	final void verifyNoLock() throws DomainObjectLockedException {
@@ -194,7 +201,7 @@ abstract class AbstractTransactionManager {
 		}
 	}
 
-	final int startTransaction(DomainObjectAdapterDB object, String description,
+	final int startTransactionChecked(DomainObjectAdapterDB object, String description,
 			AbortedTransactionListener listener, boolean notify)
 			throws TerminatedTransactionException {
 
@@ -222,47 +229,53 @@ abstract class AbstractTransactionManager {
 			boolean commit, boolean notify) throws IllegalStateException;
 
 	/**
-	 * Returns the undo stack depth.
+	 * Returns the undo stack depth. (The number of items on the undo stack) This method is for
-	 * (The number of items on the undo stack)
+	 * JUnits.
-	 * This method is for JUnits.
+	 * 
 	 * @return the undo stack depth
 	 */
 	abstract int getUndoStackDepth();
 
 	/**
 	 * Returns true if there is at least one redo transaction to be redone.
+	 * 
 	 * @return true if there is at least one redo transaction to be redone
 	 */
 	abstract boolean canRedo();
 
 	/**
 	 * Returns true if there is at least one undo transaction to be undone.
+	 * 
 	 * @return true if there is at least one undo transaction to be undone
 	 */
 	abstract boolean canUndo();
 
 	/**
 	 * Returns the name of the next undo transaction (The most recent change).
+	 * 
 	 * @return the name of the next undo transaction (The most recent change)
 	 */
 	abstract String getRedoName();
 
 	/**
 	 * Returns the name of the next redo transaction (The most recent undo).
+	 * 
 	 * @return the name of the next redo transaction (The most recent undo)
 	 */
 	abstract String getUndoName();
 
 	/**
-	 * Returns the names of all undoable transactions in reverse chronological order. In other
+	 * Returns the names of all undoable transactions in reverse chronological order. In other words
-	 * words the transaction at the top of the list must be undone first.
+	 * the transaction at the top of the list must be undone first.
+	 * 
 	 * @return the names of all undoable transactions in reverse chronological order
 	 */
 	abstract List<String> getAllUndoNames();
 
 	/**
-	 * Returns the names of all redoable transactions in chronological order. In other words
+	 * Returns the names of all redoable transactions in chronological order. In other words the
-	 * the transaction at the top of the list must be redone first.
+	 * transaction at the top of the list must be redone first.
+	 * 
 	 * @return the names of all redoable transactions in chronological order
 	 */
 	abstract List<String> getAllRedoNames();
@@ -321,7 +334,7 @@ abstract class AbstractTransactionManager {
 	abstract void doClose(DomainObjectAdapterDB object);
 
 	/**
-	 * Set instance as immutable by disabling use of transactions.  Attempts to start a transaction
+	 * Set instance as immutable by disabling use of transactions. Attempts to start a transaction
 	 * will result in a {@link TerminatedTransactionException}.
 	 */
 	public void setImmutable() {

Ghidra/Framework/Project/src/main/java/ghidra/framework/data/DomainObjectAdapterDB.java

@@ -133,7 +133,7 @@ public abstract class DomainObjectAdapterDB extends DomainObjectAdapter implemen
 	 * using a shared transaction manager.  If either or both is already shared,
 	 * a transition to a single shared transaction manager will be
 	 * performed.
-	 * @param domainObj
+	 * @param domainObj the domain object to synchronize with
 	 * @throws LockException if lock or open transaction is active on either
 	 * this or the specified domain object
 	 */
@@ -184,7 +184,7 @@ public abstract class DomainObjectAdapterDB extends DomainObjectAdapter implemen
 	}
 
 	/**
-	 * Returns the open handle to the underlying database.
+	 * {@return the open handle to the underlying database}
 	 */
 	public DBHandle getDBHandle() {
 		return dbh;
@@ -259,17 +259,17 @@ public abstract class DomainObjectAdapterDB extends DomainObjectAdapter implemen
 		return transactionMgr.lock(reason);
 	}
 
-	void prepareToSave() {
+	/**
-		int txId = transactionMgr.startTransaction(this, "Update Metadata", null, true, true);
+	 * Prepare to save and store any last minute DB data.  The default behavior is to invoke
-		try {
+	 * {@link #updateMetadata()}.
+	 */
+	protected void prepareToSave() {
+		try (Transaction tx = openForcedTransaction("Update Metadata")) {
 			updateMetadata();
 		}
 		catch (IOException e) {
 			dbError(e);
 		}
-		finally {
-			transactionMgr.endTransaction(this, txId, true, true);
-		}
 	}
 
 	/**
@@ -310,24 +310,40 @@ public abstract class DomainObjectAdapterDB extends DomainObjectAdapter implemen
 		transactionMgr.unlock(handler);
 	}
 
+	private class DomainObjectTransaction extends Transaction {
+		private final int txId;
+
+		DomainObjectTransaction(int txId) {
+			this.txId = txId;
+		}
+
+		@Override
+		protected boolean endTransaction(boolean commit) {
+			DomainObjectAdapterDB.this.endTransaction(txId, commit);
+			return commit;
+		}
+
+		@Override
+		public boolean isSubTransaction() {
+			return true;
+		}
+	}
+
+	/**
+	 * Open forced transaction which bypasses any lock checking, although method should only be
+	 * used in very controlled situations where a save lock is in place.
+	 * @param description transaction description
+	 * @return {@link AutoCloseable} transaction object
+	 */
+	protected Transaction openForcedTransaction(String description) {
+		int txId = transactionMgr.startTransaction(this, description, null, true, true);
+		return new DomainObjectTransaction(txId);
+	}
+
 	@Override
 	public Transaction openTransaction(String description)
 			throws TerminatedTransactionException, IllegalStateException {
-		return new Transaction() {
+		return new DomainObjectTransaction(startTransaction(description));
-
-			int txId = startTransaction(description);
-
-			@Override
-			protected boolean endTransaction(boolean commit) {
-				DomainObjectAdapterDB.this.endTransaction(txId, commit);
-				return commit;
-			}
-
-			@Override
-			public boolean isSubTransaction() {
-				return true;
-			}
-		};
 	}
 
 	@Override
@@ -348,7 +364,8 @@ public abstract class DomainObjectAdapterDB extends DomainObjectAdapter implemen
 
 		while (true) {
 			try {
-				return transactionMgr.startTransaction(this, description, listener, true);
+				return transactionMgr.startTransactionChecked(this, description, listener,
+					true);
 			}
 			catch (DomainObjectLockedException e) {
 				if (!exceptionHandler.apply(e)) {

Ghidra/Framework/Project/src/main/java/ghidra/framework/data/DomainObjectTransactionManager.java

@@ -103,11 +103,12 @@ class DomainObjectTransactionManager extends AbstractTransactionManager {
 			throw new IllegalArgumentException("invalid domain object");
 		}
 
-		if (!force) {
-			verifyNoLock();
-		}
-
 		if (transaction == null) {
+
+			if (!force) {
+				verifyNoLock();
+			}
+
 			transactionTerminated = false;
 			transaction =
 				new DomainObjectDBTransaction(domainObj.dbh.startTransaction(), domainObj);

Ghidra/Framework/Project/src/main/java/ghidra/framework/data/GhidraFolderData.java

@@ -1037,7 +1037,7 @@ class GhidraFolderData {
 			if (doa.isClosed()) {
 				throw new ClosedException();
 			}
-			if (!doa.lock(null)) {
+			if (!doa.lock("create file")) {
 				throw new IOException("Object is busy and can not be saved");
 			}
 

Ghidra/Framework/Project/src/main/java/ghidra/framework/data/SynchronizedTransactionManager.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.
@@ -152,11 +152,12 @@ class SynchronizedTransactionManager extends AbstractTransactionManager {
 	synchronized int startTransaction(DomainObjectAdapterDB object, String description,
 			AbortedTransactionListener listener, boolean force, boolean notify) {
 
-		if (!force) {
-			verifyNoLock();
-		}
-
 		if (transaction == null) {
+
+			if (!force) {
+				verifyNoLock();
+			}
+
 			transactionTerminated = false;
 			transaction = new SynchronizedTransaction(domainObjectTransactionManagers);
 			int txId = transaction.addEntry(object, description, listener);

Ghidra/Framework/Project/src/main/java/ghidra/framework/main/FileActionManager.java

@@ -311,7 +311,7 @@ class FileActionManager {
 				locked = false;
 				break;
 			}
-			if (!domainObjects[lastIndex].lock(null)) {
+			if (!domainObjects[lastIndex].lock("save changes")) {
 				String title = "Exit Ghidra";
 				StringBuffer buf = new StringBuffer();
 				DomainObject d = domainObjects[lastIndex];

Ghidra/Framework/Project/src/main/java/ghidra/framework/model/DomainObject.java

@@ -400,7 +400,7 @@ public interface DomainObject {
 	 * </pre>
 	 * 
 	 * @param description a short description of the changes to be made.
-	 * @return transaction object
+	 * @return {@link AutoCloseable} transaction object
 	 * @throws IllegalStateException if this {@link DomainObject} has already been closed.
 	 */
 	public Transaction openTransaction(String description) throws IllegalStateException;