diff --git a/Ghidra/Debug/Debugger-agent-dbgeng/src/main/java/agent/dbgeng/dbgeng/DebugInputCallbacks.java b/Ghidra/Debug/Debugger-agent-dbgeng/src/main/java/agent/dbgeng/dbgeng/DebugInputCallbacks.java index 7aef585707..6ab40c88a7 100644 --- a/Ghidra/Debug/Debugger-agent-dbgeng/src/main/java/agent/dbgeng/dbgeng/DebugInputCallbacks.java +++ b/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. * + *

* Note: The wrapper implementation will select the appropriate native interface version. */ @FunctionalInterface diff --git a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/model/DefaultTraceRecorder.java b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/model/DefaultTraceRecorder.java index 0510020341..f1bd6c693c 100644 --- a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/model/DefaultTraceRecorder.java +++ b/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); } diff --git a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/model/PermanentTransactionExecutor.java b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/model/PermanentTransactionExecutor.java index e97565e341..0c145ae637 100644 --- a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/model/PermanentTransactionExecutor.java +++ b/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 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(); } diff --git a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/model/TraceEventListener.java b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/model/TraceEventListener.java index bc7e944e45..73c8bf2d96 100644 --- a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/model/TraceEventListener.java +++ b/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(); + } + } diff --git a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/model/TraceObjectListener.java b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/model/TraceObjectListener.java index 03232756f3..5b73d45b01 100644 --- a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/model/TraceObjectListener.java +++ b/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> findDependenciesTop(TargetObject added) { List result = new ArrayList<>(); diff --git a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/model/TraceObjectManager.java b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/model/TraceObjectManager.java index 89af511689..efe918145c 100644 --- a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/model/TraceObjectManager.java +++ b/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(); + } + } diff --git a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/services/TraceRecorder.java b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/services/TraceRecorder.java index b16bcd474b..d3de96e3aa 100644 --- a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/services/TraceRecorder.java +++ b/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 * + *

* 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}. * diff --git a/Ghidra/Debug/Debugger/src/test/java/ghidra/app/plugin/core/debug/service/model/DebuggerModelServiceTest.java b/Ghidra/Debug/Debugger/src/test/java/ghidra/app/plugin/core/debug/service/model/DebuggerModelServiceTest.java index 871d269151..8e9c0c4902 100644 --- a/Ghidra/Debug/Debugger/src/test/java/ghidra/app/plugin/core/debug/service/model/DebuggerModelServiceTest.java +++ b/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 * + *

* TODO: Cover cases where multiple recorders are present */ public class DebuggerModelServiceTest extends AbstractGhidraHeadedDebuggerGUITest @@ -277,7 +278,9 @@ public class DebuggerModelServiceTest extends AbstractGhidraHeadedDebuggerGUITes CollectionChangeDelegateWrapper wrapper = new CollectionChangeDelegateWrapper<>(recorderChangeListener); modelService.addTraceRecordersChangedListener(wrapper); + Trace trace = recorder.getTrace(); recorder.stopRecording(); + waitForDomainObject(trace); new VerificationsInOrder() { { diff --git a/Ghidra/Debug/Framework-Debugging/src/main/java/ghidra/dbg/util/DebuggerCallbackReorderer.java b/Ghidra/Debug/Framework-Debugging/src/main/java/ghidra/dbg/util/DebuggerCallbackReorderer.java index a1a54bf74e..af27c22851 100644 --- a/Ghidra/Debug/Framework-Debugging/src/main/java/ghidra/dbg/util/DebuggerCallbackReorderer.java +++ b/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 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 records = new HashMap<>(); private CompletableFuture 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 removed, Map 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 removed, Map 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 objects, Runnable r, - String cb) { + private void orderedOnObjects(Collection objects, Runnable r, String cb) { AsyncFence fence = new AsyncFence(); fence.include(lastEvent); - for (TargetObject obj : objects) { - ObjectRecord record = records.get(obj); - if (record != null) { - fence.include(record.complete); + synchronized (records) { + for (TargetObject obj : objects) { + ObjectRecord record = records.get(obj); + 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 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 parameters) { + if (disposed) { + return; + } List 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 updates) { + if (disposed) { + return; + } orderedOnObjects(List.of(bank), () -> { listener.registersUpdated(bank, updates); }, "invalidateCacheRequested"); } + + public void dispose() { + disposed = true; + Set volRecs; + synchronized (records) { + volRecs = Set.copyOf(records.values()); + records.clear(); + } + for (ObjectRecord rec : volRecs) { + rec.cancel(); + } + } } diff --git a/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/util/DefaultTraceTimeViewport.java b/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/util/DefaultTraceTimeViewport.java index 614d688e9d..de20b391b8 100644 --- a/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/util/DefaultTraceTimeViewport.java +++ b/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> ordered = new ArrayList<>(); protected final RangeSet 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); } diff --git a/Ghidra/Debug/ProposedUtils/src/main/java/ghidra/util/datastruct/ListenerMap.java b/Ghidra/Debug/ProposedUtils/src/main/java/ghidra/util/datastruct/ListenerMap.java index d1fd7148bc..d1c1cc600c 100644 --- a/Ghidra/Debug/ProposedUtils/src/main/java/ghidra/util/datastruct/ListenerMap.java +++ b/Ghidra/Debug/ProposedUtils/src/main/java/ghidra/util/datastruct/ListenerMap.java @@ -118,17 +118,16 @@ public class ListenerMap { public Object invoke(Object proxy, Method method, Object[] args) throws Throwable { //Msg.debug(this, "Queuing invocation: " + method.getName() + " @" + // System.identityHashCode(executor)); - Collection listenersVolatile; - Set> chainedVolatile; - synchronized (lock) { - listenersVolatile = map.values(); - chainedVolatile = chained; - } - for (V l : listenersVolatile) { - if (!ext.isAssignableFrom(l.getClass())) { - continue; + // Listener adds/removes need to take immediate effect, even with queued events + executor.execute(() -> { + Collection listenersVolatile; + synchronized (lock) { + listenersVolatile = map.values(); } - 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 { catch (Throwable e) { reportError(l, e); } - }); + } + }); + Set> chainedVolatile; + synchronized (lock) { + chainedVolatile = chained; } - for (ListenerMap c : chained) { + for (ListenerMap c : chainedVolatile) { // Invocation will check if assignable @SuppressWarnings("unchecked") T l = ((ListenerMap) c).fire(ext); diff --git a/Ghidra/Debug/ProposedUtils/src/test/java/ghidra/util/datastruct/ListenerMapTest.java b/Ghidra/Debug/ProposedUtils/src/test/java/ghidra/util/datastruct/ListenerMapTest.java index 17fa6cf971..c642606873 100644 --- a/Ghidra/Debug/ProposedUtils/src/test/java/ghidra/util/datastruct/ListenerMapTest.java +++ b/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 listeners = + new ListenerMap<>(DummyListener.class, executor); + + Map> stalls = Map.ofEntries( + Map.entry("StallA", new CompletableFuture<>()), + Map.entry("StallB", new CompletableFuture<>()), + Map.entry("StallD", new CompletableFuture<>())); + AtomicReference 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 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 listeners =