From 806a2214e29fa3d35daef6469c1ea14dba2957ab Mon Sep 17 00:00:00 2001
From: Aaron Schulz <aschulz@wikimedia.org>
Date: Fri, 30 Jun 2017 15:01:33 -0700
Subject: [PATCH] Always log exceptions in rollbackMasterChangesAndLog()

MWExceptionHandler::rollbackMasterChangesAndLog() only logged exceptions
if there were already master changes. This is extremely problematic when
debugging, especially in situations like DeferredUpdates where they were
silently being swallowed.

This makes it log exceptions in all paths, erring on the side of logging
the same exception twice (theoretically it's possible I suppose) instead
of not at all.

Also make the method able to handle DBError exceptions, which most of
the callers seemed to be assuming. ApiMain was handling this explicitly.

Bug: T168347
Change-Id: I8739051f824a455ba669344184c3b11ac95cb561
---
 includes/api/ApiMain.php                  | 13 +-----
 includes/exception/MWExceptionHandler.php | 50 +++++++++--------------
 2 files changed, 20 insertions(+), 43 deletions(-)

diff --git a/includes/api/ApiMain.php b/includes/api/ApiMain.php
index 5cb7967d33..52f79eec2a 100644
--- a/includes/api/ApiMain.php
+++ b/includes/api/ApiMain.php
@@ -581,23 +581,12 @@ class ApiMain extends ApiBase {
 		// T65145: Rollback any open database transactions
 		if ( !( $e instanceof ApiUsageException || $e instanceof UsageException ) ) {
 			// UsageExceptions are intentional, so don't rollback if that's the case
-			try {
-				MWExceptionHandler::rollbackMasterChangesAndLog( $e );
-			} catch ( DBError $e2 ) {
-				// Rollback threw an exception too. Log it, but don't interrupt
-				// our regularly scheduled exception handling.
-				MWExceptionHandler::logException( $e2 );
-			}
+			MWExceptionHandler::rollbackMasterChangesAndLog( $e );
 		}
 
 		// Allow extra cleanup and logging
 		Hooks::run( 'ApiMain::onException', [ $this, $e ] );
 
-		// Log it
-		if ( !( $e instanceof ApiUsageException || $e instanceof UsageException ) ) {
-			MWExceptionHandler::logException( $e );
-		}
-
 		// Handle any kind of exception by outputting properly formatted error message.
 		// If this fails, an unhandled exception should be thrown so that global error
 		// handler will process and log it.
diff --git a/includes/exception/MWExceptionHandler.php b/includes/exception/MWExceptionHandler.php
index 433274e339..ef8136687d 100644
--- a/includes/exception/MWExceptionHandler.php
+++ b/includes/exception/MWExceptionHandler.php
@@ -83,29 +83,32 @@ class MWExceptionHandler {
 	}
 
 	/**
-	 * If there are any open database transactions, roll them back and log
-	 * the stack trace of the exception that should have been caught so the
-	 * transaction could be aborted properly.
+	 * Roll back any open database transactions and log the stack trace of the exception
+	 *
+	 * This method is used to attempt to recover from exceptions
 	 *
 	 * @since 1.23
 	 * @param Exception|Throwable $e
 	 */
 	public static function rollbackMasterChangesAndLog( $e ) {
 		$services = MediaWikiServices::getInstance();
-		if ( $services->isServiceDisabled( 'DBLoadBalancerFactory' ) ) {
-			return; // T147599
+		if ( !$services->isServiceDisabled( 'DBLoadBalancerFactory' ) ) {
+			// Rollback DBs to avoid transaction notices. This might fail
+			// to rollback some databases due to connection issues or exceptions.
+			// However, any sane DB driver will rollback implicitly anyway.
+			try {
+				$services->getDBLoadBalancerFactory()->rollbackMasterChanges( __METHOD__ );
+			} catch ( DBError $e2 ) {
+				// If the DB is unreacheable, rollback() will throw an error
+				// and the error report() method might need messages from the DB,
+				// which would result in an exception loop. PHP may escalate such
+				// errors to "Exception thrown without a stack frame" fatals, but
+				// it's better to be explicit here.
+				self::logException( $e2, self::CAUGHT_BY_HANDLER );
+			}
 		}
 
-		$lbFactory = $services->getDBLoadBalancerFactory();
-		if ( $lbFactory->hasMasterChanges() ) {
-			$logger = LoggerFactory::getInstance( 'Bug56269' );
-			$logger->warning(
-				'Exception thrown with an uncommited database transaction: ' .
-				self::getLogMessage( $e ),
-				self::getLogContext( $e )
-			);
-		}
-		$lbFactory->rollbackMasterChanges( __METHOD__ );
+		self::logException( $e, self::CAUGHT_BY_HANDLER );
 	}
 
 	/**
@@ -123,23 +126,8 @@ class MWExceptionHandler {
 	 * @param Exception|Throwable $e
 	 */
 	public static function handleException( $e ) {
-		try {
-			// Rollback DBs to avoid transaction notices. This may fail
-			// to rollback some DB due to connection issues or exceptions.
-			// However, any sane DB driver will rollback implicitly anyway.
-			self::rollbackMasterChangesAndLog( $e );
-		} catch ( DBError $e2 ) {
-			// If the DB is unreacheable, rollback() will throw an error
-			// and the error report() method might need messages from the DB,
-			// which would result in an exception loop. PHP may escalate such
-			// errors to "Exception thrown without a stack frame" fatals, but
-			// it's better to be explicit here.
-			self::logException( $e2, self::CAUGHT_BY_HANDLER );
-		}
-
-		self::logException( $e, self::CAUGHT_BY_HANDLER );
+		self::rollbackMasterChangesAndLog( $e );
 		self::report( $e );
-
 		// Exit value should be nonzero for the benefit of shell jobs
 		exit( 1 );
 	}
-- 
2.20.1