Ticket #22183: 22183.patch

src/org/openstreetmap/josm/gui/progress/swing/ProgressMonitorExecutor.java

@@ -10 +10 @@
 
 import org.openstreetmap.josm.tools.Logging;
 import org.openstreetmap.josm.tools.Utils;
+import org.openstreetmap.josm.tools.bugreport.BugReport;
 
 /**
  * Executor that displays the progress monitor to the user.
@@ -58 +59 @@
         }
         if (t != null) {
             Logging.error("Thread {0} raised {1}", Thread.currentThread().getName(), t);
+            BugReport.addSuppressedException(t);
         }
     }
 }

src/org/openstreetmap/josm/gui/util/GuiHelper.java

@@ -211 +211 @@
      */
     static void handleEDTException(Throwable t) {
         Logging.logWithStackTrace(Logging.LEVEL_ERROR, t, "Exception raised in EDT");
+        BugReport.addSuppressedException(t);
     }
 
     /**

src/org/openstreetmap/josm/tools/bugreport/BugReport.java

@@ -4 +4 @@
 import java.io.PrintWriter;
 import java.io.Serializable;
 import java.io.StringWriter;
+import java.time.Instant;
+import java.util.ArrayDeque;
+import java.util.Deque;
 import java.util.concurrent.CopyOnWriteArrayList;
 import java.util.function.Predicate;
 
+import org.openstreetmap.josm.tools.Pair;
+
 /**
  * This class contains utility methods to create and handle a bug report.
  * <p>
@@ -39 +44 @@
  */
 public final class BugReport implements Serializable {
     private static final long serialVersionUID = 1L;
+    /** The maximum suppressed exceptions to keep to report */
+    private static final byte MAXIMUM_SUPPRESSED_EXCEPTIONS = 4;
+    /** The list of suppressed exceptions, Pair&lt;time reported, exception&gt; */
+    private static final Deque<Pair<Instant, Throwable>> SUPPRESSED_EXCEPTIONS = new ArrayDeque<>(MAXIMUM_SUPPRESSED_EXCEPTIONS);
 
     private boolean includeStatusReport = true;
     private boolean includeData = true;
@@ -55 +64 @@
         includeAllStackTraces = e.mayHaveConcurrentSource();
     }
 
