Bug 1426721 - Encrypt Chrome logins in background thread r=keeler,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

--HG--
extra : rebase_source : 0a8e16c46e05972fb01c9703b52cdb5755b0b40b

browser/components/migration/ChromeProfileMigrator.js

@@ -412,7 +412,7 @@ async function GetWindowsPasswordsResource(aProfileFolder) {
         return;
       }
       let crypto = new OSCrypto();
-
+      let logins = [];
       for (let row of rows) {
         try {
           let origin_url = NetUtil.newURI(row.getResultByName("origin_url"));
@@ -454,11 +454,18 @@ async function GetWindowsPasswordsResource(aProfileFolder) {
               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);
     },

browser/components/migration/MigrationUtils.jsm

@@ -1058,6 +1058,20 @@ this.MigrationUtils = Object.freeze({
     }
   },
 
+  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", []]]);

toolkit/components/passwordmgr/LoginHelper.jsm

@@ -210,6 +210,51 @@ this.LoginHelper = {
     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,
@@ -571,42 +616,87 @@ this.LoginHelper = {
     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.

toolkit/components/passwordmgr/nsILoginManager.idl

@@ -40,6 +40,20 @@ interface nsILoginManager : nsISupports {
   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.
    *

toolkit/components/passwordmgr/nsILoginManagerStorage.idl

@@ -50,13 +50,15 @@ interface nsILoginManagerStorage : nsISupports {
    *
    * @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);
 
 
   /**

toolkit/components/passwordmgr/nsLoginManager.js

@@ -307,6 +307,26 @@ LoginManager.prototype = {
     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.
    */

toolkit/components/passwordmgr/storage-json.js

@@ -99,13 +99,15 @@ this.LoginManagerStorage_json.prototype = {
     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();

toolkit/components/passwordmgr/storage-mozStorage.js

@@ -196,11 +196,13 @@ LoginManagerStorage_mozStorage.prototype = {
   },
 
 
-  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();