Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JENKINS-60410] Add stack trace suppression into core as a standard behavior. #4389

Merged
merged 76 commits into from
Mar 4, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
76 commits
Select commit Hold shift + click to select a range
5f8f935
Add stack trace suppression into core as a standard.
jeffret-b Dec 6, 2019
ab78008
Clean up suppression filter.
jeffret-b Dec 9, 2019
cece2b4
Minor review comments.
jeffret-b Dec 9, 2019
b720e27
Handle two review comments.
jeffret-b Jan 9, 2020
64dc4de
Better handle AccessDeniedException.
jeffret-b Jan 9, 2020
4a01311
First pass at adding tests.
jeffret-b Jan 13, 2020
cf29446
Convert to not show the trace to admins.
jeffret-b Jan 13, 2020
0e3be66
Remove some unrelated tests.
jeffret-b Jan 13, 2020
4a7684e
Add test for basic exception.
jeffret-b Jan 14, 2020
8f3e14a
Shift to using HttpResponse.SHOW_STACK_TRACE.
jeffret-b Jan 14, 2020
fa27840
Adjust comment.
jeffret-b Jan 14, 2020
cfe722b
Rename class for better clarity.
jeffret-b Jan 14, 2020
2b6aed9
Need a new Stapler release or incremental for these tests.
jeffret-b Jan 14, 2020
8efba8a
Finish cleanup of Stapler separation.
jeffret-b Jan 15, 2020
70503e2
Add a couple more tests and some comments about the error source.
jeffret-b Jan 15, 2020
a441cbb
Add logging ID to uncaught exception handler.
jeffret-b Jan 21, 2020
e9d80f0
Revert inadvertent changes to SimpleJobTest.
jeffret-b Jan 21, 2020
e00d41c
Stop adding filter for now.
jeffret-b Jan 21, 2020
3e9444a
Small adjustments to oops.jelly.
jeffret-b Jan 21, 2020
4f22304
Fix tests.
jeffret-b Jan 21, 2020
6a14185
Catch embedded security exceptions.
jeffret-b Jan 21, 2020
86bad5a
Remove messages that aren't used after all.
jeffret-b Jan 21, 2020
20e3c78
Remove the unused messages.
jeffret-b Jan 21, 2020
232edf5
Use the stapler incremental now that's republished.
jeffret-b Jan 22, 2020
354ee66
Don't use public static boolean property.
jeffret-b Jan 23, 2020
e0c528c
Adjust tests.
jeffret-b Jan 27, 2020
3301898
Don't handle embedded security exceptions.
jeffret-b Jan 27, 2020
eef3a30
Remove now unused method.
jeffret-b Jan 28, 2020
c8f8b11
Adopt Daniel's suggestion for tying Stapler change to Jenkins.
jeffret-b Jan 28, 2020
92c24c6
Adjust naming.
jeffret-b Jan 29, 2020
dfee356
Use released Stapler.
jeffret-b Feb 10, 2020
8b50892
Add stack trace suppression into core as a standard.
jeffret-b Dec 6, 2019
a4f0f8f
Clean up suppression filter.
jeffret-b Dec 9, 2019
9472660
Minor review comments.
jeffret-b Dec 9, 2019
c3e39ef
Handle two review comments.
jeffret-b Jan 9, 2020
3cf878f
Better handle AccessDeniedException.
jeffret-b Jan 9, 2020
fff47bb
First pass at adding tests.
jeffret-b Jan 13, 2020
d41b0cb
Convert to not show the trace to admins.
jeffret-b Jan 13, 2020
1eb11af
Remove some unrelated tests.
jeffret-b Jan 13, 2020
b380e62
Add test for basic exception.
jeffret-b Jan 14, 2020
9b6aeba
Shift to using HttpResponse.SHOW_STACK_TRACE.
jeffret-b Jan 14, 2020
e6e261f
Adjust comment.
jeffret-b Jan 14, 2020
452128c
Rename class for better clarity.
jeffret-b Jan 14, 2020
c18b81b
Need a new Stapler release or incremental for these tests.
jeffret-b Jan 14, 2020
aad2b10
Finish cleanup of Stapler separation.
jeffret-b Jan 15, 2020
eead355
Add a couple more tests and some comments about the error source.
jeffret-b Jan 15, 2020
bcf587b
Add logging ID to uncaught exception handler.
jeffret-b Jan 21, 2020
45e2f5c
Revert inadvertent changes to SimpleJobTest.
jeffret-b Jan 21, 2020
e00a860
Stop adding filter for now.
jeffret-b Jan 21, 2020
c936bd1
Small adjustments to oops.jelly.
jeffret-b Jan 21, 2020
8638289
Fix tests.
jeffret-b Jan 21, 2020
b56a380
Catch embedded security exceptions.
jeffret-b Jan 21, 2020
ed53793
Remove messages that aren't used after all.
jeffret-b Jan 21, 2020
acebd93
Remove the unused messages.
jeffret-b Jan 21, 2020
b848f85
Use the stapler incremental now that's republished.
jeffret-b Jan 22, 2020
354a054
Don't use public static boolean property.
jeffret-b Jan 23, 2020
83148e0
Adjust tests.
jeffret-b Jan 27, 2020
8baddd2
Don't handle embedded security exceptions.
jeffret-b Jan 27, 2020
13d4a37
Remove now unused method.
jeffret-b Jan 28, 2020
4a01463
Adopt Daniel's suggestion for tying Stapler change to Jenkins.
jeffret-b Jan 28, 2020
cfc5e52
Adjust naming.
jeffret-b Jan 29, 2020
76e64fc
Use released Stapler.
jeffret-b Feb 10, 2020
39fc974
Pass the HTTP status code on to the oops page.
jeffret-b Feb 13, 2020
19b25ed
Merge branch 'suppressStack' of github.com:jeffret-b/jenkins into sup…
jeffret-b Feb 13, 2020
313d198
Update to use newer jenkins-test-harness.
jeffret-b Feb 13, 2020
d4b5d3f
Show stack trace for jetty:run.
jeffret-b Feb 14, 2020
df385a8
Adjust jelly for popup.
jeffret-b Feb 20, 2020
ec707b3
Change popup dialog size.
jeffret-b Feb 21, 2020
a9c3910
Adjust math operations for clarity.
jeffret-b Feb 21, 2020
7cc0628
Merge branch 'master' into suppressStack
daniel-beck Feb 22, 2020
4786a1c
Merge branch 'master' into suppressStack
jeffret-b Feb 24, 2020
984601d
Merge branch 'suppressStack' of github.com:jeffret-b/jenkins into sup…
jeffret-b Feb 24, 2020
6b68c9e
Merge branch 'master' into suppressStack
jeffret-b Feb 24, 2020
4510307
Adjust test for new permission changes.
jeffret-b Feb 24, 2020
feb6aa5
Merge branch 'suppressStack' of github.com:jeffret-b/jenkins into sup…
jeffret-b Feb 24, 2020
215319d
Update test/pom.xml
timja Feb 26, 2020
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion core/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ THE SOFTWARE.