+    /**
+     * Add a suppressed exception. Mostly useful for when a chain of exceptions causes an actual bug report.
+     * This should only be used when an exception is raised in {@link org.openstreetmap.josm.gui.util.GuiHelper}
+     * or {@link org.openstreetmap.josm.gui.progress.swing.ProgressMonitorExecutor} at this time.
+     * @param t The throwable raised. If {@code null}, we add a new {@code NullPointerException} instead.
+     * @since xxx
+     */
+    public static void addSuppressedException(Throwable t) {
+        SUPPRESSED_EXCEPTIONS.add(new Pair<>(Instant.now(), t != null ? t : new NullPointerException()));
+        // Ensure we aren't keeping exceptions forever
+        while (SUPPRESSED_EXCEPTIONS.size() > MAXIMUM_SUPPRESSED_EXCEPTIONS) {
+            SUPPRESSED_EXCEPTIONS.pop();
+        }
+    }
+
     /**
      * Determines if this report should include a system status report
      * @return <code>true</code> to include it.
@@ -135 +159 @@
         if (isIncludeAllStackTraces()) {
             exception.printReportThreadsTo(out);
         }
+        if (!SUPPRESSED_EXCEPTIONS.isEmpty()) {
+            out.println("=== ADDITIONAL EXCEPTIONS ===");
+            while (SUPPRESSED_EXCEPTIONS.peek() != null) {
+                Pair<Instant, Throwable> currentException = SUPPRESSED_EXCEPTIONS.pop();
+                out.println("==== Exception at " + currentException.a.toEpochMilli() + " ====");
+                currentException.b.printStackTrace(out);
+            }
+        }
         return stringWriter.toString().replaceAll("\r", "");
     }
 

test/unit/org/openstreetmap/josm/tools/bugreport/BugReportTest.java

@@ -1 +1 @@
 // License: GPL. For details, see LICENSE file.
 package org.openstreetmap.josm.tools.bugreport;
 
+import static org.junit.jupiter.api.Assertions.assertAll;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertSame;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 import java.io.IOException;
 import java.io.PrintWriter;
 import java.io.StringWriter;
+import java.lang.reflect.Field;
+import java.util.function.Consumer;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+import java.util.stream.Stream;
 
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+import org.junit.platform.commons.util.ReflectionUtils;
 import org.openstreetmap.josm.actions.ShowStatusReportAction;
+import org.openstreetmap.josm.gui.MainApplication;
+import org.openstreetmap.josm.gui.util.GuiHelper;
 import org.openstreetmap.josm.testutils.annotations.BasicPreferences;
+import org.openstreetmap.josm.tools.Logging;
 
 /**
  * Tests the bug report class.
@@ -69 +84 @@
     private String testGetCallingMethod2() {
         return BugReport.getCallingMethod(2);
     }
+
+    @Test
+    void testSuppressedExceptionsOrder() {
+        final String methodName = "testSuppressedExceptionsOrder";
+        BugReport.addSuppressedException(new NullPointerException(methodName));
+        BugReport.addSuppressedException(new IllegalStateException(methodName));
+        BugReport bugReport = new BugReport(BugReport.intercept(new IOException(methodName)));
+        final String report = assertDoesNotThrow(() -> bugReport.getReportText(methodName));
+        assertAll(() -> assertTrue(report.contains("NullPointerException")),
+                () -> assertTrue(report.contains("IOException")),
+                () -> assertTrue(report.contains("IllegalStateException")));
+        int ioe = report.indexOf("IOException");
+        int npe = report.indexOf("NullPointerException");
+        int ise = report.indexOf("IllegalStateException");
+        assertAll("Ordering of exceptions is wrong",
+                () -> assertTrue(ioe < npe, "IOException should be reported before NullPointerException"),
+                () -> assertTrue(npe < ise, "NullPointerException should be reported before IllegalStateException"));
+    }
+
+    static Stream<Arguments> testSuppressedExceptions() {
+        return Stream.of(
+                Arguments.of("GuiHelper::runInEDTAndWaitAndReturn",
+                        (Consumer<Runnable>) r -> GuiHelper.runInEDTAndWaitAndReturn(() -> {
+                            r.run();
+                            return null;
+                        })),
+                Arguments.of("GuiHelper::runInEDTAndWait", (Consumer<Runnable>) GuiHelper::runInEDTAndWait),
+                Arguments.of("MainApplication.worker", (Consumer<Runnable>) MainApplication.worker::execute)
+        );
+    }
+
+    @ParameterizedTest
+    @MethodSource
+    void testSuppressedExceptions(String workerName, Consumer<Runnable> worker) {
+        // Throw a npe in the worker. Workers might give us the exception, wrapped or otherwise.
+        try {
+            worker.accept(() -> {
+                throw new NullPointerException();
+            });
+        } catch (Exception e) {
+            // pass. MainApplication.worker can continue throwing the NPE;
+            Logging.trace(e);
+        }
+        // Ensure that the threads are synced
+        assertDoesNotThrow(() -> worker.accept(() -> { /* sync */ }));
+        // Now throw an exception
+        BugReport bugReport = new BugReport(BugReport.intercept(new IOException("testSuppressedExceptions")));
+        String report = bugReport.getReportText(workerName);
+        assertTrue(report.contains("IOException"));
+        assertTrue(report.contains("NullPointerException"));
+    }
+
+    @Test
+    void testSuppressedExceptionsReportedOnce() {
+        // Add the exception
+        BugReport.addSuppressedException(new NullPointerException("testSuppressedExceptionsReportedOnce"));
+        BugReport bugReport = new BugReport(BugReport.intercept(new IOException("testSuppressedExceptionsReportedOnce")));
+        // Get the report which clears the suppressed exceptions
+        String report = bugReport.getReportText("");
+        assertTrue(report.contains("IOException"));
+        assertTrue(report.contains("NullPointerException"));
+
+        BugReport bugReport2 = new BugReport(BugReport.intercept(new IOException("testSuppressedExceptionsReportedOnce")));
+        String report2 = bugReport2.getReportText("");
+        assertTrue(report2.contains("IOException"));
+        assertFalse(report2.contains("NullPointerException"));
+    }
+
+    @Test
+    void testManyExceptions() throws ReflectiveOperationException {
+        Field suppressedExceptions = BugReport.class.getDeclaredField("MAXIMUM_SUPPRESSED_EXCEPTIONS");
+        ReflectionUtils.makeAccessible(suppressedExceptions);
+        final byte expected = suppressedExceptions.getByte(null);
+        final int end = 2 * expected;
+        // Add many suppressed exceptions
+        for (int i = 0; i < end; i++) {
+            BugReport.addSuppressedException(new NullPointerException("NPE: " + i));
+        }
+        BugReport bugReport = new BugReport(BugReport.intercept(new IOException("testManyExceptions")));
+        String report = bugReport.getReportText("");
+        Matcher matcher = Pattern.compile("NPE: (\\d+)").matcher(report);
+        for (int i = end - expected; i < end; ++i) {
+            assertTrue(matcher.find());
+            assertEquals(Integer.toString(i), matcher.group(1));
+        }
+        assertFalse(matcher.find());
+    }
 }