From 5fd3dc14343575203db206a242a6f8b1ecbe3e8f Mon Sep 17 00:00:00 2001 From: Thomas Wolf Date: Tue, 25 Apr 2017 11:25:48 +0200 Subject: [PATCH] Make HyperlinkTokenScanner more robust Catch RuntimeExceptions raised by contributed IHyperlinkDetectors. Bug: 515730 Change-Id: I2456d285de9c1cdedac1f34c27b33f24f99e0429 Signed-off-by: Thomas Wolf --- .../dialogs/HyperlinkTokenScannerTest.java | 52 +++++++++++++++- .../ui/internal/dialogs/HyperlinkTokenScanner.java | 71 ++++++++++++++-------- 2 files changed, 94 insertions(+), 29 deletions(-) diff --git a/org.eclipse.egit.ui.test/src/org/eclipse/egit/ui/internal/dialogs/HyperlinkTokenScannerTest.java b/org.eclipse.egit.ui.test/src/org/eclipse/egit/ui/internal/dialogs/HyperlinkTokenScannerTest.java index 2b1eece69..fac920110 100644 --- a/org.eclipse.egit.ui.test/src/org/eclipse/egit/ui/internal/dialogs/HyperlinkTokenScannerTest.java +++ b/org.eclipse.egit.ui.test/src/org/eclipse/egit/ui/internal/dialogs/HyperlinkTokenScannerTest.java @@ -16,7 +16,11 @@ import java.util.Arrays; import org.eclipse.jface.preference.IPreferenceStore; import org.eclipse.jface.text.Document; import org.eclipse.jface.text.IDocument; +import org.eclipse.jface.text.IRegion; +import org.eclipse.jface.text.ITextViewer; import org.eclipse.jface.text.TextAttribute; +import org.eclipse.jface.text.hyperlink.AbstractHyperlinkDetector; +import org.eclipse.jface.text.hyperlink.IHyperlink; import org.eclipse.jface.text.hyperlink.IHyperlinkDetector; import org.eclipse.jface.text.hyperlink.URLHyperlinkDetector; import org.eclipse.jface.text.rules.IToken; @@ -43,6 +47,9 @@ public class HyperlinkTokenScannerTest { private IHyperlinkDetector[] detectors = new IHyperlinkDetector[] { new URLHyperlinkDetector() }; + private IHyperlinkDetector[] detectorsWithFailure = new IHyperlinkDetector[] { + new URLHyperlinkDetector(), new CrashingHyperlinkDetector() }; + @Test public void tokenizeEmpty() { String testString = ""; @@ -103,20 +110,48 @@ public class HyperlinkTokenScannerTest { assertTokens(testString, 15, 14, expected); } - @SuppressWarnings("boxing") + @Test + public void tokenizeWithFailingDetector() { + String testString = "Link: http://foo bar"; + String expected = "DDDDDDHHHHHHHHHHDDDD"; + assertTokens(testString, 0, testString.length(), detectorsWithFailure, + expected); + // With only a failing detector + expected = "DDDDDDDDDDDDDDDDDDDD"; + assertTokens(testString, 0, testString.length(), + new IHyperlinkDetector[] { new CrashingHyperlinkDetector() }, + expected); + } + + @Test + public void tokenizeWithoutDetectors() { + String testString = "Link: http://foo bar"; + String expected = "DDDDDDDDDDDDDDDDDDDD"; + assertTokens(testString, 0, testString.length(), + new IHyperlinkDetector[] {}, expected); + } + private void assertTokens(String text, int offset, int length, String expected) { + assertTokens(text, offset, length, detectors, expected); + } + + @SuppressWarnings("boxing") + private void assertTokens(String text, int offset, int length, + IHyperlinkDetector[] hyperlinkDetectors, String expected) { assertEquals("Test definition problem: 'expected' length mismatch", text.length(), expected.length()); IDocument testDocument = new Document(text); when(viewer.getDocument()).thenReturn(testDocument); - when(configuration.getHyperlinkDetectors(viewer)).thenReturn(detectors); + when(configuration.getHyperlinkDetectors(viewer)) + .thenReturn(hyperlinkDetectors); when(preferenceStore .getBoolean(AbstractTextEditor.PREFERENCE_HYPERLINKS_ENABLED)) .thenReturn(true); when(preferenceStore.getBoolean( "org.eclipse.ui.internal.editors.text.URLHyperlinkDetector")) - .thenReturn(false); + .thenReturn(hyperlinkDetectors.length == 0 + || (hyperlinkDetectors[0] instanceof CrashingHyperlinkDetector)); HyperlinkTokenScanner scanner = new HyperlinkTokenScanner(configuration, viewer, preferenceStore, null); scanner.setRangeAndColor(testDocument, offset, length, null); @@ -141,4 +176,15 @@ public class HyperlinkTokenScannerTest { assertEquals("Unexpected tokens", expected, new String(found)); } + private static class CrashingHyperlinkDetector + extends AbstractHyperlinkDetector { + + @Override + public IHyperlink[] detectHyperlinks(ITextViewer textViewer, + IRegion region, boolean canShowMultipleHyperlinks) { + throw new IllegalStateException( + "CrashingHyperlinkDetector fails on purpose"); + } + + } } diff --git a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/dialogs/HyperlinkTokenScanner.java b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/dialogs/HyperlinkTokenScanner.java index 559aed1ed..64ebc6d42 100644 --- a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/dialogs/HyperlinkTokenScanner.java +++ b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/dialogs/HyperlinkTokenScanner.java @@ -12,11 +12,11 @@ import java.util.ArrayList; import java.util.Collections; import java.util.Comparator; import java.util.Iterator; +import java.util.LinkedHashSet; import java.util.List; +import java.util.Set; import org.eclipse.core.runtime.Assert; -import org.eclipse.jgit.annotations.NonNull; -import org.eclipse.jgit.annotations.Nullable; import org.eclipse.jface.preference.IPreferenceStore; import org.eclipse.jface.resource.JFaceColors; import org.eclipse.jface.text.BadLocationException; @@ -33,6 +33,8 @@ import org.eclipse.jface.text.rules.ITokenScanner; import org.eclipse.jface.text.rules.Token; import org.eclipse.jface.text.source.ISourceViewer; import org.eclipse.jface.text.source.SourceViewerConfiguration; +import org.eclipse.jgit.annotations.NonNull; +import org.eclipse.jgit.annotations.Nullable; import org.eclipse.swt.graphics.Color; import org.eclipse.ui.editors.text.EditorsUI; import org.eclipse.ui.texteditor.AbstractTextEditor; @@ -51,7 +53,7 @@ public class HyperlinkTokenScanner implements ITokenScanner { private IToken hyperlinkToken; - private IHyperlinkDetector[] hyperlinkDetectors; + private Set hyperlinkDetectors; private final ISourceViewer viewer; @@ -149,24 +151,43 @@ public class HyperlinkTokenScanner implements ITokenScanner { hyperlinksOnLine.clear(); return Token.EOF; } - if (hyperlinkDetectors != null && hyperlinkDetectors.length > 0) { + if (hyperlinkDetectors != null && !hyperlinkDetectors.isEmpty()) { try { IRegion currentLine = document .getLineInformationOfOffset(currentOffset); if (currentLine.getOffset() != lastLineStart) { // Compute all hyperlinks in the line hyperlinksOnLine.clear(); - for (IHyperlinkDetector hyperlinkDetector : hyperlinkDetectors) { + Iterator detectors = hyperlinkDetectors + .iterator(); + while (detectors.hasNext()) { + IHyperlinkDetector hyperlinkDetector = detectors.next(); // The NoMaskHyperlinkDetectors can be skipped; if there // are any, they're only duplicates of others to ensure // hyperlinks open also if no modifier key is active. - if (!(hyperlinkDetector instanceof HyperlinkSourceViewer.NoMaskHyperlinkDetector)) { - IHyperlink[] newLinks = hyperlinkDetector - .detectHyperlinks(viewer, currentLine, - false); - if (newLinks != null && newLinks.length > 0) { - Collections.addAll(hyperlinksOnLine, newLinks); - } + if (hyperlinkDetector instanceof HyperlinkSourceViewer.NoMaskHyperlinkDetector) { + continue; + } + IHyperlink[] newLinks = null; + try { + newLinks = hyperlinkDetector.detectHyperlinks( + viewer, currentLine, false); + } catch (RuntimeException e) { + // Do *not* log: we have no way of identifying the + // broken hyperlink detector to ignore it in the + // future. Since we re-get the contributed detectors + // frequently, we'll get new instances of + // HyperlinkDetectorDelegate, and that doesn't give + // access to the extension id. So even if we remove + // the detector here, we may acquire a new broken + // instance again and then log over and over again, + // which is hyper-bothersome if the error log is + // open and set to activate on new errors. And + // anyway the problem is not in EGit. + detectors.remove(); + } + if (newLinks != null && newLinks.length > 0) { + Collections.addAll(hyperlinksOnLine, newLinks); } } // Sort them by offset, and with increasing length @@ -276,25 +297,23 @@ public class HyperlinkTokenScanner implements ITokenScanner { return null; } - private @NonNull IHyperlinkDetector[] getHyperlinkDetectors() { - IHyperlinkDetector[] allDetectors; + private @NonNull Set getHyperlinkDetectors() { + Set allDetectors = new LinkedHashSet<>(); IHyperlinkDetector[] configuredDetectors = configuration .getHyperlinkDetectors(viewer); - if (configuredDetectors == null || configuredDetectors.length == 0) { - allDetectors = new IHyperlinkDetector[0]; - } else if (preferenceStore.getBoolean(URL_HYPERLINK_DETECTOR_KEY) - || !preferenceStore.getBoolean( - AbstractTextEditor.PREFERENCE_HYPERLINKS_ENABLED)) { - allDetectors = configuredDetectors; - } else { - allDetectors = new IHyperlinkDetector[configuredDetectors.length - + 1]; - System.arraycopy(configuredDetectors, 0, allDetectors, 0, - configuredDetectors.length); + if (configuredDetectors != null && configuredDetectors.length > 0) { + for (IHyperlinkDetector detector : configuredDetectors) { + allDetectors.add(detector); + } + if (preferenceStore.getBoolean(URL_HYPERLINK_DETECTOR_KEY) + || !preferenceStore.getBoolean( + AbstractTextEditor.PREFERENCE_HYPERLINKS_ENABLED)) { + return allDetectors; + } // URLHyperlinkDetector can only detect hyperlinks at the start of // the range. We need one that can detect all hyperlinks in a given // region. - allDetectors[configuredDetectors.length] = new MultiURLHyperlinkDetector(); + allDetectors.add(new MultiURLHyperlinkDetector()); } return allDetectors; } -- 2.11.4.GIT