(bug 16020) Fix race condition in User::addToDatabase()

Fix the DB error which comes from User::addToDatabase() if it is called
when the user already exists. This is the most common DB error we log at
WMF in normal operation, perhaps because of double clicks on the "create
account" button, or perhaps due to CentralAuth autocreation when
multiple pages on another wiki are opened in the browser simultaneously,
as the bug reporter suggests.

See the doc comment for the interface rationale. Patched
Special:Userlogin to be aware of the new return value. Most extension
callers will continue to work, I will patch a couple that need it in
subsequent commits.

Change-Id: I1f6ef5e6319bfe692fb82a3fa50dc66c9fde8f15

diff --git a/includes/Status.php b/includes/Status.php
index 10dfb51..147b0df 100644 (file)
--- a/includes/Status.php
+++ b/includes/Status.php
@@ -225,6 +225,15 @@ class Status {
                }
        }
 
+       /**
+        * Get the error message as HTML. This is done by parsing the wikitext error 
+        * message.
+        */
+       public function getHTML( $shortContext = false, $longContext = false ) {
+               $text = $this->getWikiText( $shortContext, $longContext );
+               return MessageCache::singleton()->transform( $text, true );
+       }
+
        /**
         * Return an array with the wikitext for each item in the array.
         * @param $errors Array

diff --git a/includes/User.php b/includes/User.php
index 4ab90ed..a786284 100644 (file)
--- a/includes/User.php
+++ b/includes/User.php
@@ -3008,7 +3008,29 @@ class User {
        }
 
        /**
-        * Add this existing user object to the database
+        * Add this existing user object to the database. If the user already 
+        * exists, a fatal status object is returned, and the user object is 
+        * initialised with the data from the database.
+        *
+        * Previously, this function generated a DB error due to a key conflict
+        * if the user already existed. Many extension callers use this function
+        * in code along the lines of:
+        *
+        *   $user = User::newFromName( $name );
+        *   if ( !$user->isLoggedIn() ) {
+        *       $user->addToDatabase();
+        *   }
+        *   // do something with $user...
+        *
+        * However, this was vulnerable to a race condition (bug 16020). By 
+        * initialising the user object if the user exists, we aim to support this
+        * calling sequence as far as possible.
+        *
+        * Note that if the user exists, this function will acquire a write lock,
+        * so it is still advisable to make the call conditional on isLoggedIn(), 
+        * and to commit the transaction after calling.
+        *
+        * @return Status
         */
        public function addToDatabase() {
                $this->load();
@@ -3031,14 +3053,31 @@ class User {
                                'user_registration' => $dbw->timestamp( $this->mRegistration ),
                                'user_editcount' => 0,
                                'user_touched' => $dbw->timestamp( $this->mTouched ),
-                       ), __METHOD__
+                       ), __METHOD__,
+                       array( 'IGNORE' )
                );
+               if ( !$dbw->affectedRows() ) {
+                       $this->mId = $dbw->selectField( 'user', 'user_id', 
+                               array( 'user_name' => $this->mName ), __METHOD__ );
+                       $loaded = false;
+                       if ( $this->mId ) {
+                               if ( $this->loadFromDatabase() ) {
+                                       $loaded = true;
+                               }
+                       }
+                       if ( !$loaded ) {
+                               throw new MWException( __METHOD__. ": hit a key conflict attempting " .
+                                       "to insert a user row, but then it doesn't exist when we select it!" );
+                       }
+                       return Status::newFatal( 'userexists' );
+               }
                $this->mId = $dbw->insertId();
 
                // Clear instance cache other than user table data, which is already accurate
                $this->clearInstanceCache();
 
                $this->saveOptions();
+               return Status::newGood();
        }
 
        /**

diff --git a/includes/specials/SpecialUserlogin.php b/includes/specials/SpecialUserlogin.php
index b7d01c8..64bd723 100644 (file)
--- a/includes/specials/SpecialUserlogin.php
+++ b/includes/specials/SpecialUserlogin.php
@@ -442,7 +442,13 @@ class LoginForm extends SpecialPage {
                }
 
                self::clearCreateaccountToken();
-               return $this->initUser( $u, false );
+
+               $status = $this->initUser( $u, false );
+               if ( !$status->isOK() ) {
+                       $this->mainLoginForm( $status->getHTML() );
+                       return false;
+               }
+               return $status->value;
        }
 
        /**
@@ -451,13 +457,16 @@ class LoginForm extends SpecialPage {
         *
         * @param $u User object.
         * @param $autocreate boolean -- true if this is an autocreation via auth plugin
-        * @return User object.
+        * @return Status object, with the User object in the value member on success
         * @private
         */
        function initUser( $u, $autocreate ) {
                global $wgAuth;
 
-               $u->addToDatabase();
+               $status = $u->addToDatabase();
+               if ( !$status->isOK() ) {
+                       return $status;
+               }
 
                if ( $wgAuth->allowPasswordChange() ) {
                        $u->setPassword( $this->mPassword );
@@ -483,7 +492,7 @@ class LoginForm extends SpecialPage {
                # Update user count
                DeferredUpdates::addUpdate( new SiteStatsUpdate( 0, 0, 0, 0, 1 ) );
 
-               return $u;
+               return Status::newGood( $u );
        }
 
        /**
@@ -729,7 +738,14 @@ class LoginForm extends SpecialPage {
                }
 
                wfDebug( __METHOD__ . ": creating account\n" );
-               $this->initUser( $user, true );
+               $status = $this->initUser( $user, true );
+
+               if ( !$status->isOK() ) {
+                       $errors = $status->getErrorsByType( 'error' );
+                       $this->mAbortLoginErrorMsg = $errors[0]['message'];
+                       return self::ABORTED;
+               }
+
                return self::SUCCESS;
        }