From c11474714cb1e38b50e329c59854ed40a269d888 Mon Sep 17 00:00:00 2001
From: Aaron Schulz <aschulz@wikimedia.org>
Date: Wed, 7 Oct 2015 13:01:20 -0700
Subject: [PATCH] Avoid use of rollback() in WikiPage::doEditContent()

* Use the CAS style page row checks instead
* This a first step towards switching to startAtomic/endAtomic
* Only a sanity check uses rollback() now, which should never
  be hit unless there is a serious DB layer bug

Change-Id: Ideb189f918dee5d3e3c7b91cb92179df514ef35a
---
 includes/page/WikiPage.php | 58 +++++++++++++++++++++-----------------
 1 file changed, 32 insertions(+), 26 deletions(-)

diff --git a/includes/page/WikiPage.php b/includes/page/WikiPage.php
index fe648215f6..bdb84c6c39 100644
--- a/includes/page/WikiPage.php
+++ b/includes/page/WikiPage.php
@@ -1242,7 +1242,7 @@ class WikiPage implements Page, IDBAccessObject {
 	 *   Giving 0 indicates the new page flag should be set on.
 	 * @param bool $lastRevIsRedirect If given, will optimize adding and
 	 *   removing rows in redirect table.
-	 * @return bool True on success, false on failure
+	 * @return bool Success; false if the page row was missing or page_latest changed
 	 */
 	public function updateRevisionOn( $dbw, $revision, $lastRevision = null,
 		$lastRevIsRedirect = null
@@ -1804,31 +1804,36 @@ class WikiPage implements Page, IDBAccessObject {
 			$changed = !$content->equals( $old_content );
 
 			if ( $changed ) {
-				$dbw->begin( __METHOD__ );
-
 				$prepStatus = $content->prepareSave( $this, $flags, $oldid, $user );
 				$status->merge( $prepStatus );
 
 				if ( !$status->isOK() ) {
-					$dbw->rollback( __METHOD__ );
-
 					return $status;
 				}
-				$revisionId = $revision->insertOn( $dbw );
-
-				// Update page.
-				// We check for conflicts by comparing $oldid with the current latest revision ID.
-				$ok = $this->updateRevisionOn( $dbw, $revision, $oldid, $oldIsRedirect );
 
-				if ( !$ok ) {
-					// Belated edit conflict! Run away!!
+				$dbw->begin( __METHOD__ );
+				// Get the latest page_latest value while locking it.
+				// Do a CAS style check to see if it's the same as when this method
+				// started. If it changed then bail out before touching the DB.
+				$latestNow = $this->lock();
+				if ( $latestNow != $oldid ) {
+					$dbw->commit( __METHOD__ );
+					// Page updated or deleted in the mean time
 					$status->fatal( 'edit-conflict' );
 
-					$dbw->rollback( __METHOD__ );
-
 					return $status;
 				}
 
+				// At this point we are now comitted to returning an OK
+				// status unless some DB query error or other exception comes up.
+				// This way callers don't have to call rollback() if $status is bad
+				// unless they actually try to catch exceptions (which is rare).
+
+				$revisionId = $revision->insertOn( $dbw );
+
+				// Update page_latest and friends to reflect the new revision
+				$this->updateRevisionOn( $dbw, $revision, null, $oldIsRedirect );
+
 				Hooks::run( 'NewRevisionFromEditComplete',
 					array( $this, $revision, $baseRevId, $user ) );
 
@@ -1876,30 +1881,28 @@ class WikiPage implements Page, IDBAccessObject {
 			// Create new article
 			$status->value['new'] = true;
 
-			$dbw->begin( __METHOD__ );
-
 			$prepStatus = $content->prepareSave( $this, $flags, $oldid, $user );
 			$status->merge( $prepStatus );
-
 			if ( !$status->isOK() ) {
-				$dbw->rollback( __METHOD__ );
-
 				return $status;
 			}
 
-			$status->merge( $prepStatus );
+			$dbw->begin( __METHOD__ );
 
-			// Add the page record; stake our claim on this title!
-			// This will return false if the article already exists
+			// Add the page record unless one already exists for the title
 			$newid = $this->insertOn( $dbw );
-
 			if ( $newid === false ) {
-				$dbw->rollback( __METHOD__ );
+				$dbw->commit( __METHOD__ ); // nothing inserted
 				$status->fatal( 'edit-already-exists' );
 
-				return $status;
+				return $status; // nothing done
 			}
 
+			// At this point we are now comitted to returning an OK
+			// status unless some DB query error or other exception comes up.
+			// This way callers don't have to call rollback() if $status is bad
+			// unless they actually try to catch exceptions (which is rare).
+
 			// Save the revision text...
 			$revision = new Revision( array(
 				'page'       => $newid,
@@ -1924,7 +1927,10 @@ class WikiPage implements Page, IDBAccessObject {
 			}
 
 			// Update the page record with revision data
-			$this->updateRevisionOn( $dbw, $revision, 0 );
+			if ( !$this->updateRevisionOn( $dbw, $revision, 0 ) ) {
+				$dbw->rollback( __METHOD__ );
+				throw new MWException( "Failed to update page row to use new revision." );
+			}
 
 			Hooks::run( 'NewRevisionFromEditComplete', array( $this, $revision, false, $user ) );
 
-- 
2.20.1