Merge remote-tracking branch 'origin/GT-3457-dragonmacher-gtree-cannot-find-new-node'

Ghidra/Framework/Generic/src/main/java/generic/test/AbstractGTest.java

@@ -15,13 +15,13 @@
  */
 package generic.test;
 
-import static org.junit.Assert.assertNull;
-import static org.junit.Assert.fail;
+import static org.junit.Assert.*;
 
 import java.io.File;
 import java.util.*;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.function.BooleanSupplier;
 import java.util.function.Supplier;
 
@@ -360,6 +360,17 @@ public abstract class AbstractGTest {
 		}
 	}
 
+	/**
+	 * Waits for the given AtomicBoolean to return true.  This is a convenience method for 
+	 * {@link #waitFor(BooleanSupplier)}.
+	 *
+	 * @param ab the atomic boolean
+	 * @throws AssertionFailedError if the condition is not met within the timeout period
+	 */
+	public static void waitFor(AtomicBoolean ab) throws AssertionFailedError {
+		waitForCondition(() -> ab.get());
+	}
+
 	/**
 	 * Waits for the given condition to return true
 	 *

Ghidra/Framework/Generic/src/main/java/generic/timer/ExpiringSwingTimer.java

@@ -0,0 +1,135 @@
+/* ###
+ * IP: GHIDRA
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * 
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ * 
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package generic.timer;
+
+import java.util.Objects;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.function.BooleanSupplier;
+
+/**
+ * This class allows clients to run swing action at some point in the future, when the given 
+ * condition is met, allowing for the task to timeout.  While this class implements the
+ * {@link GhidraTimer} interface, it is really meant to be used to execute a code snippet one
+ * time at some point in the future.  
+ * 
+ * <p>Both the call to check for readiness and the actual client code will be run on the Swing
+ * thread.
+ */
+public class ExpiringSwingTimer extends GhidraSwingTimer {
+
+	private long startMs = System.currentTimeMillis();
+	private int expireMs;
+	private BooleanSupplier isReady;
+	private ExpiringTimerCallback expiringTimerCallback = new ExpiringTimerCallback();
+	private TimerCallback clientCallback;
+	private AtomicBoolean didRun = new AtomicBoolean();
+
+	/**
+	 * Runs the given client runnable when the given condition returns true.  The returned timer 
+	 * will be running.
+	 * 
+	 * <p>Once the timer has performed the work, any calls to start the returned timer will 
+	 * not perform any work.  You can check {@link #didRun()} to see if the work has been completed.
+	 * 
+	 * @param isReady true if the code should be run
+	 * @param expireMs the amount of time past which the code will not be run
+	 * @param runnable the code to run
+	 * @return the timer object that is running, which will execute the given code when ready
+	 */
+	public static ExpiringSwingTimer runWhen(BooleanSupplier isReady,
+			int expireMs,
+			Runnable runnable) {
+
+		// Note: we could let the client specify the period, but that would add an extra argument
+		//       to this method. For now, just use something reasonable.
+		int delay = 250;
+		ExpiringSwingTimer timer =
+			new ExpiringSwingTimer(delay, expireMs, isReady, runnable);
+		timer.start();
+		return timer;
+	}
+
+	/**
+	 * Constructor
+	 * 
+	 * <p>Note: this class sets the parent's initial delay to 0.  This is to allow the client 
+	 * code to be executed without delay when the ready condition is true.
+	 * 
+	 * @param delay the delay between calls to check <code>isReady</code>
+	 * @param isReady true if the code should be run
+	 * @param expireMs the amount of time past which the code will not be run
+	 * @param runnable the code to run
+	 */
+	public ExpiringSwingTimer(int delay, int expireMs, BooleanSupplier isReady,
+			Runnable runnable) {
+		super(0, delay, null);
+		this.expireMs = expireMs;
+		this.isReady = isReady;
+		this.clientCallback = () -> runnable.run();
+		super.setTimerCallback(expiringTimerCallback);
+		setRepeats(true);
+	}
+
+	/**
+	 * Returns true if the client runnable was run 
+	 * @return true if the client runnable was run
+	 */
+	public boolean didRun() {
+		return didRun.get();
+	}
+
+	@Override
+	public void start() {
+		if (didRun() || isExpired()) {
+			return;
+		}
+
+		super.start();
+	}
+
+	/**
+	 * Returns true the initial expiration period has passed
+	 * @return true if expired
+	 */
+	public boolean isExpired() {
+		long now = System.currentTimeMillis();
+		int elapsed = (int) (now - startMs);
+		return elapsed > expireMs;
+	}
+
+	@Override
+	public void setTimerCallback(TimerCallback callback) {
+		// overridden to ensure clients cannot overwrite out wrapping callback
+		this.clientCallback = Objects.requireNonNull(callback);
+	}
+
+	private class ExpiringTimerCallback implements TimerCallback {
+
+		@Override
+		public void timerFired() {
+
+			if (isReady.getAsBoolean()) {
+				clientCallback.timerFired();
+				didRun.set(true);
+				stop();
+				return;
+			}
+			else if (isExpired()) {
+				stop();
+			}
+		}
+	}
+}

