Bug 1433593 - Clean up usages of LoginHelper.maybeImportLogin r?MattN draft
authorDoug Thayer <dothayer@mozilla.com>
Thu, 15 Feb 2018 10:26:44 -0800
changeset 759195 5b11c18c647ad00aa0e938b96908524ad0edc5b8
parent 754572 e43f2f6ea111c2d059d95fa9a71516b869a69698
child 759196 c50552ead3c092705112b991669891d1f726ef51
push id100304
push userbmo:dothayer@mozilla.com
push dateFri, 23 Feb 2018 21:18:38 +0000
reviewersMattN
bugs1433593, 1426721
milestone60.0a1
Bug 1433593 - Clean up usages of LoginHelper.maybeImportLogin r?MattN In bug 1426721 we added a bulk interface for importing logins, which works in a background thread. This patch cleans up the single-login interface and updates the remaining usages to consume the bulk interface. MozReview-Commit-ID: IziLXkO5dxQ
browser/components/migration/IEProfileMigrator.js
browser/components/migration/MSMigrationUtils.jsm
browser/components/migration/MigrationUtils.jsm
browser/components/migration/tests/unit/test_IE7_passwords.js
browser/components/migration/tests/unit/test_automigration.js
toolkit/components/passwordmgr/LoginHelper.jsm
toolkit/components/passwordmgr/nsLoginManager.js
toolkit/components/passwordmgr/test/unit/test_maybeImportLogin.js
--- a/browser/components/migration/IEProfileMigrator.js
+++ b/browser/components/migration/IEProfileMigrator.js
@@ -117,41 +117,41 @@ IE7FormPasswords.prototype = {
       let count = key.valueCount;
       key.close();
       return count > 0;
     } catch (e) {
       return false;
     }
   },
 
