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
--- 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))