Bug 1139743 - Part 2: Make the 'current account data' less racey. r=ckarlof, a=lizzard
authorMark Hammond <mhammond@skippinet.com.au>
Fri, 03 Apr 2015 12:47:00 +1100
changeset 265515 e367fafd28d726bf618ac6935b2b98a973254c50
parent 265514 bbdecb2586cb973a053a5ac8dbf44e157a31d8fe
child 265516 5174d1db7188d06eba139f8b1a42edf432746358
push id4718
push userraliiev@mozilla.com
push dateMon, 11 May 2015 18:39:53 +0000
treeherdermozilla-beta@c20c4ef55f08 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersckarlof, lizzard
bugs1139743
milestone39.0a2
Bug 1139743 - Part 2: Make the 'current account data' less racey. r=ckarlof, a=lizzard
services/fxaccounts/FxAccounts.jsm
--- a/services/fxaccounts/FxAccounts.jsm
+++ b/services/fxaccounts/FxAccounts.jsm
@@ -76,16 +76,17 @@ let AccountState = function(fxaInternal)
 
 AccountState.prototype = {
   cert: null,
   keyPair: null,
   signedInUser: null,
   whenVerifiedDeferred: null,
   whenKeysReadyDeferred: null,
   profile: null,
+  promiseInitialAccountData: null,
 
   get isCurrent() this.fxaInternal && this.fxaInternal.currentAccountState === this,
 
   abort: function() {
     if (this.whenVerifiedDeferred) {
       this.whenVerifiedDeferred.reject(
         new Error("Verification aborted; Another user signing in"));
       this.whenVerifiedDeferred = null;
@@ -105,55 +106,67 @@ AccountState.prototype = {
 
     if (this.profile) {
       this.profile.tearDown();
       this.profile = null;
     }
   },
 
   getUserAccountData: function() {
-    // Skip disk if user is cached.
+    if (!this.isCurrent) {
+      return this.reject(new Error("Another user has signed in"));
+    }
+    if (this.promiseInitialAccountData) {
+      // We are still reading the data for the first and only time.
+      return this.promiseInitialAccountData;
+    }
+    // We've previously read it successfully (and possibly updated it since)
     if (this.signedInUser) {
       return this.resolve(this.signedInUser.accountData);
     }
 
-    return this.fxaInternal.signedInUserStorage.get().then(
+    // First and only read.
+    return this.promiseInitialAccountData = this.fxaInternal.signedInUserStorage.get().then(
       user => {
         if (logPII) {
           // don't stringify unless it will be written. We should replace this
           // check with param substitutions added in bug 966674
           log.debug("getUserAccountData -> " + JSON.stringify(user));
         }
         if (user && user.version == this.version) {
           log.debug("setting signed in user");
           this.signedInUser = user;
         }
+        this.promiseInitialAccountData = null;
         return this.resolve(user ? user.accountData : null);
       },
       err => {
+        this.promiseInitialAccountData = null;
         if (err instanceof OS.File.Error && err.becauseNoSuchFile) {
           // File hasn't been created yet.  That will be done
           // on the first call to getSignedInUser
           return this.resolve(null);
         }
         return this.reject(err);
       }
     );
   },
 
   setUserAccountData: function(accountData) {
-    return this.fxaInternal.signedInUserStorage.get().then(record => {
-      if (!this.isCurrent) {
-        return this.reject(new Error("Another user has signed in"));
-      }
-      record.accountData = accountData;
-      this.signedInUser = record;
-      return this.fxaInternal.signedInUserStorage.set(record)
+    if (!this.isCurrent) {
+      return this.reject(new Error("Another user has signed in"));
+    }
+    if (this.promiseInitialAccountData) {
+      throw new Error("Can't set account data before it's been read.");
+    }
+    // Set our signedInUser before we start the write, so any updates to the
+    // data while the write completes are still captured.
+    this.signedInUser = {version: DATA_FORMAT_VERSION, accountData: accountData};
+    return this.fxaInternal.signedInUserStorage.set(this.signedInUser)
         .then(() => this.resolve(accountData));
-    });
   },
 
 
   getCertificate: function(data, keyPair, mustBeValidUntil) {
     if (logPII) {
       // don't stringify unless it will be written. We should replace this
       // check with param substitutions added in bug 966674
       log.debug("getCertificate" + JSON.stringify(this.signedInUser));