Bug 1426721 - Encrypt Chrome logins in background thread r?MattN draft
authorDoug Thayer <dothayer@mozilla.com>
Thu, 04 Jan 2018 15:10:54 -0800
changeset 747790 085b8881d6f70c75b3950bebd8d2065d3430ac24
parent 747203 72ccc80c4ce58d48485497a71fb38cd37cd666ca
push id97009
push userbmo:dothayer@mozilla.com
push dateFri, 26 Jan 2018 21:30:33 +0000
reviewersMattN
bugs1426721
milestone60.0a1
Bug 1426721 - Encrypt Chrome logins in background thread r?MattN A significant chunk of migration jank that I observe locally happens due to login encryption. This patch reduces the locally observed jank (measured importing 100 logins) from 180ms to 25ms. Try is green, and as far as I can tell I don't see any thread safety issues, but I'm not 100% sure on that. I don't see any red flags inside the SecretDecoderRing::Encrypt implementation. I only moved Chrome logins over since I wanted to frontload any potential issues with the whole approach. It shouldn't be too hard to move the MSMigrationUtils and IEProfileMigrator uses over though. MozReview-Commit-ID: 75edUqJlk8x
browser/components/migration/ChromeProfileMigrator.js
browser/components/migration/MigrationUtils.jsm
toolkit/components/passwordmgr/LoginHelper.jsm
toolkit/components/passwordmgr/nsILoginManager.idl
toolkit/components/passwordmgr/nsILoginManagerStorage.idl
toolkit/components/passwordmgr/nsLoginManager.js
toolkit/components/passwordmgr/storage-json.js
toolkit/components/passwordmgr/storage-mozStorage.js
--- a/browser/components/migration/ChromeProfileMigrator.js
+++ b/browser/components/migration/ChromeProfileMigrator.js
@@ -403,17 +403,17 @@ function GetWindowsPasswordsResource(aPr
           aCallback(false);
         });
       // If the promise was rejected we will have already called aCallback,
       // so we can just return here.
       if (!rows) {
         return;
       }
       let crypto = new OSCrypto();
-
+      let logins = [];
       for (let row of rows) {
         try {
           let origin_url = NetUtil.newURI(row.getResultByName("origin_url"));
           // Ignore entries for non-http(s)/ftp URLs because we likely can't
           // use them anyway.
           const kValidSchemes = new Set(["https", "http", "ftp"]);
           if (!kValidSchemes.has(origin_url.scheme)) {
             continue;
@@ -445,21 +445,28 @@ function GetWindowsPasswordsResource(aPr
               // signon_realm format is URIrealm, so we need remove URI
               loginInfo.httpRealm = row.getResultByName("signon_realm")
                                        .substring(loginInfo.hostname.length + 1);
               break;
             default:
               throw new Error("Login data scheme type not supported: " +
                               row.getResultByName("scheme"));
           }
-          MigrationUtils.insertLoginWrapper(loginInfo);
+          logins.push(loginInfo);
         } catch (e) {
           Cu.reportError(e);
         }
       }
+      try {
+        if (logins.length > 0) {
+          await MigrationUtils.insertLoginsWrapper(logins);
+        }
+      } catch (e) {
+        Cu.reportError(e);
+      }
       crypto.finalize();
       aCallback(true);
     },
   };
 }
 
 ChromeProfileMigrator.prototype.classDescription = "Chrome Profile Migrator";
 ChromeProfileMigrator.prototype.contractID = "@mozilla.org/profile/migrator;1?app=browser&type=chrome";
