From b9a1260ef5c01bbc912be9a2ff591b29e7d6b13c Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 11 Feb 2019 09:58:45 -0800 Subject: [PATCH] Improve top-level fatal exception handling in PHP 7+ Summary: Depends on D20137. Ref T13250. Ref T12101. In versions of PHP beyond 7, various engine errors are gradually changing from internal fatals or internal errors to `Throwables`, a superclass of `Exception`. This is generally a good change, but code written against PHP 5.x before `Throwable` was introduced may not catch these errors, even when the code is intended to be a top-level exception handler. (The double-catch pattern here and elsewhere is because `Throwable` does not exist in older PHP, so `catch (Throwable $ex)` catches nothing. The `Exception $ex` clause catches everything in old PHP, the `Throwable $ex` clause catches everything in newer PHP.) Generalize some `Exception` into `Throwable`. Test Plan: - Added a bogus function call to the rendering stack. - Before change: got a blank page. - After change: nice exception page. {F6205012} Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13250, T12101 Differential Revision: https://secure.phabricator.com/D20138 --- .../AphrontApplicationConfiguration.php | 9 ++- support/startup/PhabricatorStartup.php | 4 +- webroot/index.php | 55 ++++++++++++++++--- 3 files changed, 58 insertions(+), 10 deletions(-) diff --git a/src/aphront/configuration/AphrontApplicationConfiguration.php b/src/aphront/configuration/AphrontApplicationConfiguration.php index 350688d4fb..8c1b4b710f 100644 --- a/src/aphront/configuration/AphrontApplicationConfiguration.php +++ b/src/aphront/configuration/AphrontApplicationConfiguration.php @@ -286,6 +286,7 @@ final class AphrontApplicationConfiguration $original_exception = $ex; } + $response_exception = null; try { if ($original_exception) { $response = $this->handleThrowable($original_exception); @@ -296,7 +297,13 @@ final class AphrontApplicationConfiguration $response->setRequest($request); self::writeResponse($sink, $response); - } catch (Exception $response_exception) { + } catch (Exception $ex) { + $response_exception = $ex; + } catch (Throwable $ex) { + $response_exception = $ex; + } + + if ($response_exception) { // If we encountered an exception while building a normal response, then // encountered another exception while building a response for the first // exception, just throw the original exception. It is more likely to be diff --git a/support/startup/PhabricatorStartup.php b/support/startup/PhabricatorStartup.php index 1bfb74d886..4c577ca20c 100644 --- a/support/startup/PhabricatorStartup.php +++ b/support/startup/PhabricatorStartup.php @@ -315,7 +315,7 @@ final class PhabricatorStartup { * * @param string Brief description of the exception context, like * `"Rendering Exception"`. - * @param Exception The exception itself. + * @param Throwable The exception itself. * @param bool True if it's okay to show the exception's stack trace * to the user. The trace will always be logged. * @return exit This method **does not return**. @@ -324,7 +324,7 @@ final class PhabricatorStartup { */ public static function didEncounterFatalException( $note, - Exception $ex, + $ex, $show_trace) { $message = '['.$note.'/'.get_class($ex).'] '.$ex->getMessage(); diff --git a/webroot/index.php b/webroot/index.php index 5c7d79bfa1..6c3d66305e 100644 --- a/webroot/index.php +++ b/webroot/index.php @@ -2,6 +2,7 @@ phabricator_startup(); +$fatal_exception = null; try { PhabricatorStartup::beginStartupPhase('libraries'); PhabricatorStartup::loadCoreLibraries(); @@ -12,25 +13,65 @@ try { PhabricatorStartup::beginStartupPhase('sink'); $sink = new AphrontPHPHTTPSink(); + // PHP introduced a "Throwable" interface in PHP 7 and began making more + // runtime errors throw as "Throwable" errors. This is generally good, but + // makes top-level exception handling that is compatible with both PHP 5 + // and PHP 7 a bit tricky. + + // In PHP 5, "Throwable" does not exist, so "catch (Throwable $ex)" catches + // nothing. + + // In PHP 7, various runtime conditions raise an Error which is a Throwable + // but NOT an Exception, so "catch (Exception $ex)" will not catch them. + + // To cover both cases, we "catch (Exception $ex)" to catch everything in + // PHP 5, and most things in PHP 7. Then, we "catch (Throwable $ex)" to catch + // everything else in PHP 7. For the most part, we only need to do this at + // the top level. + + $main_exception = null; try { PhabricatorStartup::beginStartupPhase('run'); AphrontApplicationConfiguration::runHTTPRequest($sink); } catch (Exception $ex) { + $main_exception = $ex; + } catch (Throwable $ex) { + $main_exception = $ex; + } + + if ($main_exception) { + $response_exception = null; try { $response = new AphrontUnhandledExceptionResponse(); - $response->setException($ex); + $response->setException($main_exception); PhabricatorStartup::endOutputCapture(); $sink->writeResponse($response); - } catch (Exception $response_exception) { - // If we hit a rendering exception, ignore it and throw the original - // exception. It is generally more interesting and more likely to be - // the root cause. - throw $ex; + } catch (Exception $ex) { + $response_exception = $ex; + } catch (Throwable $ex) { + $response_exception = $ex; + } + + // If we hit a rendering exception, ignore it and throw the original + // exception. It is generally more interesting and more likely to be + // the root cause. + + if ($response_exception) { + throw $main_exception; } } } catch (Exception $ex) { - PhabricatorStartup::didEncounterFatalException('Core Exception', $ex, false); + $fatal_exception = $ex; +} catch (Throwable $ex) { + $fatal_exception = $ex; +} + +if ($fatal_exception) { + PhabricatorStartup::didEncounterFatalException( + 'Core Exception', + $fatal_exception, + false); } function phabricator_startup() {