From 11e3172c0382b5d69f63fa641be062b168f4047f Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Wed, 18 Sep 2019 19:05:42 +0100 Subject: [PATCH] exception: Let MediaWiki.php control final output for ErrorPageError The same way it does already for non-error output. This makes it so that doPreOutputCommit() consistently happens between the staging of output and the actual sending of output. It is still allowed for code to bypass this, such as for fatal errors and for handlers that disable OutputPage (like Special:Export). But for cases where we do want to perform doPreOutputCommit(), it should be run consistently between staging and sending so that it can make appropiate decisions based on the current state of OutputPage. Previously, the state of OutputPage seen by doPreOutputCommit() would be the broken/incomplete output of a seemingly succesful (possibly cacheable) user action, which would then, after doPreOutputCommit() runs, be completely replaced by $e->report()/ $out->showErrorPage(). This is a prerequisite for being able to reliably send cookie-block cookies on error pages (next patch). Bug: T233594 Change-Id: Iaeaf5e55a5868e6be534ddda73f3b56b9d6ef8f0 --- includes/MediaWiki.php | 6 +++++- includes/exception/BadRequestError.php | 4 ++-- includes/exception/ErrorPageError.php | 12 ++++++++++-- includes/exception/PermissionsError.php | 6 ++++-- includes/exception/ThrottledError.php | 4 ++-- includes/exception/UserNotLoggedIn.php | 8 +++++--- includes/filerepo/file/LocalFileLockError.php | 4 ++-- 7 files changed, 30 insertions(+), 14 deletions(-) diff --git a/includes/MediaWiki.php b/includes/MediaWiki.php index 28c9e16002..1a75714fbc 100644 --- a/includes/MediaWiki.php +++ b/includes/MediaWiki.php @@ -526,11 +526,15 @@ class MediaWiki { try { $this->main(); } catch ( ErrorPageError $e ) { + $out = $this->context->getOutput(); + // TODO: Should ErrorPageError::report accept a OutputPage parameter? + $e->report( ErrorPageError::STAGE_OUTPUT ); + // T64091: while exceptions are convenient to bubble up GUI errors, // they are not internal application faults. As with normal requests, this // should commit, print the output, do deferred updates, jobs, and profiling. $this->doPreOutputCommit(); - $e->report(); // display the GUI error + $out->output(); // display the GUI error } } catch ( Exception $e ) { $context = $this->context; diff --git a/includes/exception/BadRequestError.php b/includes/exception/BadRequestError.php index 5fcf0e6217..2448421afd 100644 --- a/includes/exception/BadRequestError.php +++ b/includes/exception/BadRequestError.php @@ -26,9 +26,9 @@ */ class BadRequestError extends ErrorPageError { - public function report() { + public function report( $action = self::SEND_OUTPUT ) { global $wgOut; $wgOut->setStatusCode( 400 ); - parent::report(); + parent::report( $action ); } } diff --git a/includes/exception/ErrorPageError.php b/includes/exception/ErrorPageError.php index 4b1812673f..64216a4f4c 100644 --- a/includes/exception/ErrorPageError.php +++ b/includes/exception/ErrorPageError.php @@ -25,6 +25,8 @@ * @ingroup Exception */ class ErrorPageError extends MWException implements ILocalizedException { + const SEND_OUTPUT = 0; + const STAGE_OUTPUT = 1; public $title, $msg, $params; /** @@ -60,13 +62,19 @@ class ErrorPageError extends MWException implements ILocalizedException { return wfMessage( $this->msg, $this->params ); } - public function report() { + public function report( $action = self::SEND_OUTPUT ) { if ( self::isCommandLine() || defined( 'MW_API' ) ) { parent::report(); } else { global $wgOut; $wgOut->showErrorPage( $this->title, $this->msg, $this->params ); - $wgOut->output(); + // Allow skipping of the final output step, so that web-based page views + // from MediaWiki.php, can inspect the staged OutputPage state, and perform + // graceful shutdown via doPreOutputCommit first, just like for regular + // output when there isn't an error page. + if ( $action === self::SEND_OUTPUT ) { + $wgOut->output(); + } } } } diff --git a/includes/exception/PermissionsError.php b/includes/exception/PermissionsError.php index 87a3dc2819..9fa1c7ce29 100644 --- a/includes/exception/PermissionsError.php +++ b/includes/exception/PermissionsError.php @@ -67,10 +67,12 @@ class PermissionsError extends ErrorPageError { parent::__construct( 'permissionserrors', Message::newFromSpecifier( $errors[0] ) ); } - public function report() { + public function report( $action = self::SEND_OUTPUT ) { global $wgOut; $wgOut->showPermissionsErrorPage( $this->errors, $this->permission ); - $wgOut->output(); + if ( $action === self::SEND_OUTPUT ) { + $wgOut->output(); + } } } diff --git a/includes/exception/ThrottledError.php b/includes/exception/ThrottledError.php index bec0d904be..cdeb402a73 100644 --- a/includes/exception/ThrottledError.php +++ b/includes/exception/ThrottledError.php @@ -32,9 +32,9 @@ class ThrottledError extends ErrorPageError { ); } - public function report() { + public function report( $action = ErrorPageError::SEND_OUTPUT ) { global $wgOut; $wgOut->setStatusCode( 429 ); - parent::report(); + parent::report( $action ); } } diff --git a/includes/exception/UserNotLoggedIn.php b/includes/exception/UserNotLoggedIn.php index 246c944f2e..ff992b07b7 100644 --- a/includes/exception/UserNotLoggedIn.php +++ b/includes/exception/UserNotLoggedIn.php @@ -75,11 +75,11 @@ class UserNotLoggedIn extends ErrorPageError { * Redirect to Special:Userlogin if the specified message is compatible. Otherwise, * show an error page as usual. */ - public function report() { + public function report( $action = self::SEND_OUTPUT ) { // If an unsupported message is used, don't try redirecting to Special:Userlogin, // since the message may not be compatible. if ( !in_array( $this->msg, LoginHelper::getValidErrorMessages() ) ) { - parent::report(); + parent::report( $action ); return; } @@ -99,6 +99,8 @@ class UserNotLoggedIn extends ErrorPageError { 'warning' => $this->msg, ] ) ); - $output->output(); + if ( $action === self::SEND_OUTPUT ) { + $output->output(); + } } } diff --git a/includes/filerepo/file/LocalFileLockError.php b/includes/filerepo/file/LocalFileLockError.php index 7cfc8c22c8..b76f3da2a0 100644 --- a/includes/filerepo/file/LocalFileLockError.php +++ b/includes/filerepo/file/LocalFileLockError.php @@ -29,9 +29,9 @@ class LocalFileLockError extends ErrorPageError { ); } - public function report() { + public function report( $action = self::SEND_OUTPUT ) { global $wgOut; $wgOut->setStatusCode( 429 ); - parent::report(); + parent::report( $action ); } } -- 2.20.1