--- a/browser/components/migration/MigrationUtils.jsm
+++ b/browser/components/migration/MigrationUtils.jsm
@@ -1016,16 +1016,30 @@ this.MigrationUtils = Object.freeze({
     // will not revert this. This seems preferable over removing the login
     // outright or storing the old password in the undo file.
     if (insertedLogin && gKeepUndoData) {
       let {guid, timePasswordChanged} = insertedLogin;
       gUndoData.get("logins").push({guid, timePasswordChanged});
     }
   },
 
+  async insertLoginsWrapper(logins) {
+    this._importQuantities.logins += logins.length;
+    let inserted = await LoginHelper.maybeImportLogins(logins);
+    // Note that this means that if we import a login that has a newer password
+    // than we know about, we will update the login, and an undo of the import
+    // will not revert this. This seems preferable over removing the login
+    // outright or storing the old password in the undo file.
+    if (gKeepUndoData) {
+      for (let {guid, timePasswordChanged} of inserted) {
+        gUndoData.get("logins").push({guid, timePasswordChanged});
+      }
+    }
+  },
+
   initializeUndoData() {
     gKeepUndoData = true;
     gUndoData = new Map([["bookmarks", []], ["visits", []], ["logins", []]]);
   },
 
   async _postProcessUndoData(state) {
     if (!state) {
       return state;
--- a/toolkit/components/passwordmgr/LoginHelper.jsm
+++ b/toolkit/components/passwordmgr/LoginHelper.jsm
@@ -205,16 +205,61 @@ this.LoginHelper = {
         // newURI will throw for some values e.g. chrome://FirefoxAccounts
         return false;
       }
     }
 
     return false;
   },
 
+  /**
+   * Helper to check if there are any duplicates of the existing login. If the
+   * duplicates need to have the password updated, this performs that update.
+   *
+   * @param {nsILoginInfo} aLogin - The login to search for.
+   * @return {boolean} - true if duplicates exist, otherwise false.
+   */
+  checkForDuplicatesAndMaybeUpdate(aLogin) {
+    // While here we're passing formSubmitURL and httpRealm, they could be empty/null and get
+    // ignored in that case, leading to multiple logins for the same username.
+    let existingLogins = Services.logins.findLogins({}, aLogin.hostname,
+                                                    aLogin.formSubmitURL,
+                                                    aLogin.httpRealm);
+    // Check for an existing login that matches *including* the password.
+    // If such a login exists, we do not need to add a new login.
+    if (existingLogins.some(l => aLogin.matches(l, false /* ignorePassword */))) {
+      return true;
+    }
+    // Now check for a login with the same username, where it may be that we have an
+    // updated password.
+    let foundMatchingLogin = false;
+    for (let existingLogin of existingLogins) {
+      if (aLogin.username == existingLogin.username) {
+        foundMatchingLogin = true;
+        existingLogin.QueryInterface(Ci.nsILoginMetaInfo);
+        if (aLogin.password != existingLogin.password &
+           aLogin.timePasswordChanged > existingLogin.timePasswordChanged) {
+          // if a login with the same username and different password already exists and it's older
+          // than the current one, update its password and timestamp.
+          let propBag = Cc["@mozilla.org/hash-property-bag;1"].
+                        createInstance(Ci.nsIWritablePropertyBag);
+          propBag.setProperty("password", aLogin.password);
+          propBag.setProperty("timePasswordChanged", aLogin.timePasswordChanged);
+          Services.logins.modifyLogin(existingLogin, propBag);
+        }
+      }
+    }
+    // if the new login is an update or is older than an exiting login, don't add it.
+    if (foundMatchingLogin) {
+      return true;
+    }
+
+    return false;
+  },
+
   doLoginsMatch(aLogin1, aLogin2, {
     ignorePassword = false,
     ignoreSchemes = false,
   }) {
     if (aLogin1.httpRealm != aLogin2.httpRealm ||
         aLogin1.username != aLogin2.username)
       return false;
 
@@ -566,53 +611,98 @@ this.LoginHelper = {
                loginData.usernameElement || "",
                loginData.passwordElement || "");
 
     login.QueryInterface(Ci.nsILoginMetaInfo);
     login.timeCreated = loginData.timeCreated;
     login.timeLastUsed = loginData.timeLastUsed || loginData.timeCreated;
     login.timePasswordChanged = loginData.timePasswordChanged || loginData.timeCreated;
     login.timesUsed = loginData.timesUsed || 1;
-    // While here we're passing formSubmitURL and httpRealm, they could be empty/null and get
-    // ignored in that case, leading to multiple logins for the same username.
-    let existingLogins = Services.logins.findLogins({}, login.hostname,
-                                                    login.formSubmitURL,
-                                                    login.httpRealm);
-    // Check for an existing login that matches *including* the password.
-    // If such a login exists, we do not need to add a new login.
-    if (existingLogins.some(l => login.matches(l, false /* ignorePassword */))) {
-      return null;
-    }
-    // Now check for a login with the same username, where it may be that we have an
-    // updated password.
-    let foundMatchingLogin = false;
-    for (let existingLogin of existingLogins) {
-      if (login.username == existingLogin.username) {
-        foundMatchingLogin = true;
-        existingLogin.QueryInterface(Ci.nsILoginMetaInfo);
-        if (login.password != existingLogin.password &
-           login.timePasswordChanged > existingLogin.timePasswordChanged) {
-          // if a login with the same username and different password already exists and it's older
-          // than the current one, update its password and timestamp.
-          let propBag = Cc["@mozilla.org/hash-property-bag;1"].
-                        createInstance(Ci.nsIWritablePropertyBag);
-          propBag.setProperty("password", login.password);
-          propBag.setProperty("timePasswordChanged", login.timePasswordChanged);
-          Services.logins.modifyLogin(existingLogin, propBag);
-        }
-      }
-    }
-    // if the new login is an update or is older than an exiting login, don't add it.
-    if (foundMatchingLogin) {
+    if (this.checkForDuplicatesAndMaybeUpdate(login)) {
       return null;
     }
     return Services.logins.addLogin(login);
   },
 
   /**
+   * Equivalent to maybeImportLogin, except asynchronous and for multiple logins.
+   * @param {Object[]} loginDatas - For each login, the data that needs to be added.
+   * @returns {nsILoginInfo[]} the newly added logins, filtered if no login was added.
+   */
+  async maybeImportLogins(loginDatas) {
+    let loginsToAdd = [];
+    let loginMap = new Map();
+    for (let loginData of loginDatas) {
+      // create a new login
+      let login = Cc["@mozilla.org/login-manager/loginInfo;1"].createInstance(Ci.nsILoginInfo);
+      login.init(loginData.hostname,
+                 loginData.formSubmitURL || (typeof(loginData.httpRealm) == "string" ? null : ""),
+                 typeof(loginData.httpRealm) == "string" ? loginData.httpRealm : null,
+                 loginData.username,
+                 loginData.password,
+                 loginData.usernameElement || "",
+                 loginData.passwordElement || "");
+
+      login.QueryInterface(Ci.nsILoginMetaInfo);
+      login.timeCreated = loginData.timeCreated;
+      login.timeLastUsed = loginData.timeLastUsed || loginData.timeCreated;
+      login.timePasswordChanged = loginData.timePasswordChanged || loginData.timeCreated;
+      login.timesUsed = loginData.timesUsed || 1;
+
+      try {
+        // Ensure we only send checked logins through, since the validation is optimized
+        // out from the bulk APIs below us.
+        this.checkLoginValues(login);
+      } catch (e) {
+        Cu.reportError(e);
+        continue;
+      }
+
+      // First, we need to check the logins that we've already decided to add, to
+      // see if this is a duplicate. This should mirror the logic inside
+      // checkForDuplicatesAndMaybeUpdate, but only for the array of logins we're
+      // adding.
+      let newLogins = loginMap.get(login.hostname) || [];
+      if (!newLogins) {
+        loginMap.set(login.hostname, newLogins);
+      } else {
+        if (newLogins.some(l => login.matches(l, false /* ignorePassword */))) {
+          continue;
+        }
+        let foundMatchingNewLogin = false;
+        for (let newLogin of newLogins) {
+          if (login.username == newLogin.username) {
+            foundMatchingNewLogin = true;
+            newLogin.QueryInterface(Ci.nsILoginMetaInfo);
+            if (login.password != newLogin.password &
+                login.timePasswordChanged > newLogin.timePasswordChanged) {
+              // if a login with the same username and different password already exists and it's older
+              // than the current one, update its password and timestamp.
+              newLogin.password = login.password;
+              newLogin.timePasswordChanged = login.timePasswordChanged;
+            }
+          }
+        }
+
+        if (foundMatchingNewLogin) {
+          continue;
+        }
+      }
+
+      if (this.checkForDuplicatesAndMaybeUpdate(login)) {
+        continue;
+      }
+
+      newLogins.push(login);
+      loginsToAdd.push(login);
+    }
+    return Services.logins.addLogins(loginsToAdd);
+  },
+
+  /**
    * Convert an array of nsILoginInfo to vanilla JS objects suitable for
    * sending over IPC.
    *
    * NB: All members of nsILoginInfo and nsILoginMetaInfo are strings.
    */
   loginsToVanillaObjects(logins) {
     return logins.map(this.loginToVanillaObject);
   },
--- a/toolkit/components/passwordmgr/nsILoginManager.idl
+++ b/toolkit/components/passwordmgr/nsILoginManager.idl
@@ -36,16 +36,30 @@ interface nsILoginManager : nsISupports 
    * Default values for the login's nsILoginMetaInfo properties will be
    * created. However, if the caller specifies non-default values, they will
    * be used instead.
    */
   nsILoginInfo addLogin(in nsILoginInfo aLogin);
 
 
   /**
+   * Like addLogin, but asynchronous and for many logins.
+   *
+   * @param aLogins
+   *        A JS Array of nsILoginInfos to add.
+   * @return A promise which resolves with a JS Array of cloned logins with
+   *         the guids set.
+   *
+   * Default values for each login's nsILoginMetaInfo properties will be
+   * created. However, if the caller specifies non-default values, they will
+   * be used instead.
+   */
+  jsval addLogins(in jsval aLogins);
+
+  /**
    * Remove a login from the login manager.
    *
    * @param aLogin
    *        The login to be removed.
    *
    * The specified login must exactly match a stored login. However, the
    * values of any nsILoginMetaInfo properties are ignored.
    */
--- a/toolkit/components/passwordmgr/nsILoginManagerStorage.idl
+++ b/toolkit/components/passwordmgr/nsILoginManagerStorage.idl
@@ -45,23 +45,25 @@ interface nsILoginManagerStorage : nsISu
   jsval terminate();
 
 
   /**
    * Store a new login in the storage module.
    *
    * @param aLogin
    *        The login to be added.
+   * @param aPreEncrypted
+   *        Whether the login was already encrypted or not.
    * @return a clone of the login info with the guid set (even if it was not provided).
    *
    * Default values for the login's nsILoginMetaInfo properties will be
    * created. However, if the caller specifies non-default values, they will
    * be used instead.
    */
-  nsILoginInfo addLogin(in nsILoginInfo aLogin);
+  nsILoginInfo addLogin(in nsILoginInfo aLogin, [optional] in boolean aPreEncrypted);
 
 
   /**
    * Remove a login from the storage module.
    *
    * @param aLogin
    *        The login to be removed.
    *
--- a/toolkit/components/passwordmgr/nsLoginManager.js
+++ b/toolkit/components/passwordmgr/nsLoginManager.js
@@ -302,16 +302,36 @@ LoginManager.prototype = {
     if (logins.some(l => login.matches(l, true))) {
       throw new Error("This login already exists.");
     }
 
     log.debug("Adding login");
     return this._storage.addLogin(login);
   },
 
+  async addLogins(logins) {
+    let crypto = Cc["@mozilla.org/login-manager/crypto/SDR;1"].
+                 getService(Ci.nsILoginManagerCrypto);
+    let plaintexts = logins.map(l => l.username).concat(logins.map(l => l.password));
+    let ciphertexts = await crypto.encryptMany(plaintexts);
+    let usernames = ciphertexts.slice(0, logins.length);
+    let passwords = ciphertexts.slice(logins.length);
+    for (let i = 0; i < logins.length; i++) {
+      let plaintextUsername = logins[i].username;
+      let plaintextPassword = logins[i].password;
+      logins[i].username = usernames[i];
+      logins[i].password = passwords[i];
+      log.debug("Adding login");
+      this._storage.addLogin(logins[i], true);
+      // Reset the username and password to keep the same guarantees as addLogin
+      logins[i].username = plaintextUsername;
+      logins[i].password = plaintextPassword;
+    }
+  },
+
   /**
    * Remove the specified login from the stored logins.
    */
   removeLogin(login) {
     log.debug("Removing login");
     return this._storage.removeLogin(login);
   },
 
--- a/toolkit/components/passwordmgr/storage-json.js
+++ b/toolkit/components/passwordmgr/storage-json.js
@@ -94,23 +94,25 @@ this.LoginManagerStorage_json.prototype 
    * Internal method used by regression tests only.  It is called before
    * replacing this storage module with a new instance.
    */
   terminate() {
     this._store._saver.disarm();
     return this._store._save();
   },
 
-  addLogin(login) {
+  addLogin(login, preEncrypted = false) {
     this._store.ensureDataReady();
 
     // Throws if there are bogus values.
     LoginHelper.checkLoginValues(login);
 
-    let [encUsername, encPassword, encType] = this._encryptLogin(login);
+    let [encUsername, encPassword, encType] = preEncrypted ?
+      [login.username, login.password, this._crypto.defaultEncType] :
+      this._encryptLogin(login);
 
     // Clone the login, so we don't modify the caller's object.
     let loginClone = login.clone();
 
     // Initialize the nsILoginMetaInfo fields, unless the caller gave us values
     loginClone.QueryInterface(Ci.nsILoginMetaInfo);
     if (loginClone.guid) {
       let guid = loginClone.guid;
--- a/toolkit/components/passwordmgr/storage-mozStorage.js
+++ b/toolkit/components/passwordmgr/storage-mozStorage.js
@@ -191,21 +191,23 @@ LoginManagerStorage_mozStorage.prototype
    * Internal method used by regression tests only.  It is called before
    * replacing this storage module with a new instance.
    */
   terminate() {
     return Promise.resolve();
   },
 
 
-  addLogin(login) {
+  addLogin(login, preEncrypted = false) {
     // Throws if there are bogus values.
     LoginHelper.checkLoginValues(login);
 
-    let [encUsername, encPassword, encType] = this._encryptLogin(login);
+    let [encUsername, encPassword, encType] = preEncrypted ?
+      [login.username, login.password, this._crypto.defaultEncType] :
+      this._encryptLogin(login);
 
     // Clone the login, so we don't modify the caller's object.
     let loginClone = login.clone();
 
     // Initialize the nsILoginMetaInfo fields, unless the caller gave us values
     loginClone.QueryInterface(Ci.nsILoginMetaInfo);
     if (loginClone.guid) {
       if (!this._isGuidUnique(loginClone.guid))