Ghidra/Framework/Generic/src/main/java/generic/timer/GhidraSwingTimer.java

@@ -1,6 +1,5 @@
 /* ###
  * IP: GHIDRA
- * REVIEWED: YES
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -22,67 +21,78 @@ import java.awt.event.ActionListener;
 import javax.swing.Timer;
 
 public class GhidraSwingTimer implements GhidraTimer, ActionListener {
-	private Timer timer;
+
+	Timer timer;
 	private TimerCallback callback;
-	
+
 	public GhidraSwingTimer() {
 		this(100, null);
 	}
-	
+
 	public GhidraSwingTimer(int delay, TimerCallback callback) {
-		this(delay,delay,callback);
+		this(delay, delay, callback);
 	}
-	
+
 	public GhidraSwingTimer(int initialDelay, int delay, TimerCallback callback) {
 		this.callback = callback;
 		timer = new Timer(delay, this);
-		timer.setInitialDelay(delay);
+		timer.setInitialDelay(initialDelay);
 	}
-	
+
+	@Override
 	public void actionPerformed(ActionEvent e) {
 		if (callback != null) {
 			callback.timerFired();
 		}
 	}
-	
+
+	@Override
 	public int getDelay() {
 		return timer.getDelay();
 	}
 
+	@Override
 	public int getInitialDelay() {
 		return timer.getInitialDelay();
 	}
 
+	@Override
 	public boolean isRepeats() {
 		return timer.isRepeats();
 	}
 
+	@Override
 	public void setDelay(int delay) {
 		timer.setDelay(delay);
 	}
 
+	@Override
 	public void setInitialDelay(int initialDelay) {
 		timer.setInitialDelay(initialDelay);
 	}
 
+	@Override
 	public void setRepeats(boolean repeats) {
 		timer.setRepeats(repeats);
 	}
 
+	@Override
 	public void setTimerCallback(TimerCallback callback) {
 		this.callback = callback;
 	}
 
+	@Override
 	public void start() {
 		timer.start();
 	}
 
+	@Override
 	public void stop() {
 		timer.stop();
 	}
 
+	@Override
 	public boolean isRunning() {
 		return timer.isRunning();
 	}
-
 }

Ghidra/Framework/Generic/src/main/java/ghidra/util/task/SwingUpdateManager.java

@@ -15,8 +15,6 @@
  */
 package ghidra.util.task;
 
-import java.util.concurrent.atomic.AtomicInteger;
-
 import javax.swing.Timer;
 
 import ghidra.util.Msg;
@@ -50,13 +48,8 @@ import utilities.util.reflection.ReflectionUtilities;
  *
  * <P> This class is safe to use in a multi-threaded environment.   State variables are guarded
  * via synchronization on this object.   The Swing thread is used to perform updates, which