<properties>
<staplerFork>true</staplerFork>
<stapler.version>1.258</stapler.version>
<stapler.version>1.259</stapler.version>
<spring.version>2.5.6.SEC03</spring.version>
<groovy.version>2.4.12</groovy.version>
</properties>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,23 @@
import hudson.init.Initializer;
import jenkins.model.Jenkins;
import jenkins.telemetry.impl.java11.MissingClassTelemetry;
import org.kohsuke.MetaInfServices;
import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.NoExternalUse;
import org.kohsuke.stapler.HttpResponses;
import org.kohsuke.stapler.Stapler;
import org.kohsuke.stapler.StaplerRequest;
import org.kohsuke.stapler.StaplerResponse;
import org.kohsuke.stapler.WebApp;
import org.kohsuke.stapler.compression.CompressionFilter;

import javax.annotation.CheckForNull;
import javax.servlet.ServletException;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import java.io.EOFException;
import java.io.IOException;
import java.util.UUID;
import java.util.logging.Level;
import java.util.logging.Logger;

Expand All @@ -23,22 +33,7 @@ public class InstallUncaughtExceptionHandler {
@Initializer
public static void init(final Jenkins j) throws IOException {
CompressionFilter.setUncaughtExceptionHandler(j.servletContext, (e, context, req, rsp) -> {
if (rsp.isCommitted()) {
LOGGER.log(isEOFException(e) ? Level.FINE : Level.WARNING, null, e);
return;
}
req.setAttribute("javax.servlet.error.exception",e);
try {
// If we have an exception, let's see if it's related with missing classes on Java 11. We reach
// here with a ClassNotFoundException in an action, for example. Setting the report here is the only
// way to catch the missing classes when the plugin uses Thread.currentThread().getContextClassLoader().loadClass
MissingClassTelemetry.reportExceptionInside(e);
WebApp.get(j.servletContext).getSomeStapler().invoke(req, rsp, j, "/oops");
} catch (ServletException | IOException x) {
if (!Stapler.isSocketException(x)) {
throw x;
}
}
handleException(j, e, req, rsp, 500);
});
try {
Thread.setDefaultUncaughtExceptionHandler(new DefaultUncaughtExceptionHandler());
Expand All @@ -54,6 +49,46 @@ public static void init(final Jenkins j) throws IOException {
}
}

private static void handleException(Jenkins j, Throwable e, HttpServletRequest req, HttpServletResponse rsp, int code) throws IOException, ServletException {
if (rsp.isCommitted()) {
LOGGER.log(isEOFException(e) ? Level.FINE : Level.WARNING, null, e);
return;
}
String id = UUID.randomUUID().toString();
LOGGER.log(isEOFException(e) ? Level.FINE : Level.WARNING, "Caught unhandled exception with ID " + id, e);
req.setAttribute("jenkins.exception.id", id);
req.setAttribute("javax.servlet.error.exception",e);
rsp.setStatus(code);
try {
// If we have an exception, let's see if it's related with missing classes on Java 11. We reach
// here with a ClassNotFoundException in an action, for example. Setting the report here is the only
// way to catch the missing classes when the plugin uses Thread.currentThread().getContextClassLoader().loadClass
MissingClassTelemetry.reportExceptionInside(e);
WebApp.get(j.servletContext).getSomeStapler().invoke(req, rsp, j, "/oops");
} catch (ServletException | IOException x) {
if (!Stapler.isSocketException(x)) {
throw x;
}
}
}

@Restricted(NoExternalUse.class)
@MetaInfServices
public static class ErrorCustomizer implements HttpResponses.ErrorCustomizer {
@CheckForNull
@Override
public HttpResponses.HttpResponseException handleError(int code, Throwable cause) {
if (Jenkins.getInstanceOrNull() == null) {
return null;
}
return new HttpResponses.HttpResponseException(cause) {
public void generateResponse(StaplerRequest req, StaplerResponse rsp, Object node) throws IOException, ServletException {
handleException(Jenkins.get(), cause, req, rsp, code);
}
};
}
}

private static boolean isEOFException(Throwable e) {
if (e == null) {
return false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@ public User doCreateAccount(StaplerRequest req, StaplerResponse rsp) throws IOEx

private User _doCreateAccount(StaplerRequest req, StaplerResponse rsp, String formView) throws ServletException, IOException {
if(!allowsSignup())
throw HttpResponses.error(SC_UNAUTHORIZED,new Exception("User sign up is prohibited"));
throw HttpResponses.errorWithoutStack(SC_UNAUTHORIZED, "User sign up is prohibited");

boolean firstUser = !hasSomeUser();
User u = createAccount(req, rsp, enableCaptcha, formView);
Expand Down
7 changes: 7 additions & 0 deletions core/src/main/java/jenkins/model/Jenkins.java
Original file line number Diff line number Diff line change
Expand Up @@ -5147,6 +5147,12 @@ private static void computeVersion(ServletContext context) {
}
}

@Restricted(NoExternalUse.class)
public boolean shouldShowStackTrace() {
// Used by oops.jelly
return Boolean.getBoolean(Jenkins.class.getName() + ".SHOW_STACK_TRACE");
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given https://github.com/jenkinsci/jenkins-test-harness/pull/201/files#diff-1d6816bd6f9430280c5d8b7853286b1dR2688, I wonder whether a more classic approach (non-final field that is initialized from the system property) isn't a better approach to this.

Jenkins.SHOW_ERROR_STACK_TRACES = true; would be nicer over there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See this comment: #4389 (comment) . One approach would be to make it private, but that makes testing more difficult. On a different discussion Stephen Connolly presented an elaborate way to deal with that problem.

It's going to be difficult to have jenkins-test-harness do Jenkins.SHOW_ERROR_STACK_TRACES = true; because then JTH will depend upon a new Jenkins version and the new Jenkins needs the new JTH released for the tests to pass.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking existence of the field in JTH using reflection will cover both the previous Jenkins version without that field and the new one with it, being private.
(IIUC Java 11 supports this as long as it's not a final keyword removal :trollface:)

On a different discussion Stephen Connolly presented an elaborate way to deal with that problem.

Could you provide a link or at least a summary?


/**
* Hash of {@link #VERSION}.
*/
Expand Down Expand Up @@ -5233,6 +5239,7 @@ private static void computeVersion(ServletContext context) {
*/
static final String WORKSPACES_DIR_PROP = Jenkins.class.getName() + ".workspacesDir";


/**
* Automatically try to launch an agent when Jenkins is initialized or a new agent computer is created.
*/
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
/* Copyright 2004, 2005, 2006 Acegi Technology Pty Limited
* Copyright (c) 2020 CloudBees, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -41,6 +42,9 @@
import javax.servlet.http.HttpServletResponse;
import javax.servlet.http.HttpSession;
import java.io.IOException;
import java.util.Collections;
import java.util.IdentityHashMap;
import java.util.Set;
import java.util.logging.Level;
import java.util.logging.Logger;

Expand Down Expand Up @@ -122,8 +126,7 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha
catch (AuthenticationException | AccessDeniedException ex) {
handleException(request, response, chain, ex);
} catch (ServletException ex) {
if (ex.getRootCause() instanceof AuthenticationException
|| ex.getRootCause() instanceof AccessDeniedException) {
if (ex.getRootCause() instanceof AuthenticationException || ex.getRootCause() instanceof AccessDeniedException) {
handleException(request, response, chain, (AcegiSecurityException) ex.getRootCause());
}
else {
Expand Down Expand Up @@ -235,4 +238,5 @@ public void init(FilterConfig filterConfig) throws ServletException {

public void destroy() {
}
}

}
38 changes: 14 additions & 24 deletions core/src/main/resources/jenkins/model/Jenkins/oops.jelly
Original file line number Diff line number Diff line change
Expand Up @@ -27,32 +27,22 @@ THE SOFTWARE.
<!--
This is the page designated by web.xml and UncaughtExceptionHandler to process an exception thrown by us.
-->
<st:statusCode value="500" />
<l:layout title="Jenkins">
<st:statusCode value="${response.getStatus()}" />
<l:layout title="Jenkins" type="one-column">
<l:header />
<l:side-panel>
<l:task href="https://jenkins.io/" icon="icon-next icon-md" title="${%Jenkins project}"/>
<l:task href="https://jenkins.io/redirect/report-an-issue" icon="icon-gear2 icon-md" title="${%Bug tracker}"/>
<l:task href="https://jenkins.io/redirect/mailing-lists" icon="icon-search icon-md" title="${%Mailing Lists}"/>
<l:task href="https://twitter.com/jenkinsci" icon="icon-user icon-md" title="${%Twitter: @jenkinsci}"/>

</l:side-panel>
<l:main-panel>
<h1 style="text-align: center">
<img src="${imagesURL}/rage.png" height="179" width="154"/> <span style="font-size:50px"><st:nbsp/>${%Oops!}</span>
</h1>
<div id="error-description">
<p>
${%problemHappened}
${%checkJIRA}
${%vote}
${%pleaseReport}
${%stackTracePlease}
${%checkML}
</p>
<h2>${%Stack trace}</h2>
<pre style="margin:2em; clear:both">${h.printThrowable(request.getAttribute('javax.servlet.error.exception'))}</pre>
<h1 style="text-align: center">
<img src="${imagesURL}/rage.png" height="179" width="154"/> <span style="font-size:50px"><st:nbsp/>${%Oops!}</span>
</h1>
<div id="error-description">
<h2 style="text-align: center">${%problemHappened}</h2>
<p style="text-align: center">Logging ID=${request.getAttribute('jenkins.exception.id')}</p>
</div>
<j:if test="${app.shouldShowStackTrace()}">
<p>${%checkJIRA} ${%vote} ${%pleaseReport} ${%stackTracePlease} ${%checkML}</p>
<h2>${%Stack trace}</h2>
<pre style="margin:2em; clear:both">${h.printThrowable(request.getAttribute('javax.servlet.error.exception'))}</pre>
</j:if>
</l:main-panel>
</l:layout>
</j:jelly>
</j:jelly>
Original file line number Diff line number Diff line change
Expand Up @@ -47,3 +47,4 @@ ResourceDomainConfiguration.IOException=Failed to connect: {0}
ResourceDomainConfiguration.Invalid=Not a valid URL.
ResourceDomainConfiguration.SameAsJenkinsRoot=Cannot use the same host name for both Jenkins root URL and resource root URL.
ResourceDomainConfiguration.SameAsCurrent=You are currently accessing Jenkins through a URL similar to the proposed resource root URL. Saving this URL might remove your access to Jenkins.

8 changes: 5 additions & 3 deletions core/src/main/resources/lib/form/apply/apply.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,15 @@ Behaviour.specify("INPUT.apply-button", 'apply', 0, function (e) {
// otherwise this is possibly an error from the server, so we need to render the whole content.
var doc = target.contentDocument || target.contentWindow.document;
var error = doc.getElementById('error-description');
var r = YAHOO.util.Dom.getClientRegion();
var contentHeight = r.height/5;
var contentWidth = r.width/2;
if (!error) {
// fallback if it's not a regular error dialog from oops.jelly: use the entire body
error = Element('div', {id: 'error-description'});
error.appendChild(doc.getElementsByTagName('body')[0]);
contentHeight = r.height*3/4;
contentWidth = r.width*3/4;
}

if (oldError = $('error-description')) {
Expand All @@ -52,11 +57,8 @@ Behaviour.specify("INPUT.apply-button", 'apply', 0, function (e) {
}

$(containerId).appendChild(error);
var r = YAHOO.util.Dom.getClientRegion();

var contentHeight = r.height*3/4;
var dialogStyleHeight = contentHeight+40;
var contentWidth = r.width*3/4;
var dialogStyleWidth = contentWidth+20;

$(containerId).style.height = contentHeight+"px";
Expand Down
2 changes: 1 addition & 1 deletion pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ THE SOFTWARE.
<parent>
<groupId>org.jenkins-ci</groupId>
<artifactId>jenkins</artifactId>
<version>1.52</version>
<version>1.54</version>
<relativePath />
</parent>

Expand Down
2 changes: 1 addition & 1 deletion test/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ THE SOFTWARE.
<dependency>
<groupId>${project.groupId}</groupId>
<artifactId>jenkins-test-harness</artifactId>
<version>2.61</version>
<version>2.62</version>
<scope>test</scope>
<exclusions>
<exclusion>
Expand Down
Loading