GP-0: Fixed DebuggerModelServiceTest. Quite a few changes.

Ghidra/Debug/Debugger-agent-dbgeng/src/main/java/agent/dbgeng/dbgeng/DebugInputCallbacks.java

@@ -20,6 +20,7 @@ import java.util.concurrent.CompletableFuture;
 /**
  * The interface for receiving input callbacks via {@code IDebugInputCallbacks} or a newer variant.
  * 
+ * <p>
  * Note: The wrapper implementation will select the appropriate native interface version.
  */
 @FunctionalInterface

Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/model/DefaultTraceRecorder.java

@@ -346,7 +346,7 @@ public class DefaultTraceRecorder implements TraceRecorder {
 
 	protected void invalidate() {
 		valid = false;
-		//listenerForRecord.dispose();
+		objectManager.disposeModelListeners();
 		trace.release(this);
 	}
 

Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/model/PermanentTransactionExecutor.java

@@ -30,9 +30,11 @@ public class PermanentTransactionExecutor {
 
 	private final TransactionCoalescer txc;
 	private final Executor executor;
+	private final UndoableDomainObject obj;
 
 	public PermanentTransactionExecutor(UndoableDomainObject obj, String name,
 			Function<ThreadFactory, Executor> executorFactory, int delayMs) {
+		this.obj = obj;
 		txc = new DefaultTransactionCoalescer<>(obj, RecorderPermanentTransaction::start, delayMs);
 		this.executor = executorFactory.apply(
 			new BasicThreadFactory.Builder().namingPattern(name + "-thread-%d").build());
@@ -40,6 +42,9 @@ public class PermanentTransactionExecutor {
 
 	public void execute(String description, Runnable runnable) {
 		CompletableFuture.runAsync(() -> {
+			if (obj.isClosed()) {
+				return;
+			}
 			try (CoalescedTx tx = txc.start(description)) {
 				runnable.run();
 			}

Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/model/TraceEventListener.java

@@ -266,4 +266,9 @@ public class TraceEventListener extends AnnotatedDebuggerAttributeListener {
 		return recorder.getThreadMap();
 	}
 
+	public void dispose() {
+		target.getModel().removeModelListener(reorderer);
+		reorderer.dispose();
+	}
+
 }

Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/model/TraceObjectListener.java

@@ -209,6 +209,11 @@ public class TraceObjectListener implements DebuggerModelListener {
 		});
 	}
 
+	public void dispose() {
+		target.getModel().removeModelListener(reorderer);
+		reorderer.dispose();
+	}
+
 	/*
 	private CompletableFuture<List<TargetObject>> findDependenciesTop(TargetObject added) {
 		List<TargetObject> result = new ArrayList<>();

Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/model/TraceObjectManager.java

@@ -648,4 +648,9 @@ public class TraceObjectManager {
 		objects.remove(path);
 	}
 
+	public void disposeModelListeners() {
+		eventListener.dispose();
+		objectListener.dispose();
+	}
+
 }

Ghidra/Debug/Debugger/src/main/java/ghidra/app/services/TraceRecorder.java

@@ -183,6 +183,7 @@ public interface TraceRecorder {
 	/**
 	 * Check if recording is active and the given view is at the present
 	 * 
+	 * <p>
 	 * To be at the present means the view's trace and snap matches the recorder's trace and snap.
 	 * The recorder must also be actively recording. Otherwise, this returns {@code false}.
 	 * 

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

@@ -46,6 +46,7 @@ import mockit.VerificationsInOrder;
 /**
  * TODO: Cover the error cases, and cases where {@code null} is expected
  * 
+ * <p>
  * TODO: Cover cases where multiple recorders are present
  */
 public class DebuggerModelServiceTest extends AbstractGhidraHeadedDebuggerGUITest
@@ -277,7 +278,9 @@ public class DebuggerModelServiceTest extends AbstractGhidraHeadedDebuggerGUITes
 		CollectionChangeDelegateWrapper<TraceRecorder> wrapper =
 			new CollectionChangeDelegateWrapper<>(recorderChangeListener);
 		modelService.addTraceRecordersChangedListener(wrapper);
+		Trace trace = recorder.getTrace();
 		recorder.stopRecording();
+		waitForDomainObject(trace);
 
 		new VerificationsInOrder() {
 			{

Ghidra/Debug/Framework-Debugging/src/main/java/ghidra/dbg/util/DebuggerCallbackReorderer.java

@@ -48,7 +48,10 @@ public class DebuggerCallbackReorderer implements DebuggerModelListener {
 		ObjectRecord(TargetObject obj) {
 			this.obj = obj;
 			TargetObject parent = obj.getParent();
-			ObjectRecord parentRecord = parent == null ? null : records.get(parent);
+			ObjectRecord parentRecord;
+			synchronized (records) {
+				parentRecord = parent == null ? null : records.get(parent);
+			}
 			if (parentRecord == null) {
 				complete = addedToParent.thenApply(this::completed);
 			}
@@ -59,7 +62,9 @@ public class DebuggerCallbackReorderer implements DebuggerModelListener {
 		}
 
 		TargetObject completed(TargetObject obj) {
-			records.remove(obj);
+			synchronized (records) {
+				records.remove(obj);
+			}
 			// NB. We should already be on the clientExecutor
 			Map<String, ?> attributes = obj.getCallbackAttributes();
 			if (!attributes.isEmpty()) {
@@ -85,6 +90,11 @@ public class DebuggerCallbackReorderer implements DebuggerModelListener {
 				addedToParent.cancel(false);
 			}
 		}
+
+		public void cancel() {
+			addedToParent.cancel(false);
+			complete.cancel(false);
+		}
 	}
 
 	private final DebuggerModelListener listener;
@@ -92,6 +102,8 @@ public class DebuggerCallbackReorderer implements DebuggerModelListener {
 	private final Map<TargetObject, ObjectRecord> records = new HashMap<>();
 	private CompletableFuture<Void> lastEvent = AsyncUtils.NIL;
 
+	private volatile boolean disposed = false;
+
 	public DebuggerCallbackReorderer(DebuggerModelListener listener) {
 		this.listener = listener;
 	}
@@ -107,34 +119,57 @@ public class DebuggerCallbackReorderer implements DebuggerModelListener {
 
 	@Override
 	public void catastrophic(Throwable t) {
+		if (disposed) {
+			return;
+		}
 		listener.catastrophic(t);
 	}
 
 	@Override
 	public void modelClosed(DebuggerModelClosedReason reason) {
+		if (disposed) {
+			return;
+		}
 		listener.modelClosed(reason);
 	}
 
 	@Override
 	public void modelOpened() {
+		if (disposed) {
+			return;
+		}
 		listener.modelOpened();
 	}
 
 	@Override
 	public void modelStateChanged() {
+		if (disposed) {
+			return;
+		}
 		listener.modelStateChanged();
 	}
 
 	@Override
 	public void created(TargetObject object) {
+		if (disposed) {
+			return;
+		}
 		//System.err.println("created object='" + object.getJoinedPath(".") + "'");
-		records.put(object, new ObjectRecord(object));
+		synchronized (records) {
+			records.put(object, new ObjectRecord(object));
+		}
 		defensive(() -> listener.created(object), "created");
 	}
 
 	@Override
 	public void invalidated(TargetObject object, TargetObject branch, String reason) {
-		ObjectRecord remove = records.remove(object);
+		if (disposed) {
+			return;
+		}
+		ObjectRecord remove;
+		synchronized (records) {
+			remove = records.remove(object);
+		}
 		if (remove != null) {
 			remove.removed();
 		}
@@ -143,16 +178,27 @@ public class DebuggerCallbackReorderer implements DebuggerModelListener {
 
 	@Override
 	public void rootAdded(TargetObject root) {
+		if (disposed) {
+			return;
+		}
 		defensive(() -> listener.rootAdded(root), "rootAdded");
-		records.get(root).added();
+		synchronized (records) {
+			records.get(root).added();
+		}
 	}
 
 	@Override
 	public void attributesChanged(TargetObject object, Collection<String> removed,
 			Map<String, ?> added) {
+		if (disposed) {
+			return;
+		}
 		//System.err.println("attributesChanged object=" + object.getJoinedPath(".") + ",removed=" +
 		//	removed + ",added=" + added);
-		ObjectRecord record = records.get(object);
+		ObjectRecord record;
+		synchronized (records) {
+			record = records.get(object);
+		}
 		if (record == null) {
 			defensive(() -> listener.attributesChanged(object, removed, added),
 				"attributesChanged");
@@ -164,7 +210,10 @@ public class DebuggerCallbackReorderer implements DebuggerModelListener {
 			if (val instanceof TargetObject) {
 				TargetObject obj = (TargetObject) val;
 				if (!PathUtils.isLink(object.getPath(), ent.getKey(), obj.getPath())) {
-					ObjectRecord rec = records.get(obj);
+					ObjectRecord rec;
+					synchronized (records) {
+						rec = records.get(obj);
+					}
 					if (rec != null) {
 						rec.added();
 					}
@@ -176,9 +225,15 @@ public class DebuggerCallbackReorderer implements DebuggerModelListener {
 	@Override
 	public void elementsChanged(TargetObject object, Collection<String> removed,
 			Map<String, ? extends TargetObject> added) {
+		if (disposed) {
+			return;
+		}
 		//System.err.println("elementsChanged object=" + object.getJoinedPath(".") + ",removed=" +
 		//	removed + ",added=" + added);
-		ObjectRecord record = records.get(object);
+		ObjectRecord record;
+		synchronized (records) {
+			record = records.get(object);
+		}
 		if (record == null) {
 			defensive(() -> listener.elementsChanged(object, removed, added), "elementsChanged");
 		}
@@ -187,7 +242,10 @@ public class DebuggerCallbackReorderer implements DebuggerModelListener {
 			//System.err.println("  " + ent.getKey());
 			TargetObject obj = ent.getValue();
 			if (!PathUtils.isElementLink(object.getPath(), ent.getKey(), obj.getPath())) {
-				ObjectRecord rec = records.get(obj);
+				ObjectRecord rec;
+				synchronized (records) {
+					rec = records.get(obj);
+				}
 				if (rec != null) {
 					rec.added();
 				}
@@ -195,14 +253,15 @@ public class DebuggerCallbackReorderer implements DebuggerModelListener {
 		}
 	}
 
-	private synchronized void orderedOnObjects(Collection<TargetObject> objects, Runnable r,
+	private void orderedOnObjects(Collection<TargetObject> objects, Runnable r, String cb) {
-			String cb) {
 		AsyncFence fence = new AsyncFence();
 		fence.include(lastEvent);
-		for (TargetObject obj : objects) {
+		synchronized (records) {
-			ObjectRecord record = records.get(obj);
+			for (TargetObject obj : objects) {
-			if (record != null) {
+				ObjectRecord record = records.get(obj);
-				fence.include(record.complete);
+				if (record != null) {
+					fence.include(record.complete);
+				}
 			}
 		}
 		lastEvent = fence.ready().thenAccept(__ -> {
@@ -216,6 +275,9 @@ public class DebuggerCallbackReorderer implements DebuggerModelListener {
 	@Override
 	public void breakpointHit(TargetObject container, TargetObject trapped, TargetStackFrame frame,
 			TargetBreakpointSpec spec, TargetBreakpointLocation breakpoint) {
+		if (disposed) {
+			return;
+		}
 		List<TargetObject> args = frame == null
 				? List.of(container, trapped, spec, breakpoint)
 				: List.of(container, trapped, frame, spec, breakpoint);
@@ -226,6 +288,9 @@ public class DebuggerCallbackReorderer implements DebuggerModelListener {
 
 	@Override
 	public void consoleOutput(TargetObject console, Channel channel, byte[] data) {
+		if (disposed) {
+			return;
+		}
 		orderedOnObjects(List.of(console), () -> {
 			listener.consoleOutput(console, channel, data);
 		}, "consoleOutput");
@@ -246,6 +311,9 @@ public class DebuggerCallbackReorderer implements DebuggerModelListener {
 	@Override
 	public void event(TargetObject object, TargetThread eventThread, TargetEventType type,
 			String description, List<Object> parameters) {
+		if (disposed) {
+			return;
+		}
 		List<TargetObject> objs = eventThread == null
 				? List.of(object)
 				: List.of(object, eventThread);
@@ -256,6 +324,9 @@ public class DebuggerCallbackReorderer implements DebuggerModelListener {
 
 	@Override
 	public void invalidateCacheRequested(TargetObject object) {
+		if (disposed) {
+			return;
+		}
 		orderedOnObjects(List.of(object), () -> {
 			listener.invalidateCacheRequested(object);
 		}, "invalidateCacheRequested");
@@ -264,6 +335,9 @@ public class DebuggerCallbackReorderer implements DebuggerModelListener {
 	@Override
 	public void memoryReadError(TargetObject memory, AddressRange range,
 			DebuggerMemoryAccessException e) {
+		if (disposed) {
+			return;
+		}
 		orderedOnObjects(List.of(memory), () -> {
 			listener.memoryReadError(memory, range, e);
 		}, "invalidateCacheRequested");
@@ -271,6 +345,9 @@ public class DebuggerCallbackReorderer implements DebuggerModelListener {
 
 	@Override
 	public void memoryUpdated(TargetObject memory, Address address, byte[] data) {
+		if (disposed) {
+			return;
+		}
 		orderedOnObjects(List.of(memory), () -> {
 			listener.memoryUpdated(memory, address, data);
 		}, "invalidateCacheRequested");
@@ -278,8 +355,23 @@ public class DebuggerCallbackReorderer implements DebuggerModelListener {
 
 	@Override
 	public void registersUpdated(TargetObject bank, Map<String, byte[]> updates) {
+		if (disposed) {
+			return;
+		}
 		orderedOnObjects(List.of(bank), () -> {
 			listener.registersUpdated(bank, updates);
 		}, "invalidateCacheRequested");
 	}
+
+	public void dispose() {
+		disposed = true;
+		Set<ObjectRecord> volRecs;
+		synchronized (records) {
+			volRecs = Set.copyOf(records.values());
+			records.clear();
+		}
+		for (ObjectRecord rec : volRecs) {
+			rec.cancel();
+		}
+	}
 }

Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/util/DefaultTraceTimeViewport.java

@@ -21,6 +21,8 @@ import java.util.stream.Collectors;
 
 import com.google.common.collect.*;
 
+import ghidra.framework.model.DomainObjectClosedListener;
+import ghidra.framework.model.DomainObjectException;
 import ghidra.program.model.address.*;
 import ghidra.trace.model.Trace;
 import ghidra.trace.model.Trace.TraceSnapshotChangeType;
@@ -29,6 +31,7 @@ import ghidra.trace.model.program.TraceProgramView;
 import ghidra.trace.model.time.*;
 import ghidra.util.*;
 import ghidra.util.datastruct.ListenerSet;
+import ghidra.util.exception.ClosedException;
 
 /**
  * Computes and tracks the "viewport" resulting from forking patterns encoded in snapshot schedules
@@ -46,7 +49,8 @@ import ghidra.util.datastruct.ListenerSet;
  * optimization.
  */
 public class DefaultTraceTimeViewport implements TraceTimeViewport {
-	protected class ForSnapshotsListener extends TraceDomainObjectListener {
+	protected class ForSnapshotsListener extends TraceDomainObjectListener
+			implements DomainObjectClosedListener {
 		{
 			listenFor(TraceSnapshotChangeType.ADDED, this::snapshotAdded);
 			listenFor(TraceSnapshotChangeType.CHANGED, this::snapshotChanged);
@@ -80,8 +84,14 @@ public class DefaultTraceTimeViewport implements TraceTimeViewport {
 				return;
 			}
 		}
+
+		@Override
+		public void domainObjectClosed() {
+			trace.removeListener(this);
+		}
 	}
 
+	protected final Trace trace;
 	protected final TraceTimeManager timeManager;
 	protected final List<Range<Long>> ordered = new ArrayList<>();
 	protected final RangeSet<Long> spanSet = TreeRangeSet.create();
@@ -91,7 +101,9 @@ public class DefaultTraceTimeViewport implements TraceTimeViewport {
 	protected long snap;
 
 	public DefaultTraceTimeViewport(Trace trace) {
+		this.trace = trace;
 		this.timeManager = trace.getTimeManager();
+		trace.addCloseListener(listener);
 		trace.addListener(listener);
 	}
 

Ghidra/Debug/ProposedUtils/src/main/java/ghidra/util/datastruct/ListenerMap.java

@@ -118,17 +118,16 @@ public class ListenerMap<K, P, V extends P> {
 		public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
 			//Msg.debug(this, "Queuing invocation: " + method.getName() + " @" +
 			//	System.identityHashCode(executor));
-			Collection<V> listenersVolatile;
+			// Listener adds/removes need to take immediate effect, even with queued events
-			Set<ListenerMap<?, ? extends P, ?>> chainedVolatile;
+			executor.execute(() -> {
-			synchronized (lock) {
+				Collection<V> listenersVolatile;
-				listenersVolatile = map.values();
+				synchronized (lock) {
-				chainedVolatile = chained;
+					listenersVolatile = map.values();
-			}
-			for (V l : listenersVolatile) {
-				if (!ext.isAssignableFrom(l.getClass())) {
-					continue;
 				}
-				executor.execute(() -> {
+				for (V l : listenersVolatile) {
+					if (!ext.isAssignableFrom(l.getClass())) {
+						continue;
+					}
 					//Msg.debug(this,
 					//	"Invoking: " + method.getName() + " @" + System.identityHashCode(executor));
 					try {
@@ -141,9 +140,13 @@ public class ListenerMap<K, P, V extends P> {
 					catch (Throwable e) {
 						reportError(l, e);
 					}
-				});
+				}
+			});
+			Set<ListenerMap<?, ? extends P, ?>> chainedVolatile;
+			synchronized (lock) {
+				chainedVolatile = chained;
 			}
-			for (ListenerMap<?, ? extends P, ?> c : chained) {
+			for (ListenerMap<?, ? extends P, ?> c : chainedVolatile) {
 				// Invocation will check if assignable
 				@SuppressWarnings("unchecked")
 				T l = ((ListenerMap<?, P, ?>) c).fire(ext);

Ghidra/Debug/ProposedUtils/src/test/java/ghidra/util/datastruct/ListenerMapTest.java

@@ -16,13 +16,14 @@
 package ghidra.util.datastruct;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotEquals;
 
+import java.util.Map;
+import java.util.concurrent.*;
 import java.util.concurrent.atomic.AtomicReference;
 
 import org.junit.Test;
 
-import ghidra.util.datastruct.ListenerMap;
-
 public class ListenerMapTest {
 	public interface DummyListener {
 		void event(String e);
@@ -65,6 +66,65 @@ public class ListenerMapTest {
 		assertEquals("EventC", ar3.get());
 	}
 
+	protected void waitEvents(Executor executor) throws Throwable {
+		CompletableFuture.runAsync(() -> {
+		}, executor).get(1000, TimeUnit.MILLISECONDS);
+	}
+
+	@Test
+	public void testAddsRemovesImmediatelyEffective() throws Throwable {
+		Executor executor = Executors.newSingleThreadExecutor();
+		CompletableFuture.runAsync(() -> Thread.currentThread().setName("ExecutorThread"), executor)
+				.get();
+		ListenerMap<String, DummyListener, DummyListener> listeners =
+			new ListenerMap<>(DummyListener.class, executor);
+
+		Map<String, CompletableFuture<?>> stalls = Map.ofEntries(
+			Map.entry("StallA", new CompletableFuture<>()),
+			Map.entry("StallB", new CompletableFuture<>()),
+			Map.entry("StallD", new CompletableFuture<>()));
+		AtomicReference<String> ar1 = new AtomicReference<>();
+		DummyListener l1 = s -> {
+			CompletableFuture<?> stall = stalls.get(s);
+			if (stall != null) {
+				try {
+					stall.get();
+				}
+				catch (InterruptedException | ExecutionException e) {
+					// Nothing I really can do
+				}
+			}
+			ar1.set(s);
+		};
+		AtomicReference<String> ar2 = new AtomicReference<>();
+		DummyListener l2 = ar2::set;
+
+		listeners.put("Key1", l1);
+		ar1.set("None");
+		listeners.fire.event("StallA");
+		assertEquals("None", ar1.get());
+		stalls.get("StallA").complete(null);
+		waitEvents(executor);
+		assertEquals("StallA", ar1.get());
+
+		// NB. It's the the fire timeline that matters, but the completion timeline
+		listeners.fire.event("StallB");
+		listeners.fire.event("EventC");
+		listeners.put("Key2", l2);
+		stalls.get("StallB").complete(null);
+		waitEvents(executor);
+		assertEquals("EventC", ar1.get());
+		assertEquals("EventC", ar2.get());
+
+		listeners.fire.event("StallD");
+		listeners.fire.event("EventE");
+		listeners.remove("Key2");
+		stalls.get("StallD").complete(null);
+		waitEvents(executor);
+		assertEquals("EventE", ar1.get());
+		assertNotEquals("EventE", ar2.get());
+	}
+
 	@Test
 	public void testContinuesOnError() {
 		ListenerMap<String, DummyListener, DummyListener> listeners =