From f66ef1b7b2a64ad82c2f505711d92304279d6c16 Mon Sep 17 00:00:00 2001 From: Brian Wolff Date: Sun, 6 Apr 2014 00:16:37 -0300 Subject: [PATCH] Make doEditContent call $dbw->rollback() if exception happens doEditContent calls a variety of methods in the middle of its database transaction that can throw exceptions. For example if $revision->insertOn() throws an exception, it can cause referential integrity issues unless the transaction is rolled back by something further up the chain (Thus see I5807e, I8f1da5). It seems like it would be more reliable if doEditContent rolled back its own transaction in exceptional circumstances. So this patch puts everything in a try block, and does rollback if an exception happens. Bug: 63145 Bug: 32551 Change-Id: Icc44687da4edb1a72f13d2b95bfee2eea3ad6808 --- includes/WikiPage.php | 192 ++++++++++++++++++++++-------------------- 1 file changed, 102 insertions(+), 90 deletions(-) diff --git a/includes/WikiPage.php b/includes/WikiPage.php index 18409b0a7b..a5d66dfdc9 100644 --- a/includes/WikiPage.php +++ b/includes/WikiPage.php @@ -1812,56 +1812,62 @@ class WikiPage implements Page, IDBAccessObject { } $dbw->begin( __METHOD__ ); + try { - $prepStatus = $content->prepareSave( $this, $flags, $baseRevId, $user ); - $status->merge( $prepStatus ); + $prepStatus = $content->prepareSave( $this, $flags, $baseRevId, $user ); + $status->merge( $prepStatus ); - if ( !$status->isOK() ) { - $dbw->rollback( __METHOD__ ); + if ( !$status->isOK() ) { + $dbw->rollback( __METHOD__ ); - wfProfileOut( __METHOD__ ); - return $status; - } + wfProfileOut( __METHOD__ ); + return $status; + } + $revisionId = $revision->insertOn( $dbw ); - $revisionId = $revision->insertOn( $dbw ); + // Update page + // + // Note that we use $this->mLatest instead of fetching a value from the master DB + // during the course of this function. This makes sure that EditPage can detect + // edit conflicts reliably, either by $ok here, or by $article->getTimestamp() + // before this function is called. A previous function used a separate query, this + // creates a window where concurrent edits can cause an ignored edit conflict. + $ok = $this->updateRevisionOn( $dbw, $revision, $oldid, $oldIsRedirect ); - // Update page - // - // Note that we use $this->mLatest instead of fetching a value from the master DB - // during the course of this function. This makes sure that EditPage can detect - // edit conflicts reliably, either by $ok here, or by $article->getTimestamp() - // before this function is called. A previous function used a separate query, this - // creates a window where concurrent edits can cause an ignored edit conflict. - $ok = $this->updateRevisionOn( $dbw, $revision, $oldid, $oldIsRedirect ); + if ( !$ok ) { + // Belated edit conflict! Run away!! + $status->fatal( 'edit-conflict' ); - if ( !$ok ) { - // Belated edit conflict! Run away!! - $status->fatal( 'edit-conflict' ); + $dbw->rollback( __METHOD__ ); - $dbw->rollback( __METHOD__ ); - - wfProfileOut( __METHOD__ ); - return $status; - } + wfProfileOut( __METHOD__ ); + return $status; + } - wfRunHooks( 'NewRevisionFromEditComplete', array( $this, $revision, $baseRevId, $user ) ); - // Update recentchanges - if ( !( $flags & EDIT_SUPPRESS_RC ) ) { - // Mark as patrolled if the user can do so - $patrolled = $wgUseRCPatrol && !count( + wfRunHooks( 'NewRevisionFromEditComplete', array( $this, $revision, $baseRevId, $user ) ); + // Update recentchanges + if ( !( $flags & EDIT_SUPPRESS_RC ) ) { + // Mark as patrolled if the user can do so + $patrolled = $wgUseRCPatrol && !count( $this->mTitle->getUserPermissionsErrors( 'autopatrol', $user ) ); - // Add RC row to the DB - $rc = RecentChange::notifyEdit( $now, $this->mTitle, $isminor, $user, $summary, - $oldid, $this->getTimestamp(), $bot, '', $oldsize, $newsize, - $revisionId, $patrolled - ); + // Add RC row to the DB + $rc = RecentChange::notifyEdit( $now, $this->mTitle, $isminor, $user, $summary, + $oldid, $this->getTimestamp(), $bot, '', $oldsize, $newsize, + $revisionId, $patrolled + ); - // Log auto-patrolled edits - if ( $patrolled ) { - PatrolLog::record( $rc, true, $user ); + // Log auto-patrolled edits + if ( $patrolled ) { + PatrolLog::record( $rc, true, $user ); + } } + $user->incEditCount(); + } catch ( MWException $e ) { + $dbw->rollback( __METHOD__ ); + // Question: Would it perhaps be better if this method turned all + // exceptions into $status's? + throw $e; } - $user->incEditCount(); $dbw->commit( __METHOD__ ); } else { // Bug 32948: revision ID must be set to page {{REVISIONID}} and @@ -1891,74 +1897,80 @@ class WikiPage implements Page, IDBAccessObject { $status->value['new'] = true; $dbw->begin( __METHOD__ ); + try { - $prepStatus = $content->prepareSave( $this, $flags, $baseRevId, $user ); - $status->merge( $prepStatus ); + $prepStatus = $content->prepareSave( $this, $flags, $baseRevId, $user ); + $status->merge( $prepStatus ); - if ( !$status->isOK() ) { - $dbw->rollback( __METHOD__ ); + if ( !$status->isOK() ) { + $dbw->rollback( __METHOD__ ); - wfProfileOut( __METHOD__ ); - return $status; - } + wfProfileOut( __METHOD__ ); + return $status; + } - $status->merge( $prepStatus ); + $status->merge( $prepStatus ); - // Add the page record; stake our claim on this title! - // This will return false if the article already exists - $newid = $this->insertOn( $dbw ); + // Add the page record; stake our claim on this title! + // This will return false if the article already exists + $newid = $this->insertOn( $dbw ); - if ( $newid === false ) { - $dbw->rollback( __METHOD__ ); - $status->fatal( 'edit-already-exists' ); + if ( $newid === false ) { + $dbw->rollback( __METHOD__ ); + $status->fatal( 'edit-already-exists' ); - wfProfileOut( __METHOD__ ); - return $status; - } + wfProfileOut( __METHOD__ ); + return $status; + } - // Save the revision text... - $revision = new Revision( array( - 'page' => $newid, - 'title' => $this->getTitle(), // for determining the default content model - 'comment' => $summary, - 'minor_edit' => $isminor, - 'text' => $serialized, - 'len' => $newsize, - 'user' => $user->getId(), - 'user_text' => $user->getName(), - 'timestamp' => $now, - 'content_model' => $content->getModel(), - 'content_format' => $serialisation_format, - ) ); - $revisionId = $revision->insertOn( $dbw ); + // Save the revision text... + $revision = new Revision( array( + 'page' => $newid, + 'title' => $this->getTitle(), // for determining the default content model + 'comment' => $summary, + 'minor_edit' => $isminor, + 'text' => $serialized, + 'len' => $newsize, + 'user' => $user->getId(), + 'user_text' => $user->getName(), + 'timestamp' => $now, + 'content_model' => $content->getModel(), + 'content_format' => $serialisation_format, + ) ); + $revisionId = $revision->insertOn( $dbw ); - // Bug 37225: use accessor to get the text as Revision may trim it - $content = $revision->getContent(); // sanity; get normalized version + // Bug 37225: use accessor to get the text as Revision may trim it + $content = $revision->getContent(); // sanity; get normalized version - if ( $content ) { - $newsize = $content->getSize(); - } + if ( $content ) { + $newsize = $content->getSize(); + } - // Update the page record with revision data - $this->updateRevisionOn( $dbw, $revision, 0 ); + // Update the page record with revision data + $this->updateRevisionOn( $dbw, $revision, 0 ); - wfRunHooks( 'NewRevisionFromEditComplete', array( $this, $revision, false, $user ) ); + wfRunHooks( 'NewRevisionFromEditComplete', array( $this, $revision, false, $user ) ); - // Update recentchanges - if ( !( $flags & EDIT_SUPPRESS_RC ) ) { - // Mark as patrolled if the user can do so - $patrolled = ( $wgUseRCPatrol || $wgUseNPPatrol ) && !count( - $this->mTitle->getUserPermissionsErrors( 'autopatrol', $user ) ); - // Add RC row to the DB - $rc = RecentChange::notifyNew( $now, $this->mTitle, $isminor, $user, $summary, $bot, - '', $newsize, $revisionId, $patrolled ); + // Update recentchanges + if ( !( $flags & EDIT_SUPPRESS_RC ) ) { + // Mark as patrolled if the user can do so + $patrolled = ( $wgUseRCPatrol || $wgUseNPPatrol ) && !count( + $this->mTitle->getUserPermissionsErrors( 'autopatrol', $user ) ); + // Add RC row to the DB + $rc = RecentChange::notifyNew( $now, $this->mTitle, $isminor, $user, $summary, $bot, + '', $newsize, $revisionId, $patrolled ); - // Log auto-patrolled edits - if ( $patrolled ) { - PatrolLog::record( $rc, true, $user ); + // Log auto-patrolled edits + if ( $patrolled ) { + PatrolLog::record( $rc, true, $user ); + } } + $user->incEditCount(); + + } catch ( MWException $e ) { + $dbw->rollback( __METHOD__ ); + throw $e; } - $user->incEditCount(); $dbw->commit( __METHOD__ ); // Update links, etc. -- 2.20.1