- * guarantees that only one update will happen at a time.  There is one state variable,
- * the {@link #workCount}, that is changed both in the synchronized blocks and the Swing thread
- * which is an atomic variable.  This variable must be updated/incremented when the
- * synchronized variables are cleared to prevent {@link #isBusy()} from returning false when
- * there is a gap between 'work posted' and 'work execution'.
+ * guarantees that only one update will happen at a time.  
  */
-
 public class SwingUpdateManager {
 	private static final long NONE = 0;
 	public static final int DEFAULT_MAX_DELAY = 30000;
@@ -78,11 +71,13 @@ public class SwingUpdateManager {
 	private long bufferingStartTime;
 	private boolean disposed = false;
 
-	// this is the number of times we will be calling work
-	private AtomicInteger workCount = new AtomicInteger();
+	// This is true when work has begun and is not finished.  This is only mutated on the 
+	// Swing thread, but is read by other threads.
+	private boolean isWorking;
 
 	/**
-	 * Constructs a new SwingUpdateManager.
+	 * Constructs a new SwingUpdateManager with default values for min and max delay.  See
+	 * {@link #DEFAULT_MIN_DELAY} and {@value #DEFAULT_MAX_DELAY}.
 	 *
 	 * @param r the runnable that performs the client work.
 	 */
@@ -171,7 +166,7 @@ public class SwingUpdateManager {
 		}
 
 		requestTime = System.currentTimeMillis();
-		bufferingStartTime = bufferingStartTime == 0 ? requestTime : bufferingStartTime;
+		bufferingStartTime = bufferingStartTime == NONE ? requestTime : bufferingStartTime;
 		scheduleCheckForWork();
 	}
 
@@ -219,15 +214,15 @@ public class SwingUpdateManager {
 	}
 
 	/**
-	 * Returns true if any work is being performed or if there is buffered work.
-	 * @return true if any work is being performed or if there is buffered work.
+	 * Returns true if any work is being performed or if there is buffered work
+	 * @return true if any work is being performed or if there is buffered work
 	 */
 	public synchronized boolean isBusy() {
 		if (disposed) {
 			return false;
 		}
 
-		return requestTime != NONE || workCount.get() != 0;
+		return requestTime != NONE || isWorking;
 	}
 
 	public synchronized void dispose() {
@@ -253,21 +248,25 @@ public class SwingUpdateManager {
 			"\tname: " + name + "\n" +
 			"\tcreator: " + inceptionInformation + " ("+System.identityHashCode(this)+")\n" +
 			"\trequest time: "+requestTime + "\n" +
-			"\twork count: " + workCount.get() + "\n" +
+			"\twork count: " + isWorking + "\n" +
 		"}";
 		//@formatter:on
 	}
 
+	// note: this is called on the Swing thread
 	private void checkForWork() {
+
 		if (shouldDoWork()) {
 			doWork();
 		}
 	}
 
+	// note: this is called on the Swing thread
 	private synchronized boolean shouldDoWork() {
 
-		// If no pending request, exit without restarting timer.
+		// If no pending request, exit without restarting timer
 		if (requestTime == NONE) {
+			bufferingStartTime = NONE; // The timer has fired and there is no pending work
 			return false;
 		}
 
@@ -275,7 +274,7 @@ public class SwingUpdateManager {
 		if (isTimeToWork(now)) {
 			bufferingStartTime = now;
 			requestTime = NONE;
-			workCount.incrementAndGet();
+			isWorking = true;
 			return true;
 		}
 
@@ -304,6 +303,7 @@ public class SwingUpdateManager {
 		return false;
 	}
 
+	// note: this is called on the Swing thread
 	private void doWork() {
 		try {
 			clientRunnable.run();
@@ -313,9 +313,11 @@ public class SwingUpdateManager {
 			Msg.showError(this, null, "Unexpected Exception",
 				"Unexpected exception in Swing Update Manager", t);
 		}
-		finally {
-			workCount.decrementAndGet();
-		}
+
+		isWorking = false;
+
+		// we need to clear the buffering flag after the minDelay has passed, so start the timer
+		scheduleCheckForWork();
 	}
 
 //==================================================================================================

Ghidra/Framework/Generic/src/test/java/generic/timer/ExpiringSwingTimerTest.java

@@ -0,0 +1,86 @@
+/* ###
+ * IP: GHIDRA
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * 
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ * 
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package generic.timer;
+
+import static org.junit.Assert.*;
+
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.function.BooleanSupplier;
+
+import org.junit.Test;
+
+import generic.test.AbstractGenericTest;
+
+public class ExpiringSwingTimerTest extends AbstractGenericTest {
+
+	@Test
+	public void testRunWhenReady() {
+
+		int waitCount = 2;
+		AtomicInteger counter = new AtomicInteger();
+		BooleanSupplier isReady = () -> {
+			return counter.incrementAndGet() > waitCount;
+		};
+
+		AtomicInteger runCount = new AtomicInteger();
+		Runnable r = () -> {
+			runCount.incrementAndGet();
+		};
+		ExpiringSwingTimer.runWhen(isReady, 10000, r);
+
+		waitFor(() -> runCount.get() > 0);
+		assertTrue("Timer did not wait for the condition to be true", counter.get() > waitCount);
+		assertEquals("Client code was run more than once", 1, runCount.get());
+	}
+
+	@Test
+	public void testRunWhenReady_Timeout() {
+
+		BooleanSupplier isReady = () -> {
+			return false;
+		};
+
+		AtomicBoolean didRun = new AtomicBoolean();
+		Runnable r = () -> didRun.set(true);
+		ExpiringSwingTimer timer = ExpiringSwingTimer.runWhen(isReady, 500, r);
+
+		waitFor(() -> !timer.isRunning());
+
+		assertFalse(didRun.get());
+	}
+
+	@Test
+	public void testWorkOnlyHappensOnce() {
+
+		BooleanSupplier isReady = () -> {
+			return true;
+		};
+
+		AtomicInteger runCount = new AtomicInteger();
+		Runnable r = () -> {
+			runCount.incrementAndGet();
+		};
+
+		ExpiringSwingTimer timer = ExpiringSwingTimer.runWhen(isReady, 10000, r);
+		waitFor(() -> !timer.isRunning());
+		assertEquals(1, runCount.get());
+
+		timer.start();
+		waitFor(() -> !timer.isRunning());
+		assertEquals(1, runCount.get());
+	}
+}

Ghidra/Framework/Generic/src/test/java/ghidra/util/task/SwingUpdateManagerTest.java

@@ -37,6 +37,11 @@ public class SwingUpdateManagerTest extends AbstractGenericTest {
 	@Before
 	public void setUp() throws Exception {
 		manager = createUpdateManager(MIN_DELAY, MAX_DELAY);
+
+		// must turn this on to get the expected results, as in headless mode the update manager
+		// will run it's Swing work immediately on the test thread, which is not true to the
+		// default behavior
+		System.setProperty(SystemUtilities.HEADLESS_PROPERTY, Boolean.FALSE.toString());
 	}
 
 	@Test
@@ -160,20 +165,16 @@ public class SwingUpdateManagerTest extends AbstractGenericTest {
 		// before the update runnable is called.
 		//
 
-		// must turn this on to get the expected results, as in headless mode the update manager
-		// will run it's Swing work immediately
-		System.setProperty(SystemUtilities.HEADLESS_PROPERTY, Boolean.FALSE.toString());
-
-		final CountDownLatch startGate = new CountDownLatch(1);
-		final CountDownLatch endGate = new CountDownLatch(1);
-		final AtomicBoolean exception = new AtomicBoolean();
+		CountDownLatch startLatch = new CountDownLatch(1);
+		CountDownLatch endLatch = new CountDownLatch(1);
+		AtomicBoolean exception = new AtomicBoolean();
 
 		runSwing(new Runnable() {
 			@Override
 			public void run() {
-				startGate.countDown();
+				startLatch.countDown();
 				try {
-					endGate.await(10, TimeUnit.SECONDS);
+					endLatch.await(10, TimeUnit.SECONDS);
 				}
 				catch (InterruptedException e) {
 					exception.set(true);
@@ -181,13 +182,13 @@ public class SwingUpdateManagerTest extends AbstractGenericTest {
 			}
 		}, false);
 
-		// This will cause the swing thread to block until will countdown the endGate latch
-		startGate.await(10, TimeUnit.SECONDS);
+		// This will cause the swing thread to block until we countdown the end latch
+		startLatch.await(10, TimeUnit.SECONDS);
 
 		manager.update();
 		assertTrue("Manager not busy after requesting an update", manager.isBusy());
 
-		endGate.countDown();
+		endLatch.countDown();
 
 		waitForManager();
 		assertTrue("Manager still busy after waiting for update", !manager.isBusy());
@@ -195,9 +196,61 @@ public class SwingUpdateManagerTest extends AbstractGenericTest {
 		assertFalse("Interrupted waiting for CountDowLatch", exception.get());
 	}
 
+	@Test
+	public void testCallToUpdateWhileAnUpdateIsWorking() throws Exception {
+
+		//
+		// Test that an update call from a non-swing thread will still get processed if the
+		// manager is actively processing an update on the swing thread.
+		//
+
+		CountDownLatch startLatch = new CountDownLatch(1);
+		CountDownLatch endLatch = new CountDownLatch(1);
+		AtomicBoolean exception = new AtomicBoolean();
+
+		Runnable r = () -> {
+			runnableCalled++;
+
+			startLatch.countDown();
+			try {
+				endLatch.await(10, TimeUnit.SECONDS);
+			}
+			catch (InterruptedException e) {
+				exception.set(true);
+			}
+		};
+
+		// start the update manager and have it wait for us
+		manager = new SwingUpdateManager(MIN_DELAY, MAX_DELAY, r);
+		manager.update();
+
+		// have the swing thread block until we countdown the end latch
+		startLatch.await(10, TimeUnit.SECONDS);
+
+		// post the second update request now that the manager is actively processing
+		manager.update();
+
+		// let the update manager finish; verify 2 work items total
+		endLatch.countDown();
+		waitForManager();
+		assertEquals("Expected exactly 2 callbacks", 2, runnableCalled);
+	}
+
 //==============================================================================================
 // Private Methods
 //==============================================================================================
+	private void waitForManager() {
+
+		// let all swing updates finish, which may trigger the update manager
+		waitForSwing();
+
+		while (manager.isBusy()) {
+			sleep(DEFAULT_WAIT_DELAY);
+		}
+
+		// let any resulting swing events finish
+		waitForSwing();
+	}
 
 	private SwingUpdateManager createUpdateManager(int min, int max) {
 		return new SwingUpdateManager(min, max, new Runnable() {
@@ -209,19 +262,4 @@ public class SwingUpdateManagerTest extends AbstractGenericTest {
 			}
 		});
 	}
-
-	private void waitForManager() {
-
-		// let all swing updates finish, which may trigger the update manager
-		waitForPostedSwingRunnables();
-
-		while (manager.isBusy()) {
-			sleep(DEFAULT_WAIT_DELAY);
-		}
-
-		// let any resulting swing events finish
-		waitForPostedSwingRunnables();
-
-	}
-
 }