From fd6964070fd0f32f80d137d66bad4839b6894993 Mon Sep 17 00:00:00 2001 From: dragonmacher <48328597+dragonmacher@users.noreply.github.com> Date: Fri, 26 Mar 2021 16:07:55 -0400 Subject: [PATCH] GP-788 - Fixed UI lockup caused by Symbol Table during analysis --- .../plugin/core/bookmark/BookmarkPlugin.java | 4 + .../plugin/core/help/AboutProgramPlugin.java | 2 +- .../core/symtable/ReferenceProvider.java | 4 + .../app/plugin/core/symtable/SymbolPanel.java | 4 +- .../plugin/core/symtable/SymbolProvider.java | 13 +- .../core/symtable/SymbolReferenceModel.java | 3 +- .../core/symtable/SymbolTableModel.java | 5 +- .../core/symtable/SymbolTablePlugin.java | 385 +++++++++++++++--- .../core/symtable/SymbolTablePluginTest.java | 88 ++-- .../widgets/table/threaded/TableData.java | 26 +- 10 files changed, 411 insertions(+), 123 deletions(-) diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/bookmark/BookmarkPlugin.java b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/bookmark/BookmarkPlugin.java index 332096c47f..676e18a407 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/bookmark/BookmarkPlugin.java +++ b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/bookmark/BookmarkPlugin.java @@ -299,6 +299,10 @@ public class BookmarkPlugin extends ProgramPlugin @Override public synchronized void domainObjectChanged(DomainObjectChangedEvent ev) { + if (!provider.isVisible()) { + return; + } + if (ev.containsEvent(DomainObject.DO_OBJECT_RESTORED) || ev.containsEvent(ChangeManager.DOCR_MEMORY_BLOCK_MOVED) || ev.containsEvent(ChangeManager.DOCR_MEMORY_BLOCK_REMOVED)) { diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/help/AboutProgramPlugin.java b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/help/AboutProgramPlugin.java index 830b13d6dc..3819a3d611 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/help/AboutProgramPlugin.java +++ b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/help/AboutProgramPlugin.java @@ -106,7 +106,7 @@ public class AboutProgramPlugin extends Plugin implements FrontEndable { ProgramActionContext pac = (ProgramActionContext) context; Program program = pac.getProgram(); if (program != null) { - String menuName = "About " + program.getDomainFile().getName() + "..."; + String menuName = "About " + program.getDomainFile().getName(); getMenuBarData().setMenuItemName(menuName); return true; } diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/symtable/ReferenceProvider.java b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/symtable/ReferenceProvider.java index 1ffb91758b..a680fddcd9 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/symtable/ReferenceProvider.java +++ b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/symtable/ReferenceProvider.java @@ -134,6 +134,10 @@ class ReferenceProvider extends ComponentProviderAdapter { setVisible(true); } + boolean isBusy() { + return referenceKeyModel.isBusy(); + } + @Override public void componentHidden() { referenceKeyModel.setProgram(null); diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/symtable/SymbolPanel.java b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/symtable/SymbolPanel.java index 1f09fcc85a..cc04711930 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/symtable/SymbolPanel.java +++ b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/symtable/SymbolPanel.java @@ -151,8 +151,8 @@ class SymbolPanel extends JPanel { filterDialog.adjustFilter(symProvider, tableModel); } - NewSymbolFilter getFilter() { - return filterDialog.getFilter(); + SymbolFilter getFilter() { + return tableModel.getFilter(); } void readConfigState(SaveState saveState) { diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/symtable/SymbolProvider.java b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/symtable/SymbolProvider.java index 77ee04ffa0..54b9a4f3e7 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/symtable/SymbolProvider.java +++ b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/symtable/SymbolProvider.java @@ -149,10 +149,14 @@ class SymbolProvider extends ComponentProviderAdapter { return symbolPanel.getTable(); } - NewSymbolFilter getFilter() { + SymbolFilter getFilter() { return symbolPanel.getFilter(); } + boolean isShowingDynamicSymbols() { + return getFilter().acceptsDefaultLabelSymbols(); + } + private String generateSubTitle() { SymbolFilter filter = symbolKeyModel.getFilter(); int rowCount = symbolKeyModel.getRowCount(); @@ -174,11 +178,15 @@ class SymbolProvider extends ComponentProviderAdapter { } } + boolean isBusy() { + return symbolKeyModel.isBusy(); + } + @Override public void componentHidden() { symbolKeyModel.reload(null); if (plugin != null) { - plugin.closeReferenceProvider(); + plugin.symbolProviderClosed(); } } @@ -199,5 +207,4 @@ class SymbolProvider extends ComponentProviderAdapter { void writeConfigState(SaveState saveState) { symbolPanel.writeConfigState(saveState); } - } diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/symtable/SymbolReferenceModel.java b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/symtable/SymbolReferenceModel.java index a3f99f8114..beafc26df3 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/symtable/SymbolReferenceModel.java +++ b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/symtable/SymbolReferenceModel.java @@ -71,7 +71,7 @@ public class SymbolReferenceModel extends AddressBasedTableModel { @Override protected TableColumnDescriptor createTableColumnDescriptor() { - TableColumnDescriptor descriptor = new TableColumnDescriptor(); + TableColumnDescriptor descriptor = new TableColumnDescriptor<>(); descriptor.addVisibleColumn( DiscoverableTableUtils.adaptColumForModel(this, new ReferenceFromAddressTableColumn()), @@ -139,7 +139,6 @@ public class SymbolReferenceModel extends AddressBasedTableModel { void symbolChanged(Symbol symbol) { if (currentSymbol != null && currentSymbol.equals(symbol)) { - setCurrentSymbol(symbol); return; } checkRefs(symbol); diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/symtable/SymbolTableModel.java b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/symtable/SymbolTableModel.java index 365a3699ee..6cf66491de 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/symtable/SymbolTableModel.java +++ b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/symtable/SymbolTableModel.java @@ -264,12 +264,13 @@ class SymbolTableModel extends AddressBasedTableModel { updateObject(Symbol); } else { + // the symbol may be in the table, as it could have passed the filter before the change removeObject(Symbol); } } void delete(List rowObjects) { - if (rowObjects == null || rowObjects.size() == 0) { + if (rowObjects == null || rowObjects.isEmpty()) { return; } @@ -530,7 +531,7 @@ class SymbolTableModel extends AddressBasedTableModel { private class SourceTableColumn extends AbstractProgramBasedDynamicTableColumn { - private GColumnRenderer renderer = new AbstractGColumnRenderer() { + private GColumnRenderer renderer = new AbstractGColumnRenderer<>() { @Override protected String getText(Object value) { if (value == null) { diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/symtable/SymbolTablePlugin.java b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/symtable/SymbolTablePlugin.java index 95d35fc0d6..2f0c82bbd9 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/symtable/SymbolTablePlugin.java +++ b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/symtable/SymbolTablePlugin.java @@ -44,6 +44,9 @@ import ghidra.util.table.GhidraTable; import ghidra.util.table.SelectionNavigationAction; import ghidra.util.table.actions.MakeProgramSelectionAction; import ghidra.util.task.SwingUpdateManager; +import ghidra.util.task.TaskMonitor; +import ghidra.util.worker.Job; +import ghidra.util.worker.Worker; import resources.Icons; import resources.ResourceManager; @@ -88,6 +91,13 @@ public class SymbolTablePlugin extends Plugin implements DomainObjectListener { private BlockModelService blockModelService; private SwingUpdateManager swingMgr; + /** + * A worker that will process domain object change event work off of the Swing thread. This + * solves the issue of db lock contention that can happen during analysis while this class + * attempts to get symbols from the db while processing a flurry of domain events. + */ + private Worker domainObjectWorker = Worker.createGuiWorker(); + public SymbolTablePlugin(PluginTool tool) { super(tool); @@ -124,6 +134,7 @@ public class SymbolTablePlugin extends Plugin implements DomainObjectListener { deleteAction.dispose(); makeSelectionAction.dispose(); + domainObjectWorker.dispose(); if (symProvider != null) { symProvider.dispose(); symProvider = null; @@ -165,17 +176,15 @@ public class SymbolTablePlugin extends Plugin implements DomainObjectListener { if (oldProg != null) { inspector.setProgram(null); oldProg.removeListener(this); + domainObjectWorker.clearAllJobs(); symProvider.setProgram(null, inspector); refProvider.setProgram(null, inspector); tool.contextChanged(symProvider); } currentProgram = newProg; if (newProg != null) { - currentProgram.addListener(this); - inspector.setProgram(currentProgram); - symProvider.setProgram(currentProgram, inspector); refProvider.setProgram(currentProgram, inspector); } @@ -184,17 +193,27 @@ public class SymbolTablePlugin extends Plugin implements DomainObjectListener { } } + boolean isBusy() { + return domainObjectWorker.isBusy() || symProvider.isBusy() || refProvider.isBusy(); + } + + private void reload() { + domainObjectWorker.clearAllJobs(); + symProvider.reload(); + refProvider.reload(); + } + @Override public void domainObjectChanged(DomainObjectChangedEvent ev) { - if (!symProvider.isVisible()) { + if (!symProvider.isVisible() && !refProvider.isVisible()) { return; } + if (ev.containsEvent(DomainObject.DO_OBJECT_RESTORED) || ev.containsEvent(ChangeManager.DOCR_MEMORY_BLOCK_ADDED) || ev.containsEvent(ChangeManager.DOCR_MEMORY_BLOCK_REMOVED)) { - symProvider.reload(); - refProvider.reload(); + reload(); return; } @@ -208,103 +227,74 @@ public class SymbolTablePlugin extends Plugin implements DomainObjectListener { } ProgramChangeRecord rec = (ProgramChangeRecord) doRecord; - Symbol symbol = null; - SymbolTable symbolTable = currentProgram.getSymbolTable(); switch (eventType) { case ChangeManager.DOCR_CODE_ADDED: case ChangeManager.DOCR_CODE_REMOVED: if (rec.getNewValue() instanceof Data) { - symbol = symbolTable.getPrimarySymbol(rec.getStart()); - if (symbol != null && symbol.isDynamic()) { - symProvider.symbolChanged(symbol); - refProvider.symbolChanged(symbol); - } + domainObjectWorker.schedule( + new CodeAddedRemoveJob(currentProgram, rec.getStart())); } break; case ChangeManager.DOCR_SYMBOL_ADDED: + Address addAddr = rec.getStart(); - Symbol primaryAtAdd = symbolTable.getPrimarySymbol(addAddr); - if (primaryAtAdd != null && primaryAtAdd.isDynamic()) { - symProvider.symbolRemoved(primaryAtAdd); - } - symbol = (Symbol) rec.getNewValue(); - symProvider.symbolAdded(symbol); - refProvider.symbolAdded(symbol); + Symbol symbol = (Symbol) rec.getNewValue(); + domainObjectWorker.schedule( + new SymbolAddedJob(currentProgram, symbol, addAddr)); break; case ChangeManager.DOCR_SYMBOL_REMOVED: + Address removeAddr = rec.getStart(); Long symbolID = (Long) rec.getNewValue(); - Symbol removedSymbol = - symbolTable.createSymbolPlaceholder(removeAddr, symbolID); - symProvider.symbolRemoved(removedSymbol); - refProvider.symbolRemoved(removedSymbol); - Symbol primaryAtRemove = symbolTable.getPrimarySymbol(removeAddr); - if (primaryAtRemove != null && primaryAtRemove.isDynamic()) { - symProvider.symbolAdded(primaryAtRemove); - } + domainObjectWorker.schedule( + new SymbolRemovedJob(currentProgram, removeAddr, symbolID)); break; case ChangeManager.DOCR_SYMBOL_RENAMED: case ChangeManager.DOCR_SYMBOL_SCOPE_CHANGED: case ChangeManager.DOCR_SYMBOL_DATA_CHANGED: + symbol = (Symbol) rec.getObject(); - if (symbol.checkIsValid()) { // symbol may have been removed (e.g., parameter) - symProvider.symbolChanged(symbol); - refProvider.symbolChanged(symbol); - } + domainObjectWorker.schedule(new SymbolChangedJob(currentProgram, symbol)); break; case ChangeManager.DOCR_SYMBOL_SOURCE_CHANGED: + symbol = (Symbol) rec.getObject(); - symProvider.symbolChanged(symbol); + domainObjectWorker.schedule(new SymbolSourceChangedJob(currentProgram, symbol)); break; case ChangeManager.DOCR_SYMBOL_SET_AS_PRIMARY: + symbol = (Symbol) rec.getNewValue(); - symProvider.symbolChanged(symbol); - Symbol oldSymbol = (Symbol) rec.getOldValue(); - if (oldSymbol != null) { - symProvider.symbolChanged(oldSymbol); - } + Symbol oldPrimarySymbol = (Symbol) rec.getOldValue(); + domainObjectWorker.schedule( + new SymbolSetAsPrimaryJob(currentProgram, symbol, oldPrimarySymbol)); break; case ChangeManager.DOCR_SYMBOL_ASSOCIATION_ADDED: case ChangeManager.DOCR_SYMBOL_ASSOCIATION_REMOVED: break; case ChangeManager.DOCR_MEM_REFERENCE_ADDED: - Reference ref = (Reference) rec.getObject(); - symbol = symbolTable.getSymbol(ref); - if (symbol != null) { - symProvider.symbolChanged(symbol); - refProvider.symbolChanged(symbol); - } - break; - case ChangeManager.DOCR_MEM_REFERENCE_REMOVED: - ref = (Reference) rec.getObject(); - Address toAddr = ref.getToAddress(); - if (toAddr.isMemoryAddress()) { - symbol = symbolTable.getSymbol(ref); - if (symbol == null) { - long id = symbolTable.getDynamicSymbolID(ref.getToAddress()); - removedSymbol = symbolTable.createSymbolPlaceholder(toAddr, id); - symProvider.symbolRemoved(removedSymbol); - } - else { - refProvider.symbolChanged(symbol); - } - } + Reference ref = (Reference) rec.getObject(); + domainObjectWorker.schedule(new ReferenceAddedJob(currentProgram, ref)); + break; + + case ChangeManager.DOCR_MEM_REFERENCE_REMOVED: + + ref = (Reference) rec.getObject(); + domainObjectWorker.schedule(new ReferenceRemovedJob(currentProgram, ref)); break; case ChangeManager.DOCR_EXTERNAL_ENTRY_POINT_ADDED: case ChangeManager.DOCR_EXTERNAL_ENTRY_POINT_REMOVED: - Symbol[] symbols = symbolTable.getSymbols(rec.getStart()); - for (Symbol element : symbols) { - symProvider.symbolChanged(element); - refProvider.symbolChanged(element); - } + + Address address = rec.getStart(); + domainObjectWorker.schedule( + new ExternalEntryChangedJob(currentProgram, address)); break; } } @@ -336,7 +326,8 @@ public class SymbolTablePlugin extends Plugin implements DomainObjectListener { } } - void closeReferenceProvider() { + void symbolProviderClosed() { + domainObjectWorker.clearAllJobs(); if (refProvider != null) { refProvider.closeComponent(); } @@ -514,4 +505,268 @@ public class SymbolTablePlugin extends Plugin implements DomainObjectListener { action.setSelected(false); action.setSelected(true); } + +//================================================================================================== +// Table Update Jobs +//================================================================================================== + + private abstract class AbstractSymbolUpdateJob extends Job { + + protected Program program; + + AbstractSymbolUpdateJob(Program program) { + this.program = program; + } + + @Override + public final void run(TaskMonitor taskMonitor) { + if (program != currentProgram) { + return; + } + doRun(); + } + + protected abstract void doRun(); + } + + private class CodeAddedRemoveJob extends AbstractSymbolUpdateJob { + + private Address start; + + CodeAddedRemoveJob(Program program, Address start) { + super(program); + this.start = start; + } + + @Override + protected void doRun() { + + if (!symProvider.isShowingDynamicSymbols()) { + return; + } + + // Note: this code *should* be checking the entire address range to handle the case + // where large address range was cleared. This implementation will handle the + // case where individual code units are cleared. This feature has been this way + // for many years. The assumption is that most users are not showing dynamic + // symbols often, especially not when performing analysis or clearing large + // address ranges. Checking each address of the changed range is very slow. + // This code will need to be updated in the future if we decide updating the + // dynamic symbols in the symbol table is worth the cost. For now, if the table + // becomes out-of-date, then user can simply close and re-open the table to + // trigger an update. + SymbolTable symbolTable = currentProgram.getSymbolTable(); + Symbol symbol = symbolTable.getPrimarySymbol(start); + if (symbol != null && symbol.isDynamic()) { + symProvider.symbolChanged(symbol); + refProvider.symbolChanged(symbol); + } + } + } + + private class SymbolAddedJob extends AbstractSymbolUpdateJob { + + private Symbol symbol; + private Address address; + + SymbolAddedJob(Program program, Symbol symbol, Address address) { + super(program); + this.symbol = symbol; + this.address = address; + } + + @Override + protected void doRun() { + + symProvider.symbolAdded(symbol); + refProvider.symbolAdded(symbol); + + if (!symProvider.isShowingDynamicSymbols()) { + return; + } + + SymbolTable symbolTable = program.getSymbolTable(); + Symbol primaryAtAdd = symbolTable.getPrimarySymbol(address); + if (primaryAtAdd != null && primaryAtAdd.isDynamic()) { + symProvider.symbolRemoved(primaryAtAdd); + refProvider.symbolRemoved(primaryAtAdd); + } + } + } + + private class SymbolRemovedJob extends AbstractSymbolUpdateJob { + + private long symbolId; + private Address address; + + SymbolRemovedJob(Program program, Address address, long symbolId) { + super(program); + this.address = address; + this.symbolId = symbolId; + } + + @Override + protected void doRun() { + + SymbolTable symbolTable = currentProgram.getSymbolTable(); + Symbol removedSymbol = + symbolTable.createSymbolPlaceholder(address, symbolId); + symProvider.symbolRemoved(removedSymbol); + refProvider.symbolRemoved(removedSymbol); + + if (!symProvider.isShowingDynamicSymbols()) { + return; + } + + Symbol primaryAtRemove = symbolTable.getPrimarySymbol(address); + if (primaryAtRemove != null && primaryAtRemove.isDynamic()) { + symProvider.symbolAdded(primaryAtRemove); + refProvider.symbolAdded(primaryAtRemove); + } + } + } + + private class SymbolChangedJob extends AbstractSymbolUpdateJob { + + private Symbol symbol; + + SymbolChangedJob(Program program, Symbol symbol) { + super(program); + this.symbol = symbol; + } + + @Override + protected void doRun() { + + // Note: should not need this check--the provider should be built to handle this + // if (symbol.checkIsValid()) + symProvider.symbolChanged(symbol); + refProvider.symbolChanged(symbol); + } + } + + private class SymbolSourceChangedJob extends AbstractSymbolUpdateJob { + + private Symbol symbol; + + SymbolSourceChangedJob(Program program, Symbol symbol) { + super(program); + this.symbol = symbol; + } + + @Override + protected void doRun() { + symProvider.symbolChanged(symbol); + } + } + + private class SymbolSetAsPrimaryJob extends AbstractSymbolUpdateJob { + + private Symbol symbol; + private Symbol oldPrimarySymbol; + + SymbolSetAsPrimaryJob(Program program, Symbol symbol, Symbol oldPrimarySymbol) { + super(program); + this.symbol = symbol; + this.oldPrimarySymbol = oldPrimarySymbol; + } + + @Override + protected void doRun() { + + symProvider.symbolChanged(symbol); + if (oldPrimarySymbol != null) { + symProvider.symbolChanged(oldPrimarySymbol); + } + } + } + + private class ReferenceAddedJob extends AbstractSymbolUpdateJob { + + private Reference reference; + + ReferenceAddedJob(Program program, Reference reference) { + super(program); + this.reference = reference; + } + + @Override + protected void doRun() { + + Address toAddr = reference.getToAddress(); + boolean isValid = toAddr.isMemoryAddress() || toAddr.isExternalAddress(); + if (!isValid) { + return; + } + + SymbolTable symbolTable = program.getSymbolTable(); + Symbol symbol = symbolTable.getSymbol(reference); + if (symbol == null) { + return; + } + + if (!symProvider.isShowingDynamicSymbols() && symbol.isDynamic()) { + return; + } + + symProvider.symbolChanged(symbol); + refProvider.symbolChanged(symbol); + } + } + + private class ReferenceRemovedJob extends AbstractSymbolUpdateJob { + + private Reference reference; + + ReferenceRemovedJob(Program program, Reference reference) { + super(program); + this.reference = reference; + } + + @Override + protected void doRun() { + + Address toAddr = reference.getToAddress(); + boolean isValid = toAddr.isMemoryAddress() || toAddr.isExternalAddress(); + if (!isValid) { + return; + } + + SymbolTable symbolTable = program.getSymbolTable(); + Symbol symbol = symbolTable.getSymbol(reference); + if (symbol != null) { + symProvider.symbolChanged(symbol); + refProvider.symbolChanged(symbol); + } + + if (symProvider.isShowingDynamicSymbols()) { + long id = symbolTable.getDynamicSymbolID(reference.getToAddress()); + Symbol removedSymbol = symbolTable.createSymbolPlaceholder(toAddr, id); + symProvider.symbolRemoved(removedSymbol); + refProvider.symbolRemoved(removedSymbol); + } + } + } + + private class ExternalEntryChangedJob extends AbstractSymbolUpdateJob { + + private Address address; + + ExternalEntryChangedJob(Program program, Address address) { + super(program); + this.address = address; + } + + @Override + protected void doRun() { + + SymbolTable symbolTable = program.getSymbolTable(); + Symbol[] symbols = symbolTable.getSymbols(address); + for (Symbol element : symbols) { + symProvider.symbolChanged(element); + refProvider.symbolChanged(element); + } + } + } + } diff --git a/Ghidra/Features/Base/src/test.slow/java/ghidra/app/plugin/core/symtable/SymbolTablePluginTest.java b/Ghidra/Features/Base/src/test.slow/java/ghidra/app/plugin/core/symtable/SymbolTablePluginTest.java index 6cd1458227..4d2fde232b 100644 --- a/Ghidra/Features/Base/src/test.slow/java/ghidra/app/plugin/core/symtable/SymbolTablePluginTest.java +++ b/Ghidra/Features/Base/src/test.slow/java/ghidra/app/plugin/core/symtable/SymbolTablePluginTest.java @@ -37,7 +37,6 @@ import docking.action.DockingActionIf; import docking.action.ToggleDockingAction; import docking.widgets.filter.*; import docking.widgets.table.*; -import docking.widgets.table.threaded.ThreadedTableModel; import ghidra.app.cmd.label.AddLabelCmd; import ghidra.app.cmd.label.CreateNamespacesCmd; import ghidra.app.cmd.refs.RemoveReferenceCmd; @@ -273,7 +272,7 @@ public class SymbolTablePluginTest extends AbstractGhidraHeadedIntegrationTest { pressButtonByText(filterDialog1, "OK"); waitForSwing(); - waitForNotBusy(symbolTable); + waitForNotBusy(); // // Functions: 'ghidra', 'func_with_parms' @@ -294,7 +293,7 @@ public class SymbolTablePluginTest extends AbstractGhidraHeadedIntegrationTest { pressButtonByText(filterDialog2, "OK"); waitForSwing(); - waitForNotBusy(symbolTable); + waitForNotBusy(); // // Entry Points: 'entry' @@ -314,7 +313,7 @@ public class SymbolTablePluginTest extends AbstractGhidraHeadedIntegrationTest { }); pressButtonByText(filterDialog3, "OK"); waitForSwing(); - waitForNotBusy(symbolTable); + waitForNotBusy(); // // Locals: 'AnotherLocal', 'MyLocal' @@ -348,7 +347,7 @@ public class SymbolTablePluginTest extends AbstractGhidraHeadedIntegrationTest { public void testEditing() throws Exception { openProgram("sample"); - waitForNotBusy(symbolTable); + waitForNotBusy(); String symbolName = "ghidra"; int row = findRow(symbolName); @@ -367,7 +366,7 @@ public class SymbolTablePluginTest extends AbstractGhidraHeadedIntegrationTest { myTypeText(editor, ".Is.Cool"); runSwing(() -> symbolTable.editingStopped(new ChangeEvent(symbolTable))); - waitForNotBusy(symbolTable); + waitForNotBusy(); assertTrue(!symbolTable.isEditing()); @@ -391,7 +390,7 @@ public class SymbolTablePluginTest extends AbstractGhidraHeadedIntegrationTest { st.createLabel(sample.getNewAddress(0x01008500), "abc123", SourceType.USER_DEFINED); }); - waitForNotBusy(symbolTable); + waitForNotBusy(); int testTimeoutMs = 100; symbolTable.setAutoLookupTimeout(testTimeoutMs); @@ -432,7 +431,7 @@ public class SymbolTablePluginTest extends AbstractGhidraHeadedIntegrationTest { assertTrue(deleteAction.isEnabled()); performAction(deleteAction, true); - waitForNotBusy(symbolTable); + waitForNotBusy(); assertNull(getUniqueSymbol(program, "ghidra")); Symbol myLocalSymbol = getUniqueSymbol(program, "MyLocal"); @@ -462,7 +461,7 @@ public class SymbolTablePluginTest extends AbstractGhidraHeadedIntegrationTest { assertNull("Delete action did not delete symbol: " + anotherLocalSymbol, anotherLocalSymbol);// AnotherLocal should have been promoted to global since user defined. - waitForNotBusy(symbolTable); + waitForNotBusy(); // 1 Data label removed int expected = rowCount - 1; @@ -566,7 +565,7 @@ public class SymbolTablePluginTest extends AbstractGhidraHeadedIntegrationTest { assertFalse(clearPinnedAction.isEnabledForContext(actionContext)); performAction(setPinnedAction, actionContext, true); - waitForNotBusy(symbolTable); + waitForNotBusy(); for (int selectedRow : selectedRows) { Symbol symbol = getSymbol(selectedRow); assertTrue(symbol.isPinned()); @@ -611,7 +610,7 @@ public class SymbolTablePluginTest extends AbstractGhidraHeadedIntegrationTest { return st.createLabel(sample.getNewAddress(0x01007000), "Zeus", SourceType.USER_DEFINED); }); - waitForNotBusy(symbolTable); + waitForNotBusy(); assertEquals(rowCount + 1, symbolTable.getRowCount()); assertTrue(symbolModel.getRowIndex(sym) >= 0); @@ -619,7 +618,7 @@ public class SymbolTablePluginTest extends AbstractGhidraHeadedIntegrationTest { return st.createLabel(sample.getNewAddress(0x01007100), "Athena", SourceType.USER_DEFINED); }); - waitForNotBusy(symbolTable); + waitForNotBusy(); assertEquals(rowCount + 2, symbolTable.getRowCount()); assertTrue(symbolModel.getRowIndex(sym) >= 0); } @@ -648,7 +647,7 @@ public class SymbolTablePluginTest extends AbstractGhidraHeadedIntegrationTest { return st.createLabel(sample.getNewAddress(0x01007000), "saaaa", SourceType.USER_DEFINED); }); - waitForNotBusy(symbolTable); + waitForNotBusy(); assertTrue(symbolModel.getRowIndex(sym) >= 0); // make sure we added one while the filter is on @@ -672,7 +671,7 @@ public class SymbolTablePluginTest extends AbstractGhidraHeadedIntegrationTest { pressButtonByText(filterDialog1, "OK"); waitForSwing(); - waitForNotBusy(symbolTable); + waitForNotBusy(); // // Functions: 'ghidra', 'func_with_parms' @@ -696,7 +695,7 @@ public class SymbolTablePluginTest extends AbstractGhidraHeadedIntegrationTest { assertNull(getUniqueSymbol(program, "EXT_00000051")); tx(program, () -> st.removeSymbolSpecial(sym)); - waitForNotBusy(symbolTable); + waitForNotBusy(); // entry symbol replaced by dynamic External Entry symbol assertNull(getUniqueSymbol(program, "entry")); @@ -723,7 +722,7 @@ public class SymbolTablePluginTest extends AbstractGhidraHeadedIntegrationTest { rm.setPrimary(ref, true); }); - waitForNotBusy(symbolTable); + waitForNotBusy(); row = symbolModel.getRowIndex(s); @@ -752,7 +751,7 @@ public class SymbolTablePluginTest extends AbstractGhidraHeadedIntegrationTest { manager.delete(ref); }); - waitForNotBusy(symbolTable); + waitForNotBusy(); refCount = getRefCount(row); assertNotNull(refCount); @@ -782,19 +781,19 @@ public class SymbolTablePluginTest extends AbstractGhidraHeadedIntegrationTest { ClearCmd cmd = new ClearCmd(program.getMemory(), new ClearOptions()); applyCmd(program, cmd); waitForBusyTool(tool); - waitForNotBusy(symbolTable); + waitForNotBusy(); // Externals are not cleared int clearedRowCount = 3; assertEquals(clearedRowCount, symbolTable.getRowCount()); undo(program); - waitForNotBusy(symbolTable); + waitForNotBusy(); assertEquals(startRowCount, symbolTable.getRowCount()); redo(program); - waitForNotBusy(symbolTable); + waitForNotBusy(); assertEquals(clearedRowCount, symbolTable.getRowCount()); } @@ -815,7 +814,7 @@ public class SymbolTablePluginTest extends AbstractGhidraHeadedIntegrationTest { pressButtonByText(filterDialog, "OK"); waitForSwing(); - waitForNotBusy(symbolTable); + waitForNotBusy(); waitForSwing(); } @@ -880,8 +879,8 @@ public class SymbolTablePluginTest extends AbstractGhidraHeadedIntegrationTest { performAction(refsToAction, true); waitForSwing(); - waitForNotBusy(symbolTable); - waitForNotBusy(referenceTable); + waitForNotBusy(); + waitForNotBusy(); assertEquals(4, referenceTable.getRowCount()); assertReferencesAddressColumnValue(0, 0x54); @@ -899,8 +898,8 @@ public class SymbolTablePluginTest extends AbstractGhidraHeadedIntegrationTest { waitForSwing(); - waitForNotBusy(symbolTable); - waitForNotBusy(referenceTable); + waitForNotBusy(); + waitForNotBusy(); assertEquals(3, referenceTable.getRowCount()); assertReferencesAddressColumnValue(0, 0x53); @@ -917,8 +916,8 @@ public class SymbolTablePluginTest extends AbstractGhidraHeadedIntegrationTest { waitForSwing(); - waitForNotBusy(symbolTable); - waitForNotBusy(referenceTable); + waitForNotBusy(); + waitForNotBusy(); assertEquals(2, referenceTable.getRowCount()); // data @@ -1085,33 +1084,33 @@ public class SymbolTablePluginTest extends AbstractGhidraHeadedIntegrationTest { addLabel("bob", null, addr("010058f6")); addLabel("bob", "billy", addr("01005917")); - waitForNotBusy(symbolTable); + waitForNotBusy(); int updatedRowCount = symbolTable.getRowCount(); assertEquals(rowCount + 2, updatedRowCount); // test ascending runSwing(() -> TableUtils.columnSelected(symbolTable, 0)); - waitForNotBusy(symbolTable); + waitForNotBusy(); setFilterOptions(TextFilterStrategy.STARTS_WITH, false); myTypeText(textField, "bo"); - waitForNotBusy(symbolTable); + waitForNotBusy(); // make sure both 'bob's are in the table assertEquals("Did not find two bobs.", 2, symbolTable.getRowCount()); modelMatchesIgnoringCase("bob"); myTypeText(textField, "b"); - waitForNotBusy(symbolTable); + waitForNotBusy(); assertEquals("Did not find two bobs.", 2, symbolTable.getRowCount()); modelMatchesIgnoringCase("bob"); // test descending runSwing(() -> TableUtils.columnSelected(symbolTable, 0)); - waitForNotBusy(symbolTable); + waitForNotBusy(); assertEquals("Did not find two bobs in descending order.", 2, symbolTable.getRowCount()); modelMatchesIgnoringCase("bob"); @@ -1250,12 +1249,12 @@ public class SymbolTablePluginTest extends AbstractGhidraHeadedIntegrationTest { // use the version of triggerText that allows us to consume the event directly, bypassing // the focus system triggerText(symbolTable, text, consumer); - waitForNotBusy(symbolTable); + waitForNotBusy(); } private void setName(Symbol symbol, String name, SourceType type) throws Exception { tx(program, () -> symbol.setName(name, SourceType.DEFAULT)); - waitForNotBusy(symbolTable); + waitForNotBusy(); } private Symbol getSymbol(int row) { @@ -1315,7 +1314,7 @@ public class SymbolTablePluginTest extends AbstractGhidraHeadedIntegrationTest { private void setFilterOptions(TextFilterStrategy filterStrategy, boolean caseSensitive) throws Exception { filterPanel.setFilterOptions(new FilterOptions(filterStrategy, true, caseSensitive, false)); - waitForNotBusy(symbolTable); + waitForNotBusy(); } private JTextField getFilterTextField() { @@ -1351,7 +1350,7 @@ public class SymbolTablePluginTest extends AbstractGhidraHeadedIntegrationTest { pressButtonByText(filterDialog, "OK", true); - waitForNotBusy(symbolTable); + waitForNotBusy(); } private int getRowForSymbol(Symbol symbol) { @@ -1390,7 +1389,7 @@ public class SymbolTablePluginTest extends AbstractGhidraHeadedIntegrationTest { Command command = new AddLabelCmd(address, label, namespace, SourceType.USER_DEFINED); tool.execute(command, program); - waitForNotBusy(symbolTable); + waitForNotBusy(); } private Address addr(String address) { @@ -1399,7 +1398,7 @@ public class SymbolTablePluginTest extends AbstractGhidraHeadedIntegrationTest { private void myTypeText(Component c, String text) throws Exception { triggerText(c, text); - waitForNotBusy(symbolTable); + waitForNotBusy(); } private void deleteTextFieldText(JTextField textField) { @@ -1410,7 +1409,7 @@ public class SymbolTablePluginTest extends AbstractGhidraHeadedIntegrationTest { waitForSwing(); try { - waitForNotBusy(symbolTable); + waitForNotBusy(); } catch (Exception exc) { // we don't care @@ -1419,7 +1418,7 @@ public class SymbolTablePluginTest extends AbstractGhidraHeadedIntegrationTest { private void typeBackspaceOnComponent(Component component) throws Exception { triggerActionKey(component, 0, KeyEvent.VK_BACK_SPACE); - waitForNotBusy(symbolTable); + waitForNotBusy(); } // makes sure that all of the symbols in the model start with the given string @@ -1459,10 +1458,9 @@ public class SymbolTablePluginTest extends AbstractGhidraHeadedIntegrationTest { } } - private void waitForNotBusy(GTable table) throws Exception { + private void waitForNotBusy() throws Exception { waitForProgram(program); - ThreadedTableModel model = (ThreadedTableModel) table.getModel(); - waitForTableModel(model); + waitForCondition(() -> !plugin.isBusy()); } private void openProgram(String name) throws Exception { @@ -1583,9 +1581,9 @@ public class SymbolTablePluginTest extends AbstractGhidraHeadedIntegrationTest { filter.setFilter("Function Labels", true); filter.setFilter("Default (Functions)", true); filter.setFilter("Default (Labels)", true); - symbolModel.setFilter(filter); + runSwing(() -> symbolModel.setFilter(filter)); - waitForNotBusy(symbolTable); + waitForNotBusy(); sortAscending(SymbolTableModel.LABEL_COL); } diff --git a/Ghidra/Framework/Docking/src/main/java/docking/widgets/table/threaded/TableData.java b/Ghidra/Framework/Docking/src/main/java/docking/widgets/table/threaded/TableData.java index c4ac89b99e..72b24ff65d 100644 --- a/Ghidra/Framework/Docking/src/main/java/docking/widgets/table/threaded/TableData.java +++ b/Ghidra/Framework/Docking/src/main/java/docking/widgets/table/threaded/TableData.java @@ -153,12 +153,32 @@ public class TableData implements Iterable { return -1; } - boolean remove(ROW_OBJECT o) { + boolean remove(ROW_OBJECT t) { if (source != null) { - source.remove(o); + source.remove(t); } - return data.remove(o); + if (sortContext.isUnsorted()) { + return data.remove(t); // no sort; cannot binary search + } + + Comparator comparator = sortContext.getComparator(); + int index = Collections.binarySearch(data, t, comparator); + if (index > 0) { + data.remove(index); + return true; + } + + // + // At this point we have one of 2 conditions: the object is not in the list, or the object + // does not work with the current sort comparator. + // + // There are cases where the comparator will not work for the object handed to this method, + // such as when some db objects get deleted and the client uses a proxy object to perform + // the delete. To handle these odd cases, we still have to brute-force search. If this + // proves to be a bottleneck, then we can update how we handle item removal. + // + return data.remove(t); } /**