Index: src/org/openstreetmap/josm/gui/progress/swing/ProgressMonitorExecutor.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/src/org/openstreetmap/josm/gui/progress/swing/ProgressMonitorExecutor.java b/src/org/openstreetmap/josm/gui/progress/swing/ProgressMonitorExecutor.java --- a/src/org/openstreetmap/josm/gui/progress/swing/ProgressMonitorExecutor.java (revision 18543) +++ b/src/org/openstreetmap/josm/gui/progress/swing/ProgressMonitorExecutor.java (date 1661783112596) @@ -10,6 +10,7 @@ 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,6 +59,7 @@ } if (t != null) { Logging.error("Thread {0} raised {1}", Thread.currentThread().getName(), t); + BugReport.addSuppressedException(t); } } } Index: src/org/openstreetmap/josm/gui/util/GuiHelper.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/src/org/openstreetmap/josm/gui/util/GuiHelper.java b/src/org/openstreetmap/josm/gui/util/GuiHelper.java --- a/src/org/openstreetmap/josm/gui/util/GuiHelper.java (revision 18543) +++ b/src/org/openstreetmap/josm/gui/util/GuiHelper.java (date 1661781458094) @@ -211,6 +211,7 @@ */ static void handleEDTException(Throwable t) { Logging.logWithStackTrace(Logging.LEVEL_ERROR, t, "Exception raised in EDT"); + BugReport.addSuppressedException(t); } /** Index: src/org/openstreetmap/josm/tools/bugreport/BugReport.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/src/org/openstreetmap/josm/tools/bugreport/BugReport.java b/src/org/openstreetmap/josm/tools/bugreport/BugReport.java --- a/src/org/openstreetmap/josm/tools/bugreport/BugReport.java (revision 18543) +++ b/src/org/openstreetmap/josm/tools/bugreport/BugReport.java (date 1661789543565) @@ -4,9 +4,14 @@ 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. *

@@ -39,6 +44,10 @@ */ 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<time reported, exception> */ + private static final Deque> SUPPRESSED_EXCEPTIONS = new ArrayDeque<>(MAXIMUM_SUPPRESSED_EXCEPTIONS); private boolean includeStatusReport = true; private boolean includeData = true; @@ -55,6 +64,21 @@ 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 true to include it. @@ -135,6 +159,14 @@ if (isIncludeAllStackTraces()) { exception.printReportThreadsTo(out); } + if (!SUPPRESSED_EXCEPTIONS.isEmpty()) { + out.println("=== ADDITIONAL EXCEPTIONS ==="); + while (SUPPRESSED_EXCEPTIONS.peek() != null) { + Pair currentException = SUPPRESSED_EXCEPTIONS.pop(); + out.println("==== Exception at " + currentException.a.toEpochMilli() + " ===="); + currentException.b.printStackTrace(out); + } + } return stringWriter.toString().replaceAll("\r", ""); } Index: test/unit/org/openstreetmap/josm/tools/bugreport/BugReportTest.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/test/unit/org/openstreetmap/josm/tools/bugreport/BugReportTest.java b/test/unit/org/openstreetmap/josm/tools/bugreport/BugReportTest.java --- a/test/unit/org/openstreetmap/josm/tools/bugreport/BugReportTest.java (revision 18543) +++ b/test/unit/org/openstreetmap/josm/tools/bugreport/BugReportTest.java (date 1661791338616) @@ -1,17 +1,32 @@ // 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,4 +84,91 @@ 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 testSuppressedExceptions() { + return Stream.of( + Arguments.of("GuiHelper::runInEDTAndWaitAndReturn", + (Consumer) r -> GuiHelper.runInEDTAndWaitAndReturn(() -> { + r.run(); + return null; + })), + Arguments.of("GuiHelper::runInEDTAndWait", (Consumer) GuiHelper::runInEDTAndWait), + Arguments.of("MainApplication.worker", (Consumer) MainApplication.worker::execute) + ); + } + + @ParameterizedTest + @MethodSource + void testSuppressedExceptions(String workerName, Consumer 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()); + } }