Ticket #13352: patch-bugreport-queue.patch

src/org/openstreetmap/josm/gui/MapView.java

@@ -77 +77 @@
 import org.openstreetmap.josm.tools.Shortcut;
 import org.openstreetmap.josm.tools.Utils;
 import org.openstreetmap.josm.tools.bugreport.BugReport;
-import org.openstreetmap.josm.tools.bugreport.BugReportExceptionHandler;
 
 /**
  * This is a component used in the {@link MapFrame} for browsing the map. It use is to
@@ -826 +825 @@
             painter.paint(paintGraphics);
             g.setPaintMode();
         } catch (RuntimeException t) {
-            //TODO: only display.
-            throw BugReport.intercept(t).put("layer", layer).put("bounds", box);
+            BugReport.intercept(t).put("layer", layer).put("bounds", box).warn();
         }
     }
 
@@ -836 +834 @@
      */
     @Override
     public void paint(Graphics g) {
-        if (!prepareToDraw()) {
+        try {
+            if (!prepareToDraw()) {
+                return;
+            }
+        } catch (RuntimeException e) {
+            BugReport.intercept(e).put("center", () -> getCenter()).warn();
             return;
         }
 
@@ -914 +917 @@
             paintLayer(visibleLayers.get(i), tempG, box);
         }
 
-        synchronized (temporaryLayers) {
-            for (MapViewPaintable mvp : temporaryLayers) {
-                try {
-                    mvp.paint(tempG, this, box);
-                } catch (RuntimeException e) {
-                    throw BugReport.intercept(e).put("mvp", mvp);
-                }
-            }
+        try {
+            drawTemporaryLayers(tempG, box);
+        } catch (RuntimeException e) {
+            BugReport.intercept(e).put("temporaryLayers", temporaryLayers).warn();
         }
 
         // draw world borders
         try {
             drawWorldBorders(tempG);
         } catch (RuntimeException e) {
-            throw BugReport.intercept(e).put("bounds", getProjection()::getWorldBoundsLatLon);
+            // getProjection() needs to be inside lambda to catch errors.
+            BugReport.intercept(e).put("bounds", () -> getProjection().getWorldBoundsLatLon()).warn();
         }
 
         if (Main.isDisplayingMapView() && Main.map.filterDialog != null) {
@@ -970 +970 @@
         super.paint(g);
     }
 
+    private void drawTemporaryLayers(Graphics2D tempG, Bounds box) {
+        synchronized (temporaryLayers) {
+            for (MapViewPaintable mvp : temporaryLayers) {
+                try {
+                    mvp.paint(tempG, this, box);
+                } catch (RuntimeException e) {
+                    throw BugReport.intercept(e).put("mvp", mvp);
+                }
+            }
+        }
+    }
+
     private void drawWorldBorders(Graphics2D tempG) {
         tempG.setColor(Color.WHITE);
         Bounds b = getProjection().getWorldBoundsLatLon();
@@ -1025 +1037 @@
             zoomTo(initialViewport);
             initialViewport = null;
         }
-        if (BugReportExceptionHandler.exceptionHandlingInProgress())
-            return false;
 
         if (getCenter() == null)
             return false; // no data loaded yet.

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

@@ -18 +18 @@
 import javax.swing.JComponent;
 import javax.swing.JDialog;
 import javax.swing.JLabel;
+import javax.swing.JOptionPane;
 import javax.swing.JPanel;
 import javax.swing.KeyStroke;
 import javax.swing.UIManager;
 
 import org.openstreetmap.josm.Main;
 import org.openstreetmap.josm.actions.ExpertToggleAction;
+import org.openstreetmap.josm.gui.preferences.plugin.PluginPreference;
+import org.openstreetmap.josm.gui.util.GuiHelper;
 import org.openstreetmap.josm.gui.widgets.JMultilineLabel;
 import org.openstreetmap.josm.gui.widgets.UrlLabel;
+import org.openstreetmap.josm.plugins.PluginDownloadTask;
+import org.openstreetmap.josm.plugins.PluginHandler;
 import org.openstreetmap.josm.tools.GBC;
 import org.openstreetmap.josm.tools.ImageProvider;
+import org.openstreetmap.josm.tools.bugreport.BugReportQueue.SuppressionMode;
 
 /**
  * This is a dialog that can be used to display a bug report.
@@ -42 +48 @@
     private final JPanel content = new JPanel(new GridBagLayout());
     private final BugReport report;
     private final DebugTextDisplay textPanel;
-    private JCheckBox cbSuppress;
+    private JCheckBox cbSuppressSingle;
+    private JCheckBox cbSuppressAll;
 
     /**
      * Create a new dialog.
@@ -148 +155 @@
 
     private void addIgnoreButton() {
         JPanel panel = new JPanel(new GridBagLayout());
-        cbSuppress = new JCheckBox(tr("Suppress further error dialogs for this session."));
-        cbSuppress.setVisible(false);
-        panel.add(cbSuppress, GBC.std().fill(GBC.HORIZONTAL));
+        cbSuppressSingle = new JCheckBox(tr("Suppress this error for this session."));
+        cbSuppressSingle.setVisible(false);
+        panel.add(cbSuppressSingle, GBC.std(0, 0).fill(GBC.HORIZONTAL));
+        cbSuppressAll = new JCheckBox(tr("Suppress further error dialogs for this session."));
+        cbSuppressAll.setVisible(false);
+        panel.add(cbSuppressAll, GBC.std(0, 1).fill(GBC.HORIZONTAL));
         JButton ignore = new JButton(tr("Ignore this error."));
         ignore.addActionListener(e -> closeDialog());
-        panel.add(ignore, GBC.eol());
+        panel.add(ignore, GBC.std(1, 0).span(1, 2).anchor(GBC.CENTER));
         content.add(panel, GBC.eol().fill(GBC.HORIZONTAL).insets(20));
     }
 
@@ -162 +172 @@
      * @param showSuppress <code>true</code> to show the suppress errors checkbox.
      */
     public void setShowSuppress(boolean showSuppress) {
-        cbSuppress.setVisible(showSuppress);
+        cbSuppressSingle.setVisible(showSuppress);
+        pack();
+    }
+
+    /**
+     * Shows or hides the suppress all errors button
+     * @param showSuppress <code>true</code> to show the suppress errors checkbox.
+     */
+    public void setShowSuppressAll(boolean showSuppress) {
+        cbSuppressAll.setVisible(showSuppress);
         pack();
     }
 
@@ -170 +189 @@
      * Check if the checkbox to suppress further errors was selected
      * @return <code>true</code> if the user wishes to suppress errors.
      */
-    public boolean shouldSuppressFurtherErrors() {
-        return cbSuppress.isSelected();
+    public SuppressionMode shouldSuppressFurtherErrors() {
+        if (cbSuppressAll.isSelected()) {
+            return SuppressionMode.ALL;
+        } else if (cbSuppressSingle.isSelected()) {
+            return SuppressionMode.SAME;
+        } else {
+            return SuppressionMode.NONE;
+        }
     }
 
     private void closeDialog() {
@@ -201 +226 @@
         }
         return null;
     }
+
+    /**
+     * Show the bug report for a given exception
+     * @param e The exception to display
+     * @param exceptionCounter A counter of how many exceptions have already been worked on
+     * @return The new suppression status
+     */
+    public static SuppressionMode showFor(ReportedException e, int exceptionCounter) {
+        if (e.isOutOfMemory()) {
+            // do not translate the string, as translation may raise an exception
+            JOptionPane.showMessageDialog(Main.parent, "JOSM is out of memory. " +
+                    "Strange things may happen.\nPlease restart JOSM with the -Xmx###M option,\n" +
+                    "where ### is the number of MB assigned to JOSM (e.g. 256).\n" +
+                    "Currently, " + Runtime.getRuntime().maxMemory()/1024/1024 + " MB are available to JOSM.",
+                    "Error",
+                    JOptionPane.ERROR_MESSAGE
+                    );
+            return SuppressionMode.NONE;
+        } else {
+            return GuiHelper.runInEDTAndWaitAndReturn(() -> {
+                PluginDownloadTask downloadTask = PluginHandler.updateOrdisablePluginAfterException(e);
+                if (downloadTask != null) {
+                    // Ask for restart to install new plugin
+                    PluginPreference.notifyDownloadResults(
+                            Main.parent, downloadTask, !downloadTask.getDownloadedPlugins().isEmpty());
+                    return SuppressionMode.NONE;
+                }
+
+                BugReport report = new BugReport(e);
+                BugReportDialog dialog = new BugReportDialog(report);
+                dialog.setShowSuppress(exceptionCounter > 0);
+                dialog.setShowSuppressAll(exceptionCounter > 1);
+                dialog.setVisible(true);
+                return dialog.shouldSuppressFurtherErrors();
+            });
+        }
+    }
 }

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

@@ -1 +1 @@
 // License: GPL. For details, see LICENSE file.
 package org.openstreetmap.josm.tools.bugreport;
 
-import java.awt.GraphicsEnvironment;
-
-import javax.swing.JOptionPane;
-import javax.swing.SwingUtilities;
-
-import org.openstreetmap.josm.Main;
-import org.openstreetmap.josm.gui.preferences.plugin.PluginPreference;
-import org.openstreetmap.josm.plugins.PluginDownloadTask;
-import org.openstreetmap.josm.plugins.PluginHandler;
-
 /**
  * An exception handler that asks the user to send a bug report.
  *
@@ -19 +9 @@
  */
 public final class BugReportExceptionHandler implements Thread.UncaughtExceptionHandler {
 
-    private static boolean handlingInProgress;
-    private static volatile BugReporterThread bugReporterThread;
-    private static int exceptionCounter;
-    private static boolean suppressExceptionDialogs;
-
-    static final class BugReporterThread extends Thread {
-
-        private final class BugReporterWorker implements Runnable {
-            private final PluginDownloadTask pluginDownloadTask;
-
-            private BugReporterWorker(PluginDownloadTask pluginDownloadTask) {
-                this.pluginDownloadTask = pluginDownloadTask;
-            }
-
-            @Override
-            public void run() {
-                // Then ask for submitting a bug report, for exceptions thrown from a plugin too, unless updated to a new version
-                if (pluginDownloadTask == null) {
-                    askForBugReport(e);
-                } else {
-                    // Ask for restart to install new plugin
-                    PluginPreference.notifyDownloadResults(
-                            Main.parent, pluginDownloadTask, !pluginDownloadTask.getDownloadedPlugins().isEmpty());
-                }
-            }
-        }
-
-        private final Throwable e;
-
-        /**
-         * Constructs a new {@code BugReporterThread}.
-         * @param t the exception
-         */
-        private BugReporterThread(Throwable t) {
-            super("Bug Reporter");
-            this.e = t;
-        }
-
-        static void askForBugReport(final Throwable e) {
-            if (GraphicsEnvironment.isHeadless()) {
-                return;
-            }
-            BugReport report = new BugReport(BugReport.intercept(e));
-            BugReportDialog dialog = new BugReportDialog(report);
-            dialog.setShowSuppress(exceptionCounter > 1);
-            dialog.setVisible(true);
-            suppressExceptionDialogs = dialog.shouldSuppressFurtherErrors();
-        }
-
-        @Override
-        public void run() {
-            // Give the user a chance to deactivate the plugin which threw the exception (if it was thrown from a plugin)
-            SwingUtilities.invokeLater(new BugReporterWorker(PluginHandler.updateOrdisablePluginAfterException(e)));
-        }
-    }
-
     @Override
     public void uncaughtException(Thread t, Throwable e) {
         handleException(e);
@@ -85 +19 @@
      * @param e the exception
      */
     public static synchronized void handleException(final Throwable e) {
-        if (handlingInProgress || suppressExceptionDialogs)
-            return;                  // we do not handle secondary exceptions, this gets too messy
-        if (bugReporterThread != null && bugReporterThread.isAlive())
-            return;
-        handlingInProgress = true;
-        exceptionCounter++;
-        try {
-            Main.error(e);
-            if (Main.parent != null) {
-                if (e instanceof OutOfMemoryError) {
-                    // do not translate the string, as translation may raise an exception
-                    JOptionPane.showMessageDialog(Main.parent, "JOSM is out of memory. " +
-                            "Strange things may happen.\nPlease restart JOSM with the -Xmx###M option,\n" +
-                            "where ### is the number of MB assigned to JOSM (e.g. 256).\n" +
-                            "Currently, " + Runtime.getRuntime().maxMemory()/1024/1024 + " MB are available to JOSM.",
-                            "Error",
-                            JOptionPane.ERROR_MESSAGE
-                            );
-                    return;
-                }
-
-                bugReporterThread = new BugReporterThread(e);
-                bugReporterThread.start();
-            }
-        } finally {
-            handlingInProgress = false;
-        }
+        BugReport.intercept(e).warn();
     }
 
     /**
@@ -119 +27 @@
      * @return {@code true} if an exception is currently being handled, {@code false} otherwise
      */
     public static boolean exceptionHandlingInProgress() {
-        return handlingInProgress;
+        return BugReportQueue.getInstance().exceptionHandlingInProgress();
     }
 }

new file src/org/openstreetmap/josm/tools/bugreport/BugReportQueue.java

@@ +1 @@
+// License: GPL. For details, see LICENSE file.
+package org.openstreetmap.josm.tools.bugreport;
+
+import java.awt.GraphicsEnvironment;
+import java.util.ArrayList;
+import java.util.LinkedList;
+import java.util.function.BiFunction;
+
+import org.openstreetmap.josm.Main;
+
+/**
+ * This class handles the display of the bug report dialog.
+ * @author Michael Zangl
+ * @since xxx
+ */
+public class BugReportQueue {
+
+    private static final BugReportQueue INSTANCE = new BugReportQueue();
+
+    private LinkedList<ReportedException> reportsToDisplay = new LinkedList<>();
+    private boolean suppressAllMessages;
+    private ArrayList<ReportedException> suppressFor = new ArrayList<>();
+    private Thread displayThread;
+    private final BiFunction<ReportedException, Integer, SuppressionMode> bugReportHandler = getBestHandler();
+    private int displayedErrors;
+
+    private boolean inReportDialog;
+
+
+    /**
+     * The suppression mode that should be used after the dialog was closed.
+     */
+    public enum SuppressionMode {
+        /**
+         * Suppress no dialogs.
+         */
+        NONE,
+        /**
+         * Suppress only the ones that are for the same error
+         */
+        SAME,
+        /**
+         * Suppress all report dialogs
+         */
+        ALL
+    }
+
+    /**
+     * Submit a new error to be displayed
+     * @param report The error to display
+     */
+    public synchronized void submit(ReportedException report) {
+        if (suppressAllMessages || suppressFor.stream().anyMatch(report::isSame)) {
+            Main.info("User requested to skip error " + report);
+        } else if (reportsToDisplay.size() > 100 || reportsToDisplay.stream().filter(report::isSame).count() >= 10) {
+            Main.warn("Too many errors. Dropping " + report);
+        } else {
+            reportsToDisplay.add(report);
+            if (displayThread == null) {
+                displayThread = new Thread(this::displayAll, "bug-report-display");
+                displayThread.start();
+            }
+            notifyAll();
+        }
+    }
+
+    private void displayAll() {
+        try {
+            while (true) {
+                ReportedException e = getNext();
+                SuppressionMode suppress = displayFor(e);
+                handleDialogResult(e, suppress);
+            }
+        } catch (InterruptedException e) {
+            displayFor(BugReport.intercept(e));
+        }
+    }
+
+    private synchronized void handleDialogResult(ReportedException e, SuppressionMode suppress) {
+        if (suppress == SuppressionMode.ALL) {
+            suppressAllMessages = true;
+            reportsToDisplay.clear();
+        } else if (suppress == SuppressionMode.SAME) {
+            suppressFor.add(e);
+            reportsToDisplay.removeIf(e::isSame);
+        }
+        displayedErrors++;
+        inReportDialog = false;
+    }
+
+    private synchronized ReportedException getNext() throws InterruptedException {
+        while (reportsToDisplay.isEmpty()) {
+            wait();
+        }
+        inReportDialog = true;
+        return reportsToDisplay.removeFirst();
+    }
+
+    private SuppressionMode displayFor(ReportedException e) {
+        return bugReportHandler.apply(e, getDisplayedErrors());
+    }
+
+    private synchronized int getDisplayedErrors() {
+        return displayedErrors;
+    }
+
+    /**
+     * Check if the dialog is shown. Should only be used for e.g. debugging.
+     * @return <code>true</code> if the exception handler is still showing the exception to the user.
+     */
+    public synchronized boolean exceptionHandlingInProgress() {
+        return !reportsToDisplay.isEmpty() || inReportDialog;
+    }
+
+    private static BiFunction<ReportedException, Integer, SuppressionMode> getBestHandler() {
+        if (GraphicsEnvironment.isHeadless()) {
+            return (e, index) -> { e.printStackTrace(); return SuppressionMode.NONE; };
+        } else {
+            return BugReportDialog::showFor;
+        }
+    }
+
+    public static BugReportQueue getInstance() {
+        return INSTANCE;
+    }
+}

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

@@ -65 +65 @@
      */
     public void warn() {
         methodWarningFrom = BugReport.getCallingMethod(2);
-        // TODO: Open the dialog.
+        try {
+            BugReportQueue.getInstance().submit(this);
+        } catch (RuntimeException e) {
+            try {
+                e.printStackTrace();
+            } catch (RuntimeException e2) {
+                // we cannot do anything more...
+                // re-throwing this causes an infinite loop.
+            }
+        }
     }
 
     /**
@@ -242 +251 @@
 
     @Override
     public String toString() {
-        return new StringBuilder(48)
-            .append("CrashReportedException [on thread ")
-            .append(caughtOnThread)
-            .append(']')
-            .toString();
+        return "ReportedException [thread=" + caughtOnThread + ", exception=" + exception
+                + ", methodWarningFrom=" + methodWarningFrom + "]";
     }
 
-
     /**
      * Check if this exception may be caused by a threading issue.
      * @return <code>true</code> if it is.
@@ -261 +266 @@
     }
 
     /**
+     * Check if this is caused by an out of memory situaition
+     * @return <code>true</code> if it is.
+     */
+    public boolean isOutOfMemory() {
+        return StreamUtils.toStream(CauseTraceIterator::new).anyMatch(t -> t instanceof OutOfMemoryError);
+    }
+
+    /**
      * Iterates over the causes for this exception. Ignores cycles and aborts iteration then.
      * @author Michal Zangl
      * @since 10585

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

@@ -21 +21 @@
     }
 
     /**
-     * Unit test for {@link BugReportExceptionHandler.BugReporterThread#askForBugReport} method.
-     */
-    @Test
-    public void testAskForBugReport() {
-        BugReportExceptionHandler.BugReporterThread.askForBugReport(new Exception("testAskForBugReport"));
-    }
-
-    /**
      * Unit test for {@link BugReportExceptionHandler#handleException} method.
      */
     @Test