-  migrate(aCallback) {
+  async migrate(aCallback) {
     let historyEnumerator = Cc["@mozilla.org/profile/migrator/iehistoryenumerator;1"].
                             createInstance(Ci.nsISimpleEnumerator);
     let uris = []; // the uris of the websites that are going to be migrated
     while (historyEnumerator.hasMoreElements()) {
       let entry = historyEnumerator.getNext().QueryInterface(Ci.nsIPropertyBag2);
       let uri = entry.get("uri").QueryInterface(Ci.nsIURI);
       // MSIE stores some types of URLs in its history that we don't handle, like HTMLHelp
       // and others. Since we are not going to import the logins that are performed in these URLs
       // we can just skip them.
       if (!["http", "https", "ftp"].includes(uri.scheme)) {
         continue;
       }
 
       uris.push(uri);
     }
-    this._migrateURIs(uris);
+    await this._migrateURIs(uris);
     aCallback(true);
   },
 
   /**
    * Migrate the logins that were saved for the uris arguments.
    * @param {nsIURI[]} uris - the uris that are going to be migrated.
    */
-  _migrateURIs(uris) {
+  async _migrateURIs(uris) {
     this.ctypesKernelHelpers = new MSMigrationUtils.CtypesKernelHelpers();
     this._crypto = new OSCrypto();
     let nsIWindowsRegKey = Ci.nsIWindowsRegKey;
     let key = Cc["@mozilla.org/windows-registry-key;1"].
               createInstance(nsIWindowsRegKey);
     key.open(nsIWindowsRegKey.ROOT_KEY_CURRENT_USER, kLoginsKey,
              nsIWindowsRegKey.ACCESS_READ);
 
@@ -161,16 +161,17 @@ IE7FormPasswords.prototype = {
     /* The logins are stored in the registry, where the key is a hashed URL and its
      * value contains the encrypted details for all logins for that URL.
      *
      * First iterate through IE history, hashing each URL and looking for a match. If
      * found, decrypt the value, using the URL as a salt. Finally add any found logins
      * to the Firefox password manager.
      */
 
+    let logins = [];
     for (let uri of uris) {
       try {
         // remove the query and the ref parts of the URL
         let urlObject = new URL(uri.spec);
         let url = urlObject.origin + urlObject.pathname;
         // if the current url is already processed, it should be skipped
         if (urlsSet.has(url)) {
           continue;
@@ -195,21 +196,33 @@ IE7FormPasswords.prototype = {
         }
         // extract the login details from the decrypted data
         let ieLogins = this._extractDetails(data, uri);
         // if at least a credential was found in the current data, successfullyDecryptedValues should
         // be incremented by one
         if (ieLogins.length) {
           successfullyDecryptedValues++;
         }
-        this._addLogins(ieLogins);
+        for (let ieLogin of ieLogins) {
+          logins.push({
+            username: ieLogin.username,
+            password: ieLogin.password,
+            hostname: ieLogin.url,
+            timeCreated: ieLogin.creation,
+          });
+        }
       } catch (e) {
         Cu.reportError("Error while importing logins for " + uri.spec + ": " + e);
       }
     }
+
+    if (logins.length > 0) {
+      await MigrationUtils.insertLoginsWrapper(logins);
+    }
+
     // if the number of the imported values is less than the number of values in the key, it means
     // that not all the values were imported and an error should be reported
     if (successfullyDecryptedValues < key.valueCount) {
       Cu.reportError("We failed to decrypt and import some logins. " +
                      "This is likely because we didn't find the URLs where these " +
                      "passwords were submitted in the IE history and which are needed to be used " +
                      "as keys in the decryption.");
     }
@@ -217,37 +230,16 @@ IE7FormPasswords.prototype = {
     key.close();
     this._crypto.finalize();
     this.ctypesKernelHelpers.finalize();
   },
 
   _crypto: null,
 
   /**
-   * Add the logins to the password manager.
-   * @param {Object[]} logins - array of the login details.
-   */
-  _addLogins(ieLogins) {
-    for (let ieLogin of ieLogins) {
-      try {
-        // create a new login
-        let login = {
-          username: ieLogin.username,
-          password: ieLogin.password,
-          hostname: ieLogin.url,
-          timeCreated: ieLogin.creation,
-        };
-        MigrationUtils.insertLoginWrapper(login);
-      } catch (e) {
-        Cu.reportError(e);
-      }
-    }
-  },
-
-  /**
    * Extract the details of one or more logins from the raw decrypted data.
    * @param {string} data - the decrypted data containing raw information.
    * @param {nsURI} uri - the nsURI of page where the login has occur.
    * @returns {Object[]} array of objects where each of them contains the username, password, URL,
    * and creation time representing all the logins found in the data arguments.
    */
   _extractDetails(data, uri) {
     // the structure of the header of the IE7 decrypted data for all the logins sharing the same URL
--- a/browser/components/migration/MSMigrationUtils.jsm
+++ b/browser/components/migration/MSMigrationUtils.jsm
@@ -743,17 +743,17 @@ WindowsVaultFormPasswords.prototype = {
    * Otherwise, check if there are passwords in the vault.
    * @param {function} aCallback - a callback called when the migration is done.
    * @param {boolean} [aOnlyCheckExists=false] - if aOnlyCheckExists is true, just check if there are some
    * passwords to migrate. Import the passwords from the vault and call aCallback otherwise.
    * @return true if there are passwords in the vault and aOnlyCheckExists is set to true,
    * false if there is no password in the vault and aOnlyCheckExists is set to true, undefined if
    * aOnlyCheckExists is set to false.
    */
-  migrate(aCallback, aOnlyCheckExists = false) {
+  async migrate(aCallback, aOnlyCheckExists = false) {
     // check if the vault item is an IE/Edge one
     function _isIEOrEdgePassword(id) {
       return id[0] == INTERNET_EXPLORER_EDGE_GUID[0] &&
              id[1] == INTERNET_EXPLORER_EDGE_GUID[1] &&
              id[2] == INTERNET_EXPLORER_EDGE_GUID[2] &&
              id[3] == INTERNET_EXPLORER_EDGE_GUID[3];
     }
 
@@ -780,16 +780,18 @@ WindowsVaultFormPasswords.prototype = {
       // enumerate all the available items. This api is going to return a table of all the
       // available items and item is going to point to the first element of this table.
       error = ctypesVaultHelpers._functions.VaultEnumerateItems(vault, VAULT_ENUMERATE_ALL_ITEMS,
                                                                 itemCount.address(),
                                                                 item.address());
       if (error != RESULT_SUCCESS) {
         throw new Error("Unable to enumerate Vault items: " + error);
       }
+
+      let logins = [];
       for (let j = 0; j < itemCount.value; j++) {
         try {
           // if it's not an ie/edge password, skip it
           if (!_isIEOrEdgePassword(item.contents.schemaId.id)) {
             continue;
           }
           let url = item.contents.pResourceElement.contents.itemValue.readString();
           let realURL;
@@ -825,36 +827,39 @@ WindowsVaultFormPasswords.prototype = {
             // to seconds since epoch and multiply to get milliseconds:
             creation = ctypesKernelHelpers.
                          fileTimeToSecondsSinceEpoch(item.contents.highLastModified,
                                                      item.contents.lowLastModified) * 1000;
           } catch (ex) {
             // Ignore exceptions in the dates and just create the login for right now.
           }
           // create a new login
-          let login = {
+          logins.push({
             username, password,
             hostname: realURL.prePath,
             timeCreated: creation,
-          };
-          MigrationUtils.insertLoginWrapper(login);
+          });
 
           // close current item
           error = ctypesVaultHelpers._functions.VaultFree(credential);
           if (error == FREE_CLOSE_FAILED) {
             throw new Error("Unable to free item: " + error);
           }
         } catch (e) {
           migrationSucceeded = false;
           Cu.reportError(e);
         } finally {
           // move to next item in the table returned by VaultEnumerateItems
           item = item.increment();
         }
       }
+
+      if (logins.length > 0) {
+        await MigrationUtils.insertLoginsWrapper(logins);
+      }
     } catch (e) {
       Cu.reportError(e);
       migrationSucceeded = false;
     } finally {
       if (successfulVaultOpen) {
         // close current vault
         error = ctypesVaultHelpers._functions.VaultCloseVault(vault);
         if (error == FREE_CLOSE_FAILED) {
--- a/browser/components/migration/MigrationUtils.jsm
+++ b/browser/components/migration/MigrationUtils.jsm
@@ -1029,29 +1029,16 @@ this.MigrationUtils = Object.freeze({
   insertVisitsWrapper(places, options) {
     this._importQuantities.history += places.length;
     if (gKeepUndoData) {
       this._updateHistoryUndo(places);
     }
     return PlacesUtils.asyncHistory.updatePlaces(places, options, true);
   },
 
-  insertLoginWrapper(login) {
-    this._importQuantities.logins++;
-    let insertedLogin = LoginHelper.maybeImportLogin(login);
-    // 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 (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) {
--- a/browser/components/migration/tests/unit/test_IE7_passwords.js
+++ b/browser/components/migration/tests/unit/test_IE7_passwords.js
@@ -111,16 +111,27 @@ const TESTED_WEBSITES = {
       },
     ],
   },
   reddit: {
     uri: makeURI("http://www.reddit.com/"),
     hash: "B644028D1C109A91EC2C4B9D1F145E55A1FAE42065",
     data: [12, 0, 0, 0, 152, 0, 0, 0, 212, 0, 0, 0, 87, 73, 67, 75, 24, 0, 0, 0, 8, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 32, 8, 234, 114, 153, 212, 208, 1, 1, 0, 0, 0, 4, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 6, 0, 0, 0, 97, 93, 131, 116, 153, 212, 208, 1, 3, 0, 0, 0, 14, 0, 0, 0, 97, 93, 131, 116, 153, 212, 208, 1, 16, 0, 0, 0, 48, 0, 0, 0, 88, 150, 78, 174, 153, 212, 208, 1, 4, 0, 0, 0, 58, 0, 0, 0, 88, 150, 78, 174, 153, 212, 208, 1, 29, 0, 0, 0, 118, 0, 0, 0, 79, 102, 137, 34, 154, 212, 208, 1, 15, 0, 0, 0, 150, 0, 0, 0, 79, 102, 137, 34, 154, 212, 208, 1, 30, 0, 0, 0, 97, 0, 0, 0, 0, 0, 252, 140, 173, 138, 146, 48, 0, 0, 66, 0, 105, 0, 116, 0, 116, 0, 101, 0, 32, 0, 98, 0, 101, 0, 115, 0, 116, 0, 228, 0, 116, 0, 105, 0, 103, 0, 101, 0, 110, 0, 0, 0, 205, 145, 110, 127, 198, 91, 1, 120, 0, 0, 31, 4, 48, 4, 64, 4, 62, 4, 59, 4, 76, 4, 32, 0, 67, 4, 65, 4, 63, 4, 53, 4, 72, 4, 61, 4, 62, 4, 32, 0, 65, 4, 49, 4, 64, 4, 62, 4, 72, 4, 53, 4, 61, 4, 46, 0, 32, 0, 18, 4, 62, 4, 57, 4, 66, 4, 56, 4, 0, 0, 40, 6, 51, 6, 69, 6, 32, 0, 39, 6, 68, 6, 68, 6, 71, 6, 32, 0, 39, 6, 68, 6, 49, 6, 45, 6, 69, 6, 70, 6, 0, 0, 118, 0, 101, 0, 117, 0, 105, 0, 108, 0, 108, 0, 101, 0, 122, 0, 32, 0, 108, 0, 101, 0, 32, 0, 118, 0, 233, 0, 114, 0, 105, 0, 102, 0, 105, 0, 101, 0, 114, 0, 32, 0, 224, 0, 32, 0, 110, 0, 111, 0, 117, 0, 118, 0, 101, 0, 97, 0, 117, 0, 0, 0],
     logins: [
+      // This login is present in the data, but should be stripped out
+      // by the validation rules of the importer:
+      // {
+      //   "username": "a",
+      //   "password": "",
+      //   "hostname": "http://www.reddit.com",
+      //   "formSubmitURL": "",
+      //   "httpRealm": null,
+      //   "usernameField": "",
+      //   "passwordField": ""
+      // },
       {
         username: "購読を",
         password: "Bitte bestätigen",
         hostname: "http://www.reddit.com",
         formSubmitURL: "",
         httpRealm: null,
         usernameField: "",
         passwordField: "",
@@ -315,17 +326,17 @@ add_task(async function test_passwordsNo
   let logins = Services.logins.getAllLogins({});
   Assert.equal(logins.length, 0, "There are no logins at the beginning of the test");
 
   let uris = []; // the uris of the migrated logins
   for (let url of TESTED_URLS) {
     uris.push(makeURI(url));
      // in this test, there is no IE login data in the registry, so after the migration, the number
      // of logins in the store should be 0
-    migrator._migrateURIs(uris);
+    await migrator._migrateURIs(uris);
     logins = Services.logins.getAllLogins({});
     Assert.equal(logins.length, 0,
                  "There are no logins after doing the migration without adding values to the registry");
   }
 });
 
 add_task(async function test_passwordsAvailable() {
   if (AppConstants.isPlatformAndVersionAtLeast("win", "6.2")) {
@@ -366,17 +377,17 @@ add_task(async function test_passwordsAv
     // new value with the encrypted data
     backupAndStore(Storage2Key, website.hash,
                    crypto.encryptData(crypto.arrayToString(website.data),
                                       website.uri.spec, true));
     Assert.ok(migrator.exists, "The migrator has to exist");
     uris.push(website.uri);
     hashes.push(website.hash);
 
-    migrator._migrateURIs(uris);
+    await migrator._migrateURIs(uris);
     logins = Services.logins.getAllLogins({});
     // check that the number of logins in the password manager has increased as expected which means
     // that all the values for the current website were imported
     loginCount += website.logins.length;
     Assert.equal(logins.length, loginCount,
                  "The number of logins has increased after the migration");
     // NB: because telemetry records any login data passed to the login manager, it
     // also gets told about logins that are duplicates or invalid (for one reason
--- a/browser/components/migration/tests/unit/test_automigration.js
+++ b/browser/components/migration/tests/unit/test_automigration.js
@@ -163,26 +163,26 @@ add_task(async function checkUndoPrecond
     getSourceProfiles() {
       info("Read sourceProfiles");
       return null;
     },
     getMigrateData(profileToMigrate) {
       this._getMigrateDataArgs = profileToMigrate;
       return Ci.nsIBrowserProfileMigrator.BOOKMARKS;
     },
-    migrate(types, startup, profileToMigrate) {
+    async migrate(types, startup, profileToMigrate) {
       this._migrateArgs = [types, startup, profileToMigrate];
       if (shouldAddData) {
         // Insert a login and check that that worked.
-        MigrationUtils.insertLoginWrapper({
+        await MigrationUtils.insertLoginsWrapper([{
           hostname: "www.mozilla.org",
           formSubmitURL: "http://www.mozilla.org",
           username: "user",
           password: "pass",
-        });
+        }]);
       }
       TestUtils.executeSoon(function() {
         Services.obs.notifyObservers(null, "Migration:Ended", undefined);
       });
     },
   };
 
   gShimmedMigratorKeyPicker = function() {
@@ -234,22 +234,22 @@ add_task(async function checkUndoPrecond
 
 /**
  * Fake a migration and then try to undo it to verify all data gets removed.
  */
 add_task(async function checkUndoRemoval() {
   MigrationUtils.initializeUndoData();
   Preferences.set("browser.migrate.automigrate.browser", "automationbrowser");
   // Insert a login and check that that worked.
-  MigrationUtils.insertLoginWrapper({
+  await MigrationUtils.insertLoginsWrapper([{
     hostname: "www.mozilla.org",
     formSubmitURL: "http://www.mozilla.org",
     username: "user",
     password: "pass",
-  });
+  }]);
   let storedLogins = Services.logins.findLogins({}, "www.mozilla.org",
                                                 "http://www.mozilla.org", null);
   Assert.equal(storedLogins.length, 1, "Should have 1 login");
 
   // Insert a bookmark and check that we have exactly 1 bookmark for that URI.
   await MigrationUtils.insertBookmarkWrapper({
     parentGuid: PlacesUtils.bookmarks.toolbarGuid,
     url: "http://www.example.org/",
@@ -436,56 +436,56 @@ add_task(async function testBookmarkRemo
     let dbgStr = dbResultForGuid && dbResultForGuid.title;
     Assert.equal(null, dbResultForGuid, "Should not be able to find items that should have been removed, but found " + dbgStr);
   }
   await PlacesUtils.bookmarks.eraseEverything();
 });
 
 add_task(async function checkUndoLoginsState() {
   MigrationUtils.initializeUndoData();
-  MigrationUtils.insertLoginWrapper({
+  await MigrationUtils.insertLoginsWrapper([{
     username: "foo",
     password: "bar",
     hostname: "https://example.com",
     formSubmitURL: "https://example.com/",
     timeCreated: new Date(),
-  });
+  }]);
   let storedLogins = Services.logins.findLogins({}, "https://example.com", "", "");
   let storedLogin = storedLogins[0];
   storedLogin.QueryInterface(Ci.nsILoginMetaInfo);
   let {guid, timePasswordChanged} = storedLogin;
   let undoLoginData = (await MigrationUtils.stopAndRetrieveUndoData()).get("logins");
   Assert.deepEqual([{guid, timePasswordChanged}], undoLoginData);
   Services.logins.removeAllLogins();
 });
 
 add_task(async function testLoginsRemovalByUndo() {
   MigrationUtils.initializeUndoData();
-  MigrationUtils.insertLoginWrapper({
+  await MigrationUtils.insertLoginsWrapper([{
     username: "foo",
     password: "bar",
     hostname: "https://example.com",
     formSubmitURL: "https://example.com/",
     timeCreated: new Date(),
-  });
-  MigrationUtils.insertLoginWrapper({
+  }]);
+  await MigrationUtils.insertLoginsWrapper([{
     username: "foo",
     password: "bar",
     hostname: "https://example.org",
     formSubmitURL: "https://example.org/",
     timeCreated: new Date(new Date().getTime() - 10000),
-  });
+  }]);
   // This should update the existing login
-  LoginHelper.maybeImportLogin({
+  await LoginHelper.maybeImportLogins([{
     username: "foo",
     password: "bazzy",
     hostname: "https://example.org",
     formSubmitURL: "https://example.org/",
     timePasswordChanged: new Date(),
-  });
+  }]);
   Assert.equal(1, LoginHelper.searchLoginsWithObject({hostname: "https://example.org", formSubmitURL: "https://example.org/"}).length,
                "Should be only 1 login for example.org (that was updated)");
   let undoLoginData = (await MigrationUtils.stopAndRetrieveUndoData()).get("logins");
 
   await AutoMigrate._removeUnchangedLogins(undoLoginData);
   Assert.equal(0, LoginHelper.searchLoginsWithObject({hostname: "https://example.com", formSubmitURL: "https://example.com/"}).length,
                    "unchanged example.com entry should have been removed.");
   Assert.equal(1, LoginHelper.searchLoginsWithObject({hostname: "https://example.org", formSubmitURL: "https://example.org/"}).length,
--- a/toolkit/components/passwordmgr/LoginHelper.jsm
+++ b/toolkit/components/passwordmgr/LoginHelper.jsm
@@ -203,61 +203,16 @@ 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;
 
@@ -586,47 +541,19 @@ this.LoginHelper = {
         fieldType == "tel" ||
         fieldType == "number") {
       return true;
     }
     return false;
   },
 
   /**
-   * Add the login to the password manager if a similar one doesn't already exist. Merge it
-   * otherwise with the similar existing ones.
-   * @param {Object} loginData - the data about the login that needs to be added.
-   * @returns {nsILoginInfo} the newly added login, or null if no login was added.
-   *                          Note that we will also return null if an existing login
-   *                          was modified.
-   */
-  maybeImportLogin(loginData) {
-    // 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;
-    if (this.checkForDuplicatesAndMaybeUpdate(login)) {
-      return null;
-    }
-    return Services.logins.addLogin(login);
-  },
-
-  /**
-   * Equivalent to maybeImportLogin, except asynchronous and for multiple logins.
+   * For each login, add the login to the password manager if a similar one
+   * doesn't already exist. Merge it otherwise with the similar existing ones.
+   *
    * @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
@@ -650,19 +577,18 @@ this.LoginHelper = {
         // 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.
+      // see if this is a duplicate. This should mirror the logic below for
+      // existingLogins, 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;
@@ -680,23 +606,56 @@ this.LoginHelper = {
           }
         }
 
         if (foundMatchingNewLogin) {
           continue;
         }
       }
 
-      if (this.checkForDuplicatesAndMaybeUpdate(login)) {
+      // 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 */))) {
+        continue;
+      }
+      // 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) {
         continue;
       }
 
       newLogins.push(login);
       loginsToAdd.push(login);
     }
+    if (!loginsToAdd.length) {
+      return [];
+    }
     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.
--- a/toolkit/components/passwordmgr/nsLoginManager.js
+++ b/toolkit/components/passwordmgr/nsLoginManager.js
@@ -238,36 +238,23 @@ LoginManager.prototype = {
 
     let passwordsCountHistogram = clearAndGetHistogram("PWMGR_NUM_PASSWORDS_PER_HOSTNAME");
     for (let count of hostnameCount.values()) {
       passwordsCountHistogram.add(count);
     }
   },
 
 
-
-
-
-  /* ---------- Primary Public interfaces ---------- */
-
-
-
-
   /**
-   * @type Promise
-   * This promise is resolved when initialization is complete, and is rejected
-   * in case the asynchronous part of initialization failed.
+   * Ensures that a login isn't missing any necessary fields.
+   *
+   * @param login
+   *        The login to check.
    */
-  initializationPromise: null,
-
-
-  /**
-   * Add a new login to login storage.
-   */
-  addLogin(login) {
+  _checkLogin(login) {
     // Sanity check the login
     if (login.hostname == null || login.hostname.length == 0) {
       throw new Error("Can't add a login with a null or empty hostname.");
     }
 
     // For logins w/o a username, set to "", not null.
     if (login.username == null) {
       throw new Error("Can't add a login with a null username.");
@@ -286,18 +273,40 @@ LoginManager.prototype = {
       // We have a HTTP realm. Can't have a form submit URL.
       if (login.formSubmitURL != null) {
         throw new Error("Can't add a login with both a httpRealm and formSubmitURL.");
       }
     } else {
       // Need one or the other!
       throw new Error("Can't add a login without a httpRealm or formSubmitURL.");
     }
+  },
 
 
+
+
+  /* ---------- Primary Public interfaces ---------- */
+
+
+
+
+  /**
+   * @type Promise
+   * This promise is resolved when initialization is complete, and is rejected
+   * in case the asynchronous part of initialization failed.
+   */
+  initializationPromise: null,
+
+
+  /**
+   * Add a new login to login storage.
+   */
+  addLogin(login) {
+    this._checkLogin(login);
+
     // Look for an existing entry.
     var logins = this.findLogins({}, login.hostname, login.formSubmitURL,
                                  login.httpRealm);
 
     if (logins.some(l => login.matches(l, true))) {
       throw new Error("This login already exists.");
     }
 
@@ -307,27 +316,40 @@ LoginManager.prototype = {
 
   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);
+    let resultLogins = [];
     for (let i = 0; i < logins.length; i++) {
+      try {
+        this._checkLogin(logins[i]);
+      } catch (e) {
+        Cu.reportError(e);
+        continue;
+      }
+
       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);
+      let resultLogin = 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;
+
+      resultLogin.username = plaintextUsername;
+      resultLogin.password = plaintextPassword;
+      resultLogins.push(resultLogin);
     }
+    return resultLogins;
   },
 
   /**
    * Remove the specified login from the stored logins.
    */
   removeLogin(login) {
     log.debug("Removing login");
     return this._storage.removeLogin(login);
--- a/toolkit/components/passwordmgr/test/unit/test_maybeImportLogin.js
+++ b/toolkit/components/passwordmgr/test/unit/test_maybeImportLogin.js
@@ -8,162 +8,162 @@ const HOST2 = "https://www.mozilla.org/"
 
 const USER1 = "myuser";
 const USER2 = "anotheruser";
 
 const PASS1 = "mypass";
 const PASS2 = "anotherpass";
 const PASS3 = "yetanotherpass";
 
-add_task(function test_new_logins() {
-  let importedLogin = LoginHelper.maybeImportLogin({
+add_task(async function test_new_logins() {
+  let [importedLogin] = await LoginHelper.maybeImportLogins([{
     username: USER1,
     password: PASS1,
     hostname: HOST1,
     formSubmitURL: HOST1,
-  });
+  }]);
   Assert.ok(importedLogin, "Return value should indicate imported login.");
   let matchingLogins = LoginHelper.searchLoginsWithObject({hostname: HOST1});
   Assert.equal(matchingLogins.length, 1, `There should be 1 login for ${HOST1}`);
 
-  importedLogin = LoginHelper.maybeImportLogin({
+  [importedLogin] = await LoginHelper.maybeImportLogins([{
     username: USER1,
     password: PASS1,
     hostname: HOST2,
     formSubmitURL: HOST2,
-  });
+  }]);
 
   Assert.ok(importedLogin, "Return value should indicate another imported login.");
   matchingLogins = LoginHelper.searchLoginsWithObject({hostname: HOST1});
   Assert.equal(matchingLogins.length, 1, `There should still be 1 login for ${HOST1}`);
 
   matchingLogins = LoginHelper.searchLoginsWithObject({hostname: HOST2});
   Assert.equal(matchingLogins.length, 1, `There should also be 1 login for ${HOST2}`);
   Assert.equal(Services.logins.getAllLogins().length, 2, "There should be 2 logins in total");
   Services.logins.removeAllLogins();
 });
 
-add_task(function test_duplicate_logins() {
-  let importedLogin = LoginHelper.maybeImportLogin({
+add_task(async function test_duplicate_logins() {
+  let [importedLogin] = await LoginHelper.maybeImportLogins([{
     username: USER1,
     password: PASS1,
     hostname: HOST1,
     formSubmitURL: HOST1,
-  });
+  }]);
   Assert.ok(importedLogin, "Return value should indicate imported login.");
   let matchingLogins = LoginHelper.searchLoginsWithObject({hostname: HOST1});
   Assert.equal(matchingLogins.length, 1, `There should be 1 login for ${HOST1}`);
 
-  importedLogin = LoginHelper.maybeImportLogin({
+  [importedLogin] = await LoginHelper.maybeImportLogins([{
     username: USER1,
     password: PASS1,
     hostname: HOST1,
     formSubmitURL: HOST1,
-  });
+  }]);
   Assert.ok(!importedLogin, "Return value should indicate no new login was imported.");
   matchingLogins = LoginHelper.searchLoginsWithObject({hostname: HOST1});
   Assert.equal(matchingLogins.length, 1, `There should still be 1 login for ${HOST1}`);
   Services.logins.removeAllLogins();
 });
 
-add_task(function test_different_passwords() {
-  let importedLogin = LoginHelper.maybeImportLogin({
+add_task(async function test_different_passwords() {
+  let [importedLogin] = await LoginHelper.maybeImportLogins([{
     username: USER1,
     password: PASS1,
     hostname: HOST1,
     formSubmitURL: HOST1,
     timeCreated: new Date(Date.now() - 1000),
-  });
+  }]);
   Assert.ok(importedLogin, "Return value should indicate imported login.");
   let matchingLogins = LoginHelper.searchLoginsWithObject({hostname: HOST1});
   Assert.equal(matchingLogins.length, 1, `There should be 1 login for ${HOST1}`);
 
   // This item will be newer, so its password should take precedence.
-  importedLogin = LoginHelper.maybeImportLogin({
+  [importedLogin] = await LoginHelper.maybeImportLogins([{
     username: USER1,
     password: PASS2,
     hostname: HOST1,
     formSubmitURL: HOST1,
     timeCreated: new Date(),
-  });
+  }]);
   Assert.ok(!importedLogin, "Return value should not indicate imported login (as we updated an existing one).");
   matchingLogins = LoginHelper.searchLoginsWithObject({hostname: HOST1});
   Assert.equal(matchingLogins.length, 1, `There should still be 1 login for ${HOST1}`);
   Assert.equal(matchingLogins[0].password, PASS2, "We should have updated the password for this login.");
 
   // Now try to update with an older password:
-  importedLogin = LoginHelper.maybeImportLogin({
+  [importedLogin] = await LoginHelper.maybeImportLogins([{
     username: USER1,
     password: PASS3,
     hostname: HOST1,
     formSubmitURL: HOST1,
     timeCreated: new Date(Date.now() - 1000000),
-  });
+  }]);
   Assert.ok(!importedLogin, "Return value should not indicate imported login (as we didn't update anything).");
   matchingLogins = LoginHelper.searchLoginsWithObject({hostname: HOST1});
   Assert.equal(matchingLogins.length, 1, `There should still be 1 login for ${HOST1}`);
   Assert.equal(matchingLogins[0].password, PASS2, "We should NOT have updated the password for this login.");
 
   Services.logins.removeAllLogins();
 });
 
-add_task(function test_different_usernames() {
-  let importedLogin = LoginHelper.maybeImportLogin({
+add_task(async function test_different_usernames() {
+  let [importedLogin] = await LoginHelper.maybeImportLogins([{
     username: USER1,
     password: PASS1,
     hostname: HOST1,
     formSubmitURL: HOST1,
-  });
+  }]);
   Assert.ok(importedLogin, "Return value should indicate imported login.");
   let matchingLogins = LoginHelper.searchLoginsWithObject({hostname: HOST1});
   Assert.equal(matchingLogins.length, 1, `There should be 1 login for ${HOST1}`);
 
-  importedLogin = LoginHelper.maybeImportLogin({
+  [importedLogin] = await LoginHelper.maybeImportLogins([{
     username: USER2,
     password: PASS1,
     hostname: HOST1,
     formSubmitURL: HOST1,
-  });
+  }]);
   Assert.ok(importedLogin, "Return value should indicate another imported login.");
   matchingLogins = LoginHelper.searchLoginsWithObject({hostname: HOST1});
   Assert.equal(matchingLogins.length, 2, `There should now be 2 logins for ${HOST1}`);
 
   Services.logins.removeAllLogins();
 });
 
-add_task(function test_different_targets() {
-  let importedLogin = LoginHelper.maybeImportLogin({
+add_task(async function test_different_targets() {
+  let [importedLogin] = await LoginHelper.maybeImportLogins([{
     username: USER1,
     password: PASS1,
     hostname: HOST1,
     formSubmitURL: HOST1,
-  });
+  }]);
   Assert.ok(importedLogin, "Return value should indicate imported login.");
   let matchingLogins = LoginHelper.searchLoginsWithObject({hostname: HOST1});
   Assert.equal(matchingLogins.length, 1, `There should be 1 login for ${HOST1}`);
 
   // Not passing either a formSubmitURL or a httpRealm should be treated as
   // the same as the previous login
-  importedLogin = LoginHelper.maybeImportLogin({
+  [importedLogin] = await LoginHelper.maybeImportLogins([{
     username: USER1,
     password: PASS1,
     hostname: HOST1,
-  });
+  }]);
   Assert.ok(!importedLogin, "Return value should NOT indicate imported login " +
     "(because a missing formSubmitURL and httpRealm should be duped to the existing login).");
   matchingLogins = LoginHelper.searchLoginsWithObject({hostname: HOST1});
   Assert.equal(matchingLogins.length, 1, `There should still be 1 login for ${HOST1}`);
   Assert.equal(matchingLogins[0].formSubmitURL, HOST1, "The form submission URL should have been kept.");
 
-  importedLogin = LoginHelper.maybeImportLogin({
+  [importedLogin] = await LoginHelper.maybeImportLogins([{
     username: USER1,
     password: PASS1,
     hostname: HOST1,
     httpRealm: HOST1,
-  });
+  }]);
   Assert.ok(importedLogin, "Return value should indicate another imported login " +
     "as an httpRealm login shouldn't be duped.");
   matchingLogins = LoginHelper.searchLoginsWithObject({hostname: HOST1});
   Assert.equal(matchingLogins.length, 2, `There should now be 2 logins for ${HOST1}`);
 
   Services.logins.removeAllLogins();
 });