From 6bac39e79c8e05405ee4060c0b4bab1511c130d3 Mon Sep 17 00:00:00 2001 From: Dan <46821332+nsadeveloper789@users.noreply.github.com> Date: Thu, 6 May 2021 11:58:44 -0400 Subject: [PATCH] GP-0: Various UI test fixes, incl. viewport syncing GP-0: Fixed Listing tests wrt/ location label GP-0: Trying to synchronize view creation and memory layout GP-0: Trying to fix CME GP-0: Got viewport synchronized, and it resolved test. GP-0: Fixing regions tests - involved listing selections GP-0: Fixed modules/sections provider tests GP-0: Fixed static-mappings provider tests. --- .../gui/listing/DebuggerListingPlugin.java | 3 +- .../gui/modules/DebuggerModulesProvider.java | 2 +- .../app/services/DebuggerListingService.java | 9 +- .../listing/DebuggerListingProviderTest.java | 28 +-- .../memory/DebuggerRegionsProviderTest.java | 21 +- .../modules/DebuggerModulesProviderTest.java | 32 +-- .../DebuggerStaticMappingProviderTest.java | 11 +- .../java/ghidra/trace/database/DBTrace.java | 32 ++- .../trace/util/DefaultTraceTimeViewport.java | 210 ++++++++++++------ 9 files changed, 215 insertions(+), 133 deletions(-) diff --git a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/gui/listing/DebuggerListingPlugin.java b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/gui/listing/DebuggerListingPlugin.java index 06e41a3823..f4ff8ca1b3 100644 --- a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/gui/listing/DebuggerListingPlugin.java +++ b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/gui/listing/DebuggerListingPlugin.java @@ -180,7 +180,6 @@ public class DebuggerListingPlugin extends CodeBrowserPlugin implements Debugger actionNewListing = new NewListingAction(); } - @Override public DebuggerListingProvider createListingIfMissing(LocationTrackingSpec spec, boolean followsCurrentThread) { synchronized (disconnectedProviders) { @@ -331,7 +330,7 @@ public class DebuggerListingPlugin extends CodeBrowserPlugin implements Debugger @Override public void setCurrentSelection(ProgramSelection selection) { - getListingPanel().setSelection(selection); + getConnectedProvider().setSelection(selection); } @Override diff --git a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/gui/modules/DebuggerModulesProvider.java b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/gui/modules/DebuggerModulesProvider.java index 2739c74d41..146041c632 100644 --- a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/gui/modules/DebuggerModulesProvider.java +++ b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/gui/modules/DebuggerModulesProvider.java @@ -454,7 +454,7 @@ public class DebuggerModulesProvider extends ComponentProviderAdapter { return; } Set modules = getSelectedModules(myActionContext); - if (modules.size() != 1) { + if (modules == null || modules.size() != 1) { return; } TraceModule mod = modules.iterator().next(); diff --git a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/services/DebuggerListingService.java b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/services/DebuggerListingService.java index 6e2999f5e9..0dca9e813d 100644 --- a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/services/DebuggerListingService.java +++ b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/services/DebuggerListingService.java @@ -16,19 +16,16 @@ package ghidra.app.services; import ghidra.app.plugin.core.debug.gui.action.LocationTrackingSpec; -import ghidra.app.plugin.core.debug.gui.listing.*; +import ghidra.app.plugin.core.debug.gui.listing.DebuggerListingPlugin; import ghidra.framework.plugintool.ServiceInfo; import ghidra.program.model.address.Address; import ghidra.program.util.ProgramSelection; @ServiceInfo( // - defaultProvider = DebuggerListingPlugin.class, // - description = "Replacement CodeViewerService for Debugger" // + defaultProvider = DebuggerListingPlugin.class, // + description = "Replacement CodeViewerService for Debugger" // ) public interface DebuggerListingService extends CodeViewerService { - @Deprecated - DebuggerListingProvider createListingIfMissing(LocationTrackingSpec spec, - boolean followsCurrentThread); void setTrackingSpec(LocationTrackingSpec spec); diff --git a/Ghidra/Debug/Debugger/src/test/java/ghidra/app/plugin/core/debug/gui/listing/DebuggerListingProviderTest.java b/Ghidra/Debug/Debugger/src/test/java/ghidra/app/plugin/core/debug/gui/listing/DebuggerListingProviderTest.java index 35cee1e9d5..32f4ff4dbb 100644 --- a/Ghidra/Debug/Debugger/src/test/java/ghidra/app/plugin/core/debug/gui/listing/DebuggerListingProviderTest.java +++ b/Ghidra/Debug/Debugger/src/test/java/ghidra/app/plugin/core/debug/gui/listing/DebuggerListingProviderTest.java @@ -743,15 +743,13 @@ public class DebuggerListingProviderTest extends AbstractGhidraHeadedDebuggerGUI traceManager.activateTrace(tb.trace); waitForSwing(); assertEquals(traceManager.getCurrentView(), listingProvider.getProgram()); - assertEquals("dynamic-testCloseCurrentTraceBlanksListings", - listingProvider.locationLabel.getText()); + assertEquals("(nowhere)", listingProvider.locationLabel.getText()); DebuggerListingProvider extraProvider = runSwing( () -> listingPlugin.createListingIfMissing(trackNone, false)); waitForSwing(); assertEquals(traceManager.getCurrentView(), extraProvider.getProgram()); - assertEquals("dynamic-testCloseCurrentTraceBlanksListings", - extraProvider.locationLabel.getText()); + assertEquals("(nowhere)", extraProvider.locationLabel.getText()); traceManager.closeTrace(tb.trace); waitForSwing(); @@ -1140,26 +1138,30 @@ public class DebuggerListingProviderTest extends AbstractGhidraHeadedDebuggerGUI traceManager.activateTrace(tb.trace); waitForSwing(); - assertEquals("dynamic-testLocationLabel", listingProvider.locationLabel.getText()); + assertEquals("(nowhere)", listingProvider.locationLabel.getText()); try (UndoableTransaction tid = tb.startTransaction()) { tb.trace.getMemoryManager() - .addRegion("exe:.text", Range.atLeast(0L), tb.range(0x55550000, 0x555500ff), + .addRegion("test_region", Range.atLeast(0L), tb.range(0x55550000, 0x555502ff), TraceMemoryFlag.READ, TraceMemoryFlag.EXECUTE); } waitForDomainObject(tb.trace); - waitForPass(() -> assertEquals("dynamic-testLocationLabel (exe:.text)", - listingProvider.locationLabel.getText())); + waitForPass(() -> assertEquals("test_region", listingProvider.locationLabel.getText())); + + TraceModule modExe; + try (UndoableTransaction tid = tb.startTransaction()) { + modExe = tb.trace.getModuleManager() + .addModule("modExe", "modExe", + tb.range(0x55550000, 0x555501ff), Range.atLeast(0L)); + } + waitForDomainObject(tb.trace); + waitForPass(() -> assertEquals("modExe", listingProvider.locationLabel.getText())); try (UndoableTransaction tid = tb.startTransaction()) { - TraceModule modExe = tb.trace.getModuleManager() - .addModule("modExe", "modExe", - tb.range(0x55550000, 0x555500ff), Range.atLeast(0L)); modExe.addSection(".text", tb.range(0x55550000, 0x555500ff)); } waitForDomainObject(tb.trace); - waitForPass(() -> assertEquals("dynamic-testLocationLabel (modExe:.text)", - listingProvider.locationLabel.getText())); + waitForPass(() -> assertEquals("modExe:.text", listingProvider.locationLabel.getText())); } @Test diff --git a/Ghidra/Debug/Debugger/src/test/java/ghidra/app/plugin/core/debug/gui/memory/DebuggerRegionsProviderTest.java b/Ghidra/Debug/Debugger/src/test/java/ghidra/app/plugin/core/debug/gui/memory/DebuggerRegionsProviderTest.java index ecef8ad6d8..c8c1458c19 100644 --- a/Ghidra/Debug/Debugger/src/test/java/ghidra/app/plugin/core/debug/gui/memory/DebuggerRegionsProviderTest.java +++ b/Ghidra/Debug/Debugger/src/test/java/ghidra/app/plugin/core/debug/gui/memory/DebuggerRegionsProviderTest.java @@ -15,12 +15,12 @@ */ package ghidra.app.plugin.core.debug.gui.memory; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertTrue; +import static org.junit.Assert.*; import java.util.Set; -import org.junit.*; +import org.junit.Before; +import org.junit.Test; import com.google.common.collect.Range; @@ -186,6 +186,7 @@ public class DebuggerRegionsProviderTest extends AbstractGhidraHeadedDebuggerGUI traceManager.openTrace(tb.trace); traceManager.activateTrace(tb.trace); waitForSwing(); + waitForPass(() -> assertEquals(1, provider.regionTable.getRowCount())); RegionRow row = Unique.assertOne(provider.regionTableModel.getModelData()); assertEquals(region, row.getRegion()); @@ -198,7 +199,6 @@ public class DebuggerRegionsProviderTest extends AbstractGhidraHeadedDebuggerGUI } @Test - @Ignore("TODO: Seems the error is in the listing") public void testActionSelectAddresses() throws Exception { addPlugin(tool, DebuggerListingPlugin.class); DebuggerListingProvider listing = waitForComponentProvider(DebuggerListingProvider.class); @@ -217,16 +217,17 @@ public class DebuggerRegionsProviderTest extends AbstractGhidraHeadedDebuggerGUI waitForSwing(); RegionRow row = Unique.assertOne(provider.regionTableModel.getModelData()); + waitForPass(() -> assertEquals(1, provider.regionTable.getRowCount())); assertEquals(region, row.getRegion()); + assertFalse(tb.trace.getProgramView().getMemory().isEmpty()); provider.setSelectedRegions(Set.of(region)); waitForSwing(); assertTrue(provider.actionSelectAddresses.isEnabled()); performAction(provider.actionSelectAddresses); - // TODO: This seems to me an error in the listing.... - // When debugging, I see the selection in the listing, but this still returns empty... - assertEquals(tb.range(0x00400000, 0x0040ffff), new AddressSet(listing.getSelection())); + waitForPass(() -> assertEquals(tb.set(tb.range(0x00400000, 0x0040ffff)), + new AddressSet(listing.getSelection()))); } @Test @@ -249,12 +250,16 @@ public class DebuggerRegionsProviderTest extends AbstractGhidraHeadedDebuggerGUI RegionRow row = Unique.assertOne(provider.regionTableModel.getModelData()); assertEquals(region, row.getRegion()); + assertFalse(tb.trace.getProgramView().getMemory().isEmpty()); listing.setSelection(new ProgramSelection(tb.set(tb.range(0x00401234, 0x00404321)))); + waitForPass(() -> assertEquals(tb.set(tb.range(0x00401234, 0x00404321)), + new AddressSet(listing.getSelection()))); + waitForSwing(); assertTrue(provider.actionSelectRows.isEnabled()); performAction(provider.actionSelectRows); - assertEquals(Set.of(row), Set.copyOf(provider.getSelectedRows())); + waitForPass(() -> assertEquals(Set.of(row), Set.copyOf(provider.getSelectedRows()))); } } diff --git a/Ghidra/Debug/Debugger/src/test/java/ghidra/app/plugin/core/debug/gui/modules/DebuggerModulesProviderTest.java b/Ghidra/Debug/Debugger/src/test/java/ghidra/app/plugin/core/debug/gui/modules/DebuggerModulesProviderTest.java index 8140f33431..dc2da484c9 100644 --- a/Ghidra/Debug/Debugger/src/test/java/ghidra/app/plugin/core/debug/gui/modules/DebuggerModulesProviderTest.java +++ b/Ghidra/Debug/Debugger/src/test/java/ghidra/app/plugin/core/debug/gui/modules/DebuggerModulesProviderTest.java @@ -222,6 +222,7 @@ public class DebuggerModulesProviderTest extends AbstractGhidraHeadedDebuggerGUI program.setName(modExe.getName()); } waitForDomainObject(program); + waitForPass(() -> assertEquals(4, modulesProvider.sectionTable.getRowCount())); modulesProvider.setSelectedSections(Set.of(secExeText)); performAction(modulesProvider.actionMapSections, false); @@ -349,6 +350,7 @@ public class DebuggerModulesProviderTest extends AbstractGhidraHeadedDebuggerGUI addBlock(); // So the program has a size } waitForDomainObject(program); + waitForPass(() -> assertEquals(2, modulesProvider.moduleTable.getRowCount())); modulesProvider.setSelectedModules(Set.of(modExe)); waitForSwing(); @@ -411,6 +413,7 @@ public class DebuggerModulesProviderTest extends AbstractGhidraHeadedDebuggerGUI program.setName(modExe.getName()); } waitForDomainObject(program); + waitForPass(() -> assertEquals(4, modulesProvider.sectionTable.getRowCount())); modulesProvider.setSelectedSections(Set.of(secExeText)); waitForSwing(); @@ -470,6 +473,8 @@ public class DebuggerModulesProviderTest extends AbstractGhidraHeadedDebuggerGUI traceManager.activateTrace(tb.trace); waitForSwing(); // NOTE: The table may select first by default, enabling action + waitForPass(() -> assertEquals(2, modulesProvider.moduleTable.getRowCount())); + waitForPass(() -> assertEquals(4, modulesProvider.sectionTable.getRowCount())); modulesProvider.setSelectedModules(Set.of(modExe)); waitForSwing(); assertTrue(modulesProvider.actionSelectAddresses.isEnabled()); @@ -608,6 +613,8 @@ public class DebuggerModulesProviderTest extends AbstractGhidraHeadedDebuggerGUI try (UndoableTransaction tid = tb.startTransaction()) { modExe.setName("/bin/echo"); // File has to exist } + waitForPass(() -> assertEquals(2, modulesProvider.moduleTable.getRowCount())); + modulesProvider.setSelectedModules(Set.of(modExe)); waitForSwing(); performAction(modulesProvider.actionImportFromFileSystem, false); @@ -616,6 +623,10 @@ public class DebuggerModulesProviderTest extends AbstractGhidraHeadedDebuggerGUI dialog.close(); } + protected Set visibleSections() { + return Set.copyOf(modulesProvider.sectionFilterPanel.getTableFilterModel().getModelData()); + } + @Test public void testActionFilterSections() throws Exception { addPlugin(tool, ImporterPlugin.class); @@ -623,35 +634,29 @@ public class DebuggerModulesProviderTest extends AbstractGhidraHeadedDebuggerGUI addModules(); traceManager.activateTrace(tb.trace); waitForSwing(); + waitForPass(() -> assertEquals(2, modulesProvider.moduleTable.getRowCount())); + waitForPass(() -> assertEquals(4, modulesProvider.sectionTable.getRowCount())); - Set visible = - Set.copyOf(modulesProvider.sectionFilterPanel.getTableFilterModel().getModelData()); - assertEquals(4, visible.size()); + assertEquals(4, visibleSections().size()); modulesProvider.setSelectedModules(Set.of(modExe)); waitForSwing(); - visible = - Set.copyOf(modulesProvider.sectionFilterPanel.getTableFilterModel().getModelData()); - assertEquals(4, visible.size()); + assertEquals(4, visibleSections().size()); assertTrue(modulesProvider.actionFilterSectionsByModules.isEnabled()); performAction(modulesProvider.actionFilterSectionsByModules); waitForSwing(); - visible = - Set.copyOf(modulesProvider.sectionFilterPanel.getTableFilterModel().getModelData()); - assertEquals(2, visible.size()); - for (SectionRow row : visible) { + assertEquals(2, visibleSections().size()); + for (SectionRow row : visibleSections()) { assertEquals(modExe, row.getModule()); } modulesProvider.setSelectedModules(Set.of()); waitForSwing(); - visible = - Set.copyOf(modulesProvider.sectionFilterPanel.getTableFilterModel().getModelData()); - assertEquals(4, visible.size()); + waitForPass(() -> assertEquals(4, visibleSections().size())); } protected static final Set POPUP_ACTIONS = Set.of( @@ -693,6 +698,7 @@ public class DebuggerModulesProviderTest extends AbstractGhidraHeadedDebuggerGUI addModules(); traceManager.activateTrace(tb.trace); waitForSwing(); + waitForPass(() -> assertEquals(4, modulesProvider.sectionTable.getRowCount())); clickTableCellWithButton(modulesProvider.sectionTable, 0, 0, MouseEvent.BUTTON3); waitForSwing(); diff --git a/Ghidra/Debug/Debugger/src/test/java/ghidra/app/plugin/core/debug/gui/modules/DebuggerStaticMappingProviderTest.java b/Ghidra/Debug/Debugger/src/test/java/ghidra/app/plugin/core/debug/gui/modules/DebuggerStaticMappingProviderTest.java index 1e5f093b4c..19370398ac 100644 --- a/Ghidra/Debug/Debugger/src/test/java/ghidra/app/plugin/core/debug/gui/modules/DebuggerStaticMappingProviderTest.java +++ b/Ghidra/Debug/Debugger/src/test/java/ghidra/app/plugin/core/debug/gui/modules/DebuggerStaticMappingProviderTest.java @@ -171,9 +171,10 @@ public class DebuggerStaticMappingProviderTest extends AbstractGhidraHeadedDebug waitForDomainObject(tb.trace); // First check that all records are displayed - List correlationsDisplayed = + waitForPass(() -> assertEquals(3, mappingsProvider.mappingTable.getRowCount())); + List mappingsDisplayed = mappingsProvider.mappingTableModel.getModelData(); - assertEquals(3, correlationsDisplayed.size()); + assertEquals(3, mappingsDisplayed.size()); // Select and remove the first 2 via the action // NOTE: I'm not responsible for making the transaction here. The UI should do it. @@ -182,9 +183,9 @@ public class DebuggerStaticMappingProviderTest extends AbstractGhidraHeadedDebug waitForDomainObject(tb.trace); // Now, check that only the final one remains - correlationsDisplayed = mappingsProvider.mappingTableModel.getModelData(); - assertEquals(1, correlationsDisplayed.size()); - StaticMappingRow record = correlationsDisplayed.get(0); + mappingsDisplayed = mappingsProvider.mappingTableModel.getModelData(); + assertEquals(1, mappingsDisplayed.size()); + StaticMappingRow record = mappingsDisplayed.get(0); assertEquals(tb.addr(0xdeadbeef + 0x180), record.getTraceAddress()); // Check that they were removed from the trace as well diff --git a/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/DBTrace.java b/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/DBTrace.java index 2b43c242cb..a620aa6575 100644 --- a/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/DBTrace.java +++ b/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/DBTrace.java @@ -525,19 +525,23 @@ public class DBTrace extends DBCachedDomainObjectAdapter implements Trace, Trace @Override public DBTraceProgramView getFixedProgramView(long snap) { - DBTraceProgramView view = fixedProgramViews.computeIfAbsent(snap, t -> { - Msg.debug(this, "Creating fixed view at snap=" + snap); - return new DBTraceProgramView(this, snap, baseCompilerSpec); - }); - return view; + synchronized (fixedProgramViews) { + DBTraceProgramView view = fixedProgramViews.computeIfAbsent(snap, t -> { + Msg.debug(this, "Creating fixed view at snap=" + snap); + return new DBTraceProgramView(this, snap, baseCompilerSpec); + }); + return view; + } } @Override public DBTraceVariableSnapProgramView createProgramView(long snap) { - DBTraceVariableSnapProgramView view = - new DBTraceVariableSnapProgramView(this, snap, baseCompilerSpec); - programViews.put(view, null); - return view; + synchronized (programViews) { + DBTraceVariableSnapProgramView view = + new DBTraceVariableSnapProgramView(this, snap, baseCompilerSpec); + programViews.put(view, null); + return view; + } } @Override @@ -675,10 +679,14 @@ public class DBTrace extends DBCachedDomainObjectAdapter implements Trace, Trace } protected void allViews(Consumer action) { - for (DBTraceProgramView view : programViews.keySet()) { - action.accept(view); + Collection all = new ArrayList<>(); + synchronized (programViews) { + all.addAll(programViews.keySet()); } - for (DBTraceProgramView view : fixedProgramViews.values()) { + synchronized (fixedProgramViews) { + all.addAll(fixedProgramViews.values()); + } + for (DBTraceProgramView view : all) { action.accept(view); } } 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 2d11744655..053d51f9f6 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 @@ -56,30 +56,20 @@ public class DefaultTraceTimeViewport implements TraceTimeViewport { } private void snapshotAdded(TraceSnapshot snapshot) { - if (snapshot.getSchedule() == null) { - return; - } - if (spanSet.contains(snapshot.getKey())) { - recomputeSnapRanges(); - return; + if (checkSnapshotAddedNeedsRefresh(snapshot)) { + refreshSnapRanges(); } } private void snapshotChanged(TraceSnapshot snapshot) { - if (isLower(snapshot.getKey())) { - recomputeSnapRanges(); - return; - } - if (spanSet.contains(snapshot.getKey()) && snapshot.getSchedule() != null) { - recomputeSnapRanges(); - return; + if (checkSnapshotChangedNeedsRefresh(snapshot)) { + refreshSnapRanges(); } } private void snapshotDeleted(TraceSnapshot snapshot) { - if (isLower(snapshot.getKey())) { - recomputeSnapRanges(); - return; + if (checkSnapshotDeletedNeedsRefresh(snapshot)) { + refreshSnapRanges(); } } @@ -117,27 +107,31 @@ public class DefaultTraceTimeViewport implements TraceTimeViewport { @Override public boolean containsAnyUpper(Range range) { - // NB. This should only ever visit the first range intersecting that given - for (Range intersecting : spanSet.subRangeSet(range).asRanges()) { - if (range.contains(intersecting.upperEndpoint())) { - return true; + synchronized (ordered) { + // NB. This should only ever visit the first range intersecting that given + for (Range intersecting : spanSet.subRangeSet(range).asRanges()) { + if (range.contains(intersecting.upperEndpoint())) { + return true; + } } + return false; } - return false; } @Override public boolean isCompletelyVisible(AddressRange range, Range lifespan, T object, Occlusion occlusion) { - for (Range rng : ordered) { - if (lifespan.contains(rng.upperEndpoint())) { - return true; - } - if (occlusion.occluded(object, range, rng)) { - return false; + synchronized (ordered) { + for (Range rng : ordered) { + if (lifespan.contains(rng.upperEndpoint())) { + return true; + } + if (occlusion.occluded(object, range, rng)) { + return false; + } } + return false; } - return false; } @Override @@ -147,13 +141,15 @@ public class DefaultTraceTimeViewport implements TraceTimeViewport { return new AddressSet(); } AddressSet remains = new AddressSet(set); - for (Range rng : ordered) { - if (lifespan.contains(rng.upperEndpoint())) { - return remains; - } - occlusion.remove(object, remains, rng); - if (remains.isEmpty()) { - return remains; + synchronized (ordered) { + for (Range rng : ordered) { + if (lifespan.contains(rng.upperEndpoint())) { + return remains; + } + occlusion.remove(object, remains, rng); + if (remains.isEmpty()) { + return remains; + } } } // This condition should have been detected by !containsAnyUpper @@ -161,14 +157,17 @@ public class DefaultTraceTimeViewport implements TraceTimeViewport { } protected boolean isLower(long lower) { - Range range = spanSet.rangeContaining(lower); - if (range == null) { - return false; + synchronized (ordered) { + Range range = spanSet.rangeContaining(lower); + if (range == null) { + return false; + } + return range.lowerEndpoint().longValue() == lower; } - return range.lowerEndpoint().longValue() == lower; } - protected boolean addSnapRange(long lower, long upper) { + protected static boolean addSnapRange(long lower, long upper, RangeSet spanSet, + List> ordered) { if (spanSet.contains(lower)) { return false; } @@ -178,7 +177,7 @@ public class DefaultTraceTimeViewport implements TraceTimeViewport { return true; } - protected TraceSnapshot locateMostRecentFork(long from) { + protected static TraceSnapshot locateMostRecentFork(TraceTimeManager timeManager, long from) { while (true) { TraceSnapshot prev = timeManager.getMostRecentSnapshot(from); if (prev == null) { @@ -203,11 +202,23 @@ public class DefaultTraceTimeViewport implements TraceTimeViewport { } } - protected void traverseAndAddForkRanges(long curSnap) { + /** + * Construct the ranges (set and ordered) + * + *

+ * NOTE: I cannot hold the lock during this, because I also require the DB's read lock. There + * are other operations, e.g., addRegion, that will hold the DB's write lock, and then also + * require the viewport's lock to check if it is visible. That would cause the classic tango of + * death. + * + * @param curSnap the seed snap + */ + protected static void collectForkRanges(TraceTimeManager timeManager, long curSnap, + RangeSet spanSet, List> ordered) { while (true) { - TraceSnapshot fork = locateMostRecentFork(curSnap); + TraceSnapshot fork = locateMostRecentFork(timeManager, curSnap); long prevSnap = fork == null ? Long.MIN_VALUE : fork.getKey(); - if (!addSnapRange(prevSnap, curSnap)) { + if (!addSnapRange(prevSnap, curSnap, spanSet, ordered)) { return; } if (fork == null) { @@ -217,71 +228,124 @@ public class DefaultTraceTimeViewport implements TraceTimeViewport { } } - protected void recomputeSnapRanges() { - spanSet.clear(); - ordered.clear(); - traverseAndAddForkRanges(snap); + protected void refreshSnapRanges() { + RangeSet spanSet = TreeRangeSet.create(); + List> ordered = new ArrayList<>(); + collectForkRanges(timeManager, snap, spanSet, ordered); + synchronized (this.ordered) { + this.spanSet.clear(); + this.ordered.clear(); + this.spanSet.addAll(spanSet); + this.ordered.addAll(ordered); + } assert !ordered.isEmpty(); changeListeners.fire.run(); } public void setSnap(long snap) { this.snap = snap; - recomputeSnapRanges(); + refreshSnapRanges(); + } + + protected boolean checkSnapshotAddedNeedsRefresh(TraceSnapshot snapshot) { + synchronized (ordered) { + if (snapshot.getSchedule() == null) { + return false; + } + if (spanSet.contains(snapshot.getKey())) { + return true; + } + return false; + } + } + + protected boolean checkSnapshotChangedNeedsRefresh(TraceSnapshot snapshot) { + synchronized (ordered) { + if (isLower(snapshot.getKey())) { + return true; + } + if (spanSet.contains(snapshot.getKey()) && snapshot.getSchedule() != null) { + return true; + } + return false; + } + } + + protected boolean checkSnapshotDeletedNeedsRefresh(TraceSnapshot snapshot) { + synchronized (ordered) { + if (isLower(snapshot.getKey())) { + return true; + } + return false; + } } @Override public boolean isForked() { - return ordered.size() > 1; + synchronized (ordered) { + return ordered.size() > 1; + } } @Override public List getOrderedSnaps() { - return ordered - .stream() - .map(Range::upperEndpoint) - .collect(Collectors.toList()); + synchronized (ordered) { + return ordered + .stream() + .map(Range::upperEndpoint) + .collect(Collectors.toList()); + } } @Override public List getReversedSnaps() { - return Lists.reverse(ordered) - .stream() - .map(Range::upperEndpoint) - .collect(Collectors.toList()); + synchronized (ordered) { + return Lists.reverse(ordered) + .stream() + .map(Range::upperEndpoint) + .collect(Collectors.toList()); + } } @Override public T getTop(Function func) { - for (Range rng : ordered) { - T t = func.apply(rng.upperEndpoint()); - if (t != null) { - return t; + synchronized (ordered) { + for (Range rng : ordered) { + T t = func.apply(rng.upperEndpoint()); + if (t != null) { + return t; + } } + return null; } - return null; } @Override public Iterator mergedIterator(Function> iterFunc, Comparator comparator) { - if (!isForked()) { - return iterFunc.apply(snap); + List> iters; + synchronized (ordered) { + if (!isForked()) { + return iterFunc.apply(snap); + } + iters = ordered.stream() + .map(rng -> iterFunc.apply(rng.upperEndpoint())) + .collect(Collectors.toList()); } - List> iters = ordered.stream() - .map(rng -> iterFunc.apply(rng.upperEndpoint())) - .collect(Collectors.toList()); return new UniqIterator<>(new MergeSortingIterator<>(iters, comparator)); } @Override public AddressSetView unionedAddresses(Function viewFunc) { - if (!isForked()) { - return viewFunc.apply(snap); + List views; + synchronized (ordered) { + if (!isForked()) { + return viewFunc.apply(snap); + } + views = ordered.stream() + .map(rng -> viewFunc.apply(rng.upperEndpoint())) + .collect(Collectors.toList()); } - List views = ordered.stream() - .map(rng -> viewFunc.apply(rng.upperEndpoint())) - .collect(Collectors.toList()); return new UnionAddressSetView(views); } }