From 187356fea5155b5895b9e4c07344476c09196dc1 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 11 Feb 2019 13:00:53 -0800 Subject: [PATCH] Let the top-level exception handler dump a stack trace if we reach debug mode before things go sideways Summary: Depends on D20140. Ref T13250. Currently, the top-level exception handler doesn't dump stacks because we might not be in debug mode, and we might double-extra-super fatal if we call `PhabricatorEnv:...` to try to figure out if we're in debug mode or not. We can get around this by setting a flag on the Sink once we're able to confirm that we're in debug mode. Then it's okay for the top-level error handler to show traces. There's still some small possibility that showing a trace could make us double-super-fatal since we have to call a little more code, but AphrontStackTraceView is pretty conservative about what it does and 99% of the time this is a huge improvement. Test Plan: {F6205122} Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13250 Differential Revision: https://secure.phabricator.com/D20142 --- resources/celerity/map.php | 4 +- .../AphrontApplicationConfiguration.php | 6 +++ .../AphrontUnhandledExceptionResponse.php | 43 ++++++++++++++++++- src/aphront/sink/AphrontHTTPSink.php | 14 ++++-- src/view/widget/AphrontStackTraceView.php | 1 - webroot/index.php | 1 + .../config/unhandled-exception.css | 32 +++++++++++++- 7 files changed, 91 insertions(+), 10 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index cb44dd5585..99f3333106 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -46,7 +46,7 @@ return array( 'rsrc/css/application/config/config-options.css' => '16c920ae', 'rsrc/css/application/config/config-template.css' => '20babf50', 'rsrc/css/application/config/setup-issue.css' => '5eed85b2', - 'rsrc/css/application/config/unhandled-exception.css' => '9da8fdab', + 'rsrc/css/application/config/unhandled-exception.css' => '9ecfc00d', 'rsrc/css/application/conpherence/color.css' => 'b17746b0', 'rsrc/css/application/conpherence/durable-column.css' => '2d57072b', 'rsrc/css/application/conpherence/header-pane.css' => 'c9a3db8e', @@ -877,7 +877,7 @@ return array( 'syntax-highlighting-css' => '8a16f91b', 'tokens-css' => 'ce5a50bd', 'typeahead-browse-css' => 'b7ed02d2', - 'unhandled-exception-css' => '9da8fdab', + 'unhandled-exception-css' => '9ecfc00d', ), 'requires' => array( '01384686' => array( diff --git a/src/aphront/configuration/AphrontApplicationConfiguration.php b/src/aphront/configuration/AphrontApplicationConfiguration.php index 8c1b4b710f..8cd27fa62b 100644 --- a/src/aphront/configuration/AphrontApplicationConfiguration.php +++ b/src/aphront/configuration/AphrontApplicationConfiguration.php @@ -118,6 +118,12 @@ final class AphrontApplicationConfiguration $database_exception = $ex; } + // If we're in developer mode, set a flag so that top-level exception + // handlers can add more information. + if (PhabricatorEnv::getEnvConfig('phabricator.developer-mode')) { + $sink->setShowStackTraces(true); + } + if ($database_exception) { $issue = PhabricatorSetupIssue::newDatabaseConnectionIssue( $database_exception, diff --git a/src/aphront/response/AphrontUnhandledExceptionResponse.php b/src/aphront/response/AphrontUnhandledExceptionResponse.php index efd9d70ead..32d612ca50 100644 --- a/src/aphront/response/AphrontUnhandledExceptionResponse.php +++ b/src/aphront/response/AphrontUnhandledExceptionResponse.php @@ -4,8 +4,20 @@ final class AphrontUnhandledExceptionResponse extends AphrontStandaloneHTMLResponse { private $exception; + private $showStackTraces; + + public function setShowStackTraces($show_stack_traces) { + $this->showStackTraces = $show_stack_traces; + return $this; + } + + public function getShowStackTraces() { + return $this->showStackTraces; + } + + public function setException($exception) { + // NOTE: We accept an Exception or a Throwable. - public function setException(Exception $exception) { // Log the exception unless it's specifically a silent malformed request // exception. @@ -61,10 +73,36 @@ final class AphrontUnhandledExceptionResponse $body = $ex->getMessage(); $body = phutil_escape_html_newlines($body); + $classes = array(); + $classes[] = 'unhandled-exception-detail'; + + $stack = null; + if ($this->getShowStackTraces()) { + try { + $stack = id(new AphrontStackTraceView()) + ->setTrace($ex->getTrace()); + + $stack = hsprintf('%s', $stack); + + $stack = phutil_tag( + 'div', + array( + 'class' => 'unhandled-exception-stack', + ), + $stack); + + $classes[] = 'unhandled-exception-with-stack'; + } catch (Exception $trace_exception) { + $stack = null; + } catch (Throwable $trace_exception) { + $stack = null; + } + } + return phutil_tag( 'div', array( - 'class' => 'unhandled-exception-detail', + 'class' => implode(' ', $classes), ), array( phutil_tag( @@ -79,6 +117,7 @@ final class AphrontUnhandledExceptionResponse 'class' => 'unhandled-exception-body', ), $body), + $stack, )); } diff --git a/src/aphront/sink/AphrontHTTPSink.php b/src/aphront/sink/AphrontHTTPSink.php index 51c54df520..60deba78ca 100644 --- a/src/aphront/sink/AphrontHTTPSink.php +++ b/src/aphront/sink/AphrontHTTPSink.php @@ -5,14 +5,22 @@ * Normally this is just @{class:AphrontPHPHTTPSink}, which uses "echo" and * "header()" to emit responses. * - * Mostly, this class allows us to do install security or metrics hooks in the - * output pipeline. - * * @task write Writing Response Components * @task emit Emitting the Response */ abstract class AphrontHTTPSink extends Phobject { + private $showStackTraces = false; + + final public function setShowStackTraces($show_stack_traces) { + $this->showStackTraces = $show_stack_traces; + return $this; + } + + final public function getShowStackTraces() { + return $this->showStackTraces; + } + /* -( Writing Response Components )---------------------------------------- */ diff --git a/src/view/widget/AphrontStackTraceView.php b/src/view/widget/AphrontStackTraceView.php index 1d0616df3d..edb805af8f 100644 --- a/src/view/widget/AphrontStackTraceView.php +++ b/src/view/widget/AphrontStackTraceView.php @@ -10,7 +10,6 @@ final class AphrontStackTraceView extends AphrontView { } public function render() { - $user = $this->getUser(); $trace = $this->trace; $libraries = PhutilBootloader::getInstance()->getAllLibraries(); diff --git a/webroot/index.php b/webroot/index.php index 6c3d66305e..0014edfa2c 100644 --- a/webroot/index.php +++ b/webroot/index.php @@ -44,6 +44,7 @@ try { try { $response = new AphrontUnhandledExceptionResponse(); $response->setException($main_exception); + $response->setShowStackTraces($sink->getShowStackTraces()); PhabricatorStartup::endOutputCapture(); $sink->writeResponse($response); diff --git a/webroot/rsrc/css/application/config/unhandled-exception.css b/webroot/rsrc/css/application/config/unhandled-exception.css index 831148cdad..cd8ad313dc 100644 --- a/webroot/rsrc/css/application/config/unhandled-exception.css +++ b/webroot/rsrc/css/application/config/unhandled-exception.css @@ -8,12 +8,12 @@ background: #fff; border: 1px solid #c0392b; border-radius: 3px; - padding: 0 8px; + padding: 8px; } .unhandled-exception-detail .unhandled-exception-title { color: #c0392b; - padding: 12px 8px; + padding: 4px 8px 12px; border-bottom: 1px solid #f4dddb; font-size: 16px; font-weight: 500; @@ -23,3 +23,31 @@ .unhandled-exception-detail .unhandled-exception-body { padding: 16px 12px; } + +.unhandled-exception-with-stack { + max-width: 95%; +} + +.unhandled-exception-stack { + background: #fcfcfc; + overflow-x: auto; + overflow-y: hidden; +} + +.unhandled-exception-stack table { + border-spacing: 0; + border-collapse: collapse; + width: 100%; + border: 1px solid #d7d7d7; +} + +.unhandled-exception-stack th { + background: #e7e7e7; + border-bottom: 1px solid #d7d7d7; + padding: 8px; +} + +.unhandled-exception-stack td { + padding: 4px 8px; + white-space: nowrap; +}