Merge pull request #32 from jglick/ErrorAction-JENKINS-39346

[JENKINS-39346] Check for unserializable nested exceptions
jenkinsci · Feb 14, 2017 · b8f6ce6 · b8f6ce6
2 parents d061745 + 724456b
commit b8f6ce6
Show file tree

Hide file tree

Showing 3 changed files with 50 additions and 14 deletions.
diff --git a/pom.xml b/pom.xml
@@ -28,7 +28,7 @@
     <parent>
         <groupId>org.jenkins-ci.plugins</groupId>
         <artifactId>plugin</artifactId>
-        <version>2.21</version>
+        <version>2.22</version>
         <relativePath/>
     </parent>
     <groupId>org.jenkins-ci.plugins.workflow</groupId>
@@ -70,6 +70,12 @@
             <artifactId>workflow-step-api</artifactId>
             <version>2.7</version>
         </dependency>
+        <dependency>
+            <groupId>org.jenkins-ci.plugins.workflow</groupId>
+            <artifactId>workflow-support</artifactId>
+            <version>2.13</version>
+            <scope>test</scope>
+        </dependency>
         <dependency>
             <groupId>org.jenkins-ci.plugins.workflow</groupId>
             <artifactId>workflow-job</artifactId>
@@ -79,7 +85,7 @@
         <dependency>
             <groupId>org.jenkins-ci.plugins.workflow</groupId>
             <artifactId>workflow-cps</artifactId>
-            <version>2.25</version>
+            <version>2.27</version>
             <scope>test</scope>
         </dependency>
         <dependency>
@@ -91,7 +97,7 @@
         <dependency>  <!-- For Semaphore step -->
             <groupId>org.jenkins-ci.plugins.workflow</groupId>
             <artifactId>workflow-support</artifactId>
-            <version>2.12</version>
+            <version>2.13</version>
             <scope>test</scope>
             <classifier>tests</classifier>
         </dependency>

diff --git a/src/main/java/org/jenkinsci/plugins/workflow/actions/ErrorAction.java b/src/main/java/org/jenkinsci/plugins/workflow/actions/ErrorAction.java
@@ -27,35 +27,33 @@
 import groovy.lang.MissingMethodException;
 import hudson.remoting.ClassFilter;
 import hudson.remoting.ProxyException;
+import javax.annotation.CheckForNull;
 import org.codehaus.groovy.control.MultipleCompilationErrorsException;
 import org.jenkinsci.plugins.workflow.graph.AtomNode;
-import hudson.model.Action;
+import javax.annotation.Nonnull;
 
 /**
  * Attached to {@link AtomNode} that caused an error.
  *
  * This has to be Action because it's added after a node is created.
- *
- * @author Kohsuke Kawaguchi
  */
 public class ErrorAction implements PersistentAction {
-    private final Throwable error;
 
-    public ErrorAction(Throwable error) {
+    private final @Nonnull Throwable error;
+
+    public ErrorAction(@Nonnull Throwable error) {
         if (isUnserializableException(error)) {
             error = new ProxyException(error);
         }
-        assert error!=null;
         this.error = error;
     }
 
     /**
      * Some exceptions don't serialize properly. If so, we need to replace that with
      * an equivalent that captures the same details but serializes nicely.
      */
-    private boolean isUnserializableException(Throwable error) {
+    private boolean isUnserializableException(@CheckForNull Throwable error) {
         if (error == null) {
-            // This shouldn't happen.
             return false;
         }
         try {
@@ -67,11 +65,21 @@ private boolean isUnserializableException(Throwable error) {
         } catch (SecurityException x) {
             return true;
         }
-        return error instanceof MultipleCompilationErrorsException ||
-               error instanceof MissingMethodException;
+        if (error instanceof MultipleCompilationErrorsException || error instanceof MissingMethodException) {
+            return true;
+        }
+        if (isUnserializableException(error.getCause())) {
+            return true;
+        }
+        for (Throwable t : error.getSuppressed()) {
+            if (isUnserializableException(t)) {
+                return true;
+            }
+        }
+        return false;
     }
 
-    public Throwable getError() {
+    public @Nonnull Throwable getError() {
         return error;
     }
 

diff --git a/src/test/java/org/jenkinsci/plugins/workflow/actions/ErrorActionTest.java b/src/test/java/org/jenkinsci/plugins/workflow/actions/ErrorActionTest.java
@@ -110,4 +110,26 @@ public void unserializableForSecurityReason() throws Exception {
             assertEquals(ProxyException.class, e.getError().getClass());
         }
     }
+
+    @Issue("JENKINS-39346")
+    @Test public void wrappedUnserializableException() throws Exception {
+        WorkflowJob p = r.jenkins.createProject(WorkflowJob.class, "p");
+        p.setDefinition(new CpsFlowDefinition(
+            "catchError {\n" +
+            "  try {\n" +
+            "    try {\n" +
+            "      throw new NullPointerException('oops')\n" +
+            "    } catch (e) {\n" +
+            "      throw new org.codehaus.groovy.runtime.InvokerInvocationException(e)\n" + // TODO is there a way to convince Groovy to throw this on its own?
+            "    }\n" +
+            "  } catch (e) {\n" +
+            "    throw new IllegalArgumentException(e)\n" +
+            "  }\n" +
+            "}\n" +
+            "echo 'got to the end'", false));
+        WorkflowRun b = r.assertBuildStatus(Result.FAILURE, p.scheduleBuild2(0).get());
+        r.assertLogContains("got to the end", b);
+        r.assertLogContains("java.lang.NullPointerException: oops", b);
+    }
+
 }