Improve top-level exception handling
Summary:
Fixes T6692. Addresses two main issues:
- The write guard would sometimes not get disposed of on exception pathways, generating an unnecessary secondary error which was just a symptom of the original root error.
- This was generally confusing and reduced the quality of reports we received because users would report the symptomatic error sometimes instead of the real error.
- Instead, reflow the handling so that we always dispose of the write guard if we create one.
- If we missed the Controller-level error page generation (normally, a nice page with full CSS, etc), we'd jump straight to Startup-level error page generation (very basic plain text).
- A large class of errors occur too early or too late to be handled by Controller-level pages, but many of these errors are not fundamental, and the plain text page is excessively severe.
- Provide a mid-level simple HTML error page for errors which can't get full CSS, but also aren't so fundamental that we have no recourse but plain text.
Test Plan:
Mid-level errors now produce an intentional-looking error page:
{F259885}
Verified that setup errors still render properly.
@chad, feel free to tweak the exception page -- I just did a rough pass on it. Like the setup error stuff, it doesn't have Celerity, so we can't use `{$colors}` and no other CSS will be loaded.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley, chad
Maniphest Tasks: T6692
Differential Revision: https://secure.phabricator.com/D11126
This commit is contained in:
@@ -54,6 +54,192 @@ abstract class AphrontApplicationConfiguration {
|
||||
public function willBuildRequest() {}
|
||||
|
||||
|
||||
/**
|
||||
* @phutil-external-symbol class PhabricatorStartup
|
||||
*/
|
||||
public static function runHTTPRequest(AphrontHTTPSink $sink) {
|
||||
PhabricatorEnv::initializeWebEnvironment();
|
||||
|
||||
$debug_time_limit = PhabricatorEnv::getEnvConfig('debug.time-limit');
|
||||
if ($debug_time_limit) {
|
||||
PhabricatorStartup::setDebugTimeLimit($debug_time_limit);
|
||||
}
|
||||
|
||||
// This is the earliest we can get away with this, we need env config first.
|
||||
PhabricatorAccessLog::init();
|
||||
$access_log = PhabricatorAccessLog::getLog();
|
||||
PhabricatorStartup::setGlobal('log.access', $access_log);
|
||||
$access_log->setData(
|
||||
array(
|
||||
'R' => AphrontRequest::getHTTPHeader('Referer', '-'),
|
||||
'r' => idx($_SERVER, 'REMOTE_ADDR', '-'),
|
||||
'M' => idx($_SERVER, 'REQUEST_METHOD', '-'),
|
||||
));
|
||||
|
||||
DarkConsoleXHProfPluginAPI::hookProfiler();
|
||||
DarkConsoleErrorLogPluginAPI::registerErrorHandler();
|
||||
|
||||
$response = PhabricatorSetupCheck::willProcessRequest();
|
||||
if ($response) {
|
||||
PhabricatorStartup::endOutputCapture();
|
||||
$sink->writeResponse($response);
|
||||
return;
|
||||
}
|
||||
|
||||
$host = AphrontRequest::getHTTPHeader('Host');
|
||||
$path = $_REQUEST['__path__'];
|
||||
|
||||
switch ($host) {
|
||||
default:
|
||||
$config_key = 'aphront.default-application-configuration-class';
|
||||
$application = PhabricatorEnv::newObjectFromConfig($config_key);
|
||||
break;
|
||||
}
|
||||
|
||||
$application->setHost($host);
|
||||
$application->setPath($path);
|
||||
$application->willBuildRequest();
|
||||
$request = $application->buildRequest();
|
||||
|
||||
// Build the server URI implied by the request headers. If an administrator
|
||||
// has not configured "phabricator.base-uri" yet, we'll use this to generate
|
||||
// links.
|
||||
|
||||
$request_protocol = ($request->isHTTPS() ? 'https' : 'http');
|
||||
$request_base_uri = "{$request_protocol}://{$host}/";
|
||||
PhabricatorEnv::setRequestBaseURI($request_base_uri);
|
||||
|
||||
$access_log->setData(
|
||||
array(
|
||||
'U' => (string)$request->getRequestURI()->getPath(),
|
||||
));
|
||||
|
||||
$write_guard = new AphrontWriteGuard(array($request, 'validateCSRF'));
|
||||
|
||||
$processing_exception = null;
|
||||
try {
|
||||
$response = $application->processRequest($request, $access_log, $sink);
|
||||
$response_code = $response->getHTTPResponseCode();
|
||||
} catch (Exception $ex) {
|
||||
$processing_exception = $ex;
|
||||
$response_code = 500;
|
||||
}
|
||||
|
||||
$write_guard->dispose();
|
||||
|
||||
$access_log->setData(
|
||||
array(
|
||||
'c' => $response_code,
|
||||
'T' => PhabricatorStartup::getMicrosecondsSinceStart(),
|
||||
));
|
||||
|
||||
$access_log->write();
|
||||
|
||||
DarkConsoleXHProfPluginAPI::saveProfilerSample($access_log);
|
||||
|
||||
// Add points to the rate limits for this request.
|
||||
if (isset($_SERVER['REMOTE_ADDR'])) {
|
||||
$user_ip = $_SERVER['REMOTE_ADDR'];
|
||||
|
||||
// The base score for a request allows users to make 30 requests per
|
||||
// minute.
|
||||
$score = (1000 / 30);
|
||||
|
||||
// If the user was logged in, let them make more requests.
|
||||
if ($request->getUser() && $request->getUser()->getPHID()) {
|
||||
$score = $score / 5;
|
||||
}
|
||||
|
||||
PhabricatorStartup::addRateLimitScore($user_ip, $score);
|
||||
}
|
||||
|
||||
if ($processing_exception) {
|
||||
throw $processing_exception;
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
public function processRequest(
|
||||
AphrontRequest $request,
|
||||
PhutilDeferredLog $access_log,
|
||||
AphrontHTTPSink $sink) {
|
||||
|
||||
$this->setRequest($request);
|
||||
|
||||
list($controller, $uri_data) = $this->buildController();
|
||||
|
||||
$access_log->setData(
|
||||
array(
|
||||
'C' => get_class($controller),
|
||||
));
|
||||
|
||||
$request->setURIMap($uri_data);
|
||||
$controller->setRequest($request);
|
||||
|
||||
// If execution throws an exception and then trying to render that
|
||||
// exception throws another exception, we want to show the original
|
||||
// exception, as it is likely the root cause of the rendering exception.
|
||||
$original_exception = null;
|
||||
try {
|
||||
$response = $controller->willBeginExecution();
|
||||
|
||||
if ($request->getUser() && $request->getUser()->getPHID()) {
|
||||
$access_log->setData(
|
||||
array(
|
||||
'u' => $request->getUser()->getUserName(),
|
||||
'P' => $request->getUser()->getPHID(),
|
||||
));
|
||||
}
|
||||
|
||||
if (!$response) {
|
||||
$controller->willProcessRequest($uri_data);
|
||||
$response = $controller->handleRequest($request);
|
||||
}
|
||||
} catch (Exception $ex) {
|
||||
$original_exception = $ex;
|
||||
$response = $this->handleException($ex);
|
||||
}
|
||||
|
||||
try {
|
||||
$response = $controller->didProcessRequest($response);
|
||||
$response = $this->willSendResponse($response, $controller);
|
||||
$response->setRequest($request);
|
||||
|
||||
$unexpected_output = PhabricatorStartup::endOutputCapture();
|
||||
if ($unexpected_output) {
|
||||
$unexpected_output = pht(
|
||||
"Unexpected output:\n\n%s",
|
||||
$unexpected_output);
|
||||
|
||||
phlog($unexpected_output);
|
||||
|
||||
if ($response instanceof AphrontWebpageResponse) {
|
||||
echo phutil_tag(
|
||||
'div',
|
||||
array('style' =>
|
||||
'background: #eeddff;'.
|
||||
'white-space: pre-wrap;'.
|
||||
'z-index: 200000;'.
|
||||
'position: relative;'.
|
||||
'padding: 8px;'.
|
||||
'font-family: monospace',
|
||||
),
|
||||
$unexpected_output);
|
||||
}
|
||||
}
|
||||
|
||||
$sink->writeResponse($response);
|
||||
} catch (Exception $ex) {
|
||||
if ($original_exception) {
|
||||
throw $original_exception;
|
||||
}
|
||||
throw $ex;
|
||||
}
|
||||
|
||||
return $response;
|
||||
}
|
||||
|
||||
|
||||
/* -( URI Routing )-------------------------------------------------------- */
|
||||
|
||||
|
||||
|
||||
Reference in New Issue
Block a user