Bug 1551657 part 4. Stop using [array] in searchLogins. r=MattN
authorBoris Zbarsky <bzbarsky@mit.edu>
Tue, 14 May 2019 19:32:17 +0000
changeset 532761 bdbfb93093eeba973bb1e1f3fc2c2697ed4e64c4
parent 532760 288bafa86adbbcbdf71e015ac2bc8b15e127f8f8
child 532762 bbbbed508f9be54d655124bc1662d6b2cb47cbef
push id11272
push userapavel@mozilla.com
push dateThu, 16 May 2019 15:28:22 +0000
treeherdermozilla-beta@2265bfc5920d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersMattN
bugs1551657
milestone68.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1551657 part 4. Stop using [array] in searchLogins. r=MattN Audited https://searchfox.org/mozilla-central/search?q=%5B%5EA-Za-z_%5D%5BsS%5DearchLogins%5B%5EA-Za-z_%5D&case=true&regexp=true&path= Differential Revision: https://phabricator.services.mozilla.com/D31120
services/sync/modules/engines/passwords.js
toolkit/components/passwordmgr/LoginHelper.jsm
toolkit/components/passwordmgr/LoginManager.jsm
toolkit/components/passwordmgr/nsILoginManager.idl
toolkit/components/passwordmgr/nsILoginManagerStorage.idl
toolkit/components/passwordmgr/storage-json.js
toolkit/components/passwordmgr/storage-mozStorage.js
toolkit/components/passwordmgr/test/unit/test_legacy_empty_formSubmitURL.js
toolkit/components/passwordmgr/test/unit/test_logins_decrypt_failure.js
toolkit/components/passwordmgr/test/unit/test_logins_metainfo.js
toolkit/components/passwordmgr/test/unit/test_logins_search.js
toolkit/components/passwordmgr/test/unit/test_search_schemeUpgrades.js
--- a/services/sync/modules/engines/passwords.js
+++ b/services/sync/modules/engines/passwords.js
@@ -194,17 +194,17 @@ PasswordStore.prototype = {
 
     return info;
   },
 
   async _getLoginFromGUID(id) {
     let prop = this._newPropertyBag();
     prop.setPropertyAsAUTF8String("guid", id);
 
-    let logins = Services.logins.searchLogins({}, prop);
+    let logins = Services.logins.searchLogins(prop);
     await Async.promiseYield(); // Yield back to main thread after synchronous operation.
 
     if (logins.length > 0) {
       this._log.trace(logins.length + " items matching " + id + " found.");
       return logins[0];
     }
 
     this._log.trace("No items matching " + id + " found. Ignoring");
--- a/toolkit/components/passwordmgr/LoginHelper.jsm
+++ b/toolkit/components/passwordmgr/LoginHelper.jsm
@@ -167,24 +167,24 @@ var LoginHelper = {
       }
     }
     return propertyBag.QueryInterface(Ci.nsIPropertyBag)
                       .QueryInterface(Ci.nsIPropertyBag2)
                       .QueryInterface(Ci.nsIWritablePropertyBag2);
   },
 
   /**
-   * Helper to avoid the `count` argument and property bags when calling
+   * Helper to avoid the property bags when calling
    * Services.logins.searchLogins from JS.
    *
    * @param {Object} aSearchOptions - A regular JS object to copy to a property bag before searching
    * @return {nsILoginInfo[]} - The result of calling searchLogins.
    */
   searchLoginsWithObject(aSearchOptions) {
-    return Services.logins.searchLogins({}, this.newPropertyBag(aSearchOptions));
+    return Services.logins.searchLogins(this.newPropertyBag(aSearchOptions));
   },
 
   /**
    * @param {string} aURL
    * @returns {string} which is the hostPort of aURL if supported by the scheme
    *                   otherwise, returns the original aURL.
    */
   maybeGetHostPortForURL(aURL) {
--- a/toolkit/components/passwordmgr/LoginManager.jsm
+++ b/toolkit/components/passwordmgr/LoginManager.jsm
@@ -374,31 +374,31 @@ LoginManager.prototype = {
 
 
   /**
    * Public wrapper around _searchLogins to convert the nsIPropertyBag to a
    * JavaScript object and decrypt the results.
    *
    * @return {nsILoginInfo[]} which are decrypted.
    */
-  searchLogins(count, matchData) {
+  searchLogins(matchData) {
     log.debug("Searching for logins");
 
     matchData.QueryInterface(Ci.nsIPropertyBag2);
     if (!matchData.hasKey("guid")) {
       if (!matchData.hasKey("hostname")) {
         log.warn("searchLogins: A `hostname` is recommended");
       }
 
       if (!matchData.hasKey("formSubmitURL") && !matchData.hasKey("httpRealm")) {
         log.warn("searchLogins: `formSubmitURL` or `httpRealm` is recommended");
       }
     }
 
-    return this._storage.searchLogins(count, matchData);
+    return this._storage.searchLogins(matchData);
   },
 
 
   /**
    * Search for the known logins for entries matching the specified criteria,
    * returns only the count.
    */
   countLogins(origin, formActionOrigin, httpRealm) {
--- a/toolkit/components/passwordmgr/nsILoginManager.idl
+++ b/toolkit/components/passwordmgr/nsILoginManager.idl
@@ -194,33 +194,27 @@ interface nsILoginManager : nsISupports 
    */
   unsigned long countLogins(in AString aHostname, in AString aActionURL,
                             in AString aHttpRealm);
 
   /**
    * Search for logins in the login manager. An array is always returned;
    * if there are no logins the array is empty.
    *
-   * @param count
-   *        The number of elements in the array. JS callers can simply use
-   *        the array's .length property, and supply an dummy object for
-   *        this out param. For example: |searchLogins({}, matchData)|
    * @param matchData
    *        The data used to search. This does not follow the same
    *        requirements as findLogins for those fields. Wildcard matches are
    *        simply not specified.
-   * @param logins
-   *        An array of nsILoginInfo objects.
+   * @return An array of nsILoginInfo objects.
    *
    * NOTE: This can be called from JS as:
-   *       var logins = pwmgr.searchLogins({}, matchData);
+   *       var logins = pwmgr.searchLogins(matchData);
    *       (|logins| is an array).
    */
-  void searchLogins(out unsigned long count, in nsIPropertyBag matchData,
-                    [retval, array, size_is(count)] out nsILoginInfo logins);
+  Array<nsILoginInfo> searchLogins(in nsIPropertyBag matchData);
 
  /**
   * True when a master password prompt is being displayed.
   */
   readonly attribute boolean uiBusy;
 
  /**
   * True when the master password has already been entered, and so a caller
--- a/toolkit/components/passwordmgr/nsILoginManagerStorage.idl
+++ b/toolkit/components/passwordmgr/nsILoginManagerStorage.idl
@@ -117,33 +117,27 @@ interface nsILoginManagerStorage : nsISu
    */
   Array<nsILoginInfo> getAllLogins();
 
 
   /**
    * Search for logins in the login manager. An array is always returned;
    * if there are no logins the array is empty.
    *
-   * @param count
-   *        The number of elements in the array. JS callers can simply use
-   *        the array's .length property, and supply an dummy object for
-   *        this out param. For example: |searchLogins({}, matchData)|
    * @param matchData
    *        The data used to search. This does not follow the same
    *        requirements as findLogins for those fields. Wildcard matches are
    *        simply not specified.
-   * @param logins
-   *        An array of nsILoginInfo objects.
+   * @return An array of nsILoginInfo objects.
    *
    * NOTE: This can be called from JS as:
-   *       var logins = pwmgr.searchLogins({}, matchData);
+   *       var logins = pwmgr.searchLogins(matchData);
    *       (|logins| is an array).
    */
-  void searchLogins(out unsigned long count, in nsIPropertyBag matchData,
-                    [retval, array, size_is(count)] out nsILoginInfo logins);
+  Array<nsILoginInfo> searchLogins(in nsIPropertyBag matchData);
 
 
   /**
    * Search for logins matching the specified criteria. Called when looking
    * for logins that might be applicable to a form or authentication request.
    *
    * @param aHostname
    *        The hostname to restrict searches to, in URL format. For
--- a/toolkit/components/passwordmgr/storage-json.js
+++ b/toolkit/components/passwordmgr/storage-json.js
@@ -259,17 +259,17 @@ this.LoginManagerStorage_json.prototype 
   },
 
   /**
    * Public wrapper around _searchLogins to convert the nsIPropertyBag to a
    * JavaScript object and decrypt the results.
    *
    * @return {nsILoginInfo[]} which are decrypted.
    */
-  searchLogins(count, matchData) {
+  searchLogins(matchData) {
     let realMatchData = {};
     let options = {};
     // Convert nsIPropertyBag to normal JS object
     for (let prop of matchData.enumerator) {
       switch (prop.name) {
         // Some property names aren't field names but are special options to affect the search.
         case "schemeUpgrades": {
           options[prop.name] = prop.value;
@@ -282,17 +282,16 @@ this.LoginManagerStorage_json.prototype 
       }
     }
 
     let [logins, ids] = this._searchLogins(realMatchData, options);
 
     // Decrypt entries found for the caller.
     logins = this._decryptLogins(logins);
 
-    count.value = logins.length; // needed for XPCOM
     return logins;
   },
 
   /**
    * Private method to perform arbitrary searches on any field. Decryption is
    * left to the caller.
    *
    * Returns [logins, ids] for logins that match the arguments, where logins
--- a/toolkit/components/passwordmgr/storage-mozStorage.js
+++ b/toolkit/components/passwordmgr/storage-mozStorage.js
@@ -408,17 +408,17 @@ LoginManagerStorage_mozStorage.prototype
 
 
   /**
    * Public wrapper around _searchLogins to convert the nsIPropertyBag to a
    * JavaScript object and decrypt the results.
    *
    * @return {nsILoginInfo[]} which are decrypted.
    */
-  searchLogins(count, matchData) {
+  searchLogins(matchData) {
     let realMatchData = {};
     let options = {};
     // Convert nsIPropertyBag to normal JS object
     for (let prop of matchData.enumerator) {
       switch (prop.name) {
         // Some property names aren't field names but are special options to affect the search.
         case "schemeUpgrades": {
           options[prop.name] = prop.value;
@@ -431,17 +431,16 @@ LoginManagerStorage_mozStorage.prototype
       }
     }
 
     let [logins, ids] = this._searchLogins(realMatchData, options);
 
     // Decrypt entries found for the caller.
     logins = this._decryptLogins(logins);
 
-    count.value = logins.length; // needed for XPCOM
     return logins;
   },
 
 
   /**
    * Private method to perform arbitrary searches on any field. Decryption is
    * left to the caller.
    *
--- a/toolkit/components/passwordmgr/test/unit/test_legacy_empty_formSubmitURL.js
+++ b/toolkit/components/passwordmgr/test/unit/test_legacy_empty_formSubmitURL.js
@@ -64,41 +64,40 @@ add_task(function test_addLogin_wildcard
 /**
  * Verifies that findLogins, searchLogins, and countLogins include all logins
  * that have an empty formSubmitURL in the store, even when a formSubmitURL is
  * specified.
  */
 add_task(function test_search_all_wildcard() {
   // Search a given formSubmitURL on any host.
   let matchData = newPropertyBag({ formSubmitURL: "http://www.example.com" });
-  Assert.equal(Services.logins.searchLogins({}, matchData).length, 2);
+  Assert.equal(Services.logins.searchLogins(matchData).length, 2);
 
   Assert.equal(Services.logins.findLogins("", "http://www.example.com",
                                           null).length, 2);
 
   Assert.equal(Services.logins.countLogins("", "http://www.example.com",
                                            null), 2);
 
   // Restrict the search to one host.
   matchData.setProperty("hostname", "http://any.example.com");
-  Assert.equal(Services.logins.searchLogins({}, matchData).length, 1);
+  Assert.equal(Services.logins.searchLogins(matchData).length, 1);
 
   Assert.equal(Services.logins.findLogins("http://any.example.com",
                                           "http://www.example.com",
                                           null).length, 1);
 
   Assert.equal(Services.logins.countLogins("http://any.example.com",
                                            "http://www.example.com",
                                            null), 1);
 });
 
 /**
  * Verifies that specifying an empty string for formSubmitURL in searchLogins
  * includes only logins that have an empty formSubmitURL in the store.
  */
 add_task(function test_searchLogins_wildcard() {
-  let logins = Services.logins.searchLogins({},
-                                            newPropertyBag({ formSubmitURL: "" }));
+  let logins = Services.logins.searchLogins(newPropertyBag({ formSubmitURL: "" }));
 
   let loginInfo = TestData.formLogin({ hostname: "http://any.example.com",
                                        formSubmitURL: "" });
   LoginTestUtils.assertLoginListsEqual(logins, [loginInfo]);
 });
--- a/toolkit/components/passwordmgr/test/unit/test_logins_decrypt_failure.js
+++ b/toolkit/components/passwordmgr/test/unit/test_logins_decrypt_failure.js
@@ -34,17 +34,17 @@ add_task(function test_logins_decrypt_fa
   }
 
   // This makes the existing logins non-decryptable.
   resetMasterPassword();
 
   // These functions don't see the non-decryptable entries anymore.
   Assert.equal(Services.logins.getAllLogins().length, 0);
   Assert.equal(Services.logins.findLogins("", "", "").length, 0);
-  Assert.equal(Services.logins.searchLogins({}, newPropertyBag()).length, 0);
+  Assert.equal(Services.logins.searchLogins(newPropertyBag()).length, 0);
   Assert.throws(() => Services.logins.modifyLogin(logins[0], newPropertyBag()),
                 /No matching logins/);
   Assert.throws(() => Services.logins.removeLogin(logins[0]),
                 /No matching logins/);
 
   // The function that counts logins sees the non-decryptable entries also.
   Assert.equal(Services.logins.countLogins("", "", ""), logins.length);
 
@@ -54,17 +54,17 @@ add_task(function test_logins_decrypt_fa
   }
   LoginTestUtils.checkLogins(logins);
   Assert.equal(Services.logins.countLogins("", "", ""), logins.length * 2);
 
   // Finding logins doesn't return the non-decryptable duplicates.
   Assert.equal(Services.logins.findLogins("http://www.example.com",
                                           "", "").length, 1);
   let matchData = newPropertyBag({ hostname: "http://www.example.com" });
-  Assert.equal(Services.logins.searchLogins({}, matchData).length, 1);
+  Assert.equal(Services.logins.searchLogins(matchData).length, 1);
 
   // Removing single logins does not remove non-decryptable logins.
   for (let loginInfo of TestData.loginList()) {
     Services.logins.removeLogin(loginInfo);
   }
   Assert.equal(Services.logins.getAllLogins().length, 0);
   Assert.equal(Services.logins.countLogins("", "", ""), logins.length);
 
@@ -95,29 +95,29 @@ add_task(function test_add_logins_with_d
 
   Services.logins.addLogin(login);
 
   // We can search for this login by GUID.
   let searchProp = Cc["@mozilla.org/hash-property-bag;1"]
                    .createInstance(Ci.nsIWritablePropertyBag2);
   searchProp.setPropertyAsAUTF8String("guid", login.guid);
 
-  equal(Services.logins.searchLogins({}, searchProp).length, 1);
+  equal(Services.logins.searchLogins(searchProp).length, 1);
 
   // We should fail to re-add it as it remains good.
   Assert.throws(() => Services.logins.addLogin(login),
                 /This login already exists./);
   // We should fail to re-add a different login with the same GUID.
   Assert.throws(() => Services.logins.addLogin(loginDupeGuid),
                 /specified GUID already exists/);
 
   // This makes the existing login non-decryptable.
   resetMasterPassword();
 
   // We can no longer find it in our search.
-  equal(Services.logins.searchLogins({}, searchProp).length, 0);
+  equal(Services.logins.searchLogins(searchProp).length, 0);
 
   // So we should be able to re-add a login with that same GUID.
   Services.logins.addLogin(login);
-  equal(Services.logins.searchLogins({}, searchProp).length, 1);
+  equal(Services.logins.searchLogins(searchProp).length, 1);
 
   Services.logins.removeAllLogins();
 });
--- a/toolkit/components/passwordmgr/test/unit/test_logins_metainfo.js
+++ b/toolkit/components/passwordmgr/test/unit/test_logins_metainfo.js
@@ -230,33 +230,33 @@ add_task(function test_modifyLogin_nsIPr
   LoginTestUtils.checkLogins([gLoginInfo1, gLoginInfo2, gLoginInfo3]);
 });
 
 /**
  * Tests searching logins using nsILoginMetaInfo properties.
  */
 add_task(function test_searchLogins_metainfo() {
   // Find by GUID.
-  let logins = Services.logins.searchLogins({}, newPropertyBag({
+  let logins = Services.logins.searchLogins(newPropertyBag({
     guid: gLoginMetaInfo1.guid,
   }));
   Assert.equal(logins.length, 1);
   let foundLogin = logins[0].QueryInterface(Ci.nsILoginMetaInfo);
   assertMetaInfoEqual(foundLogin, gLoginMetaInfo1);
 
   // Find by timestamp.
-  logins = Services.logins.searchLogins({}, newPropertyBag({
+  logins = Services.logins.searchLogins(newPropertyBag({
     timePasswordChanged: gLoginMetaInfo2.timePasswordChanged,
   }));
   Assert.equal(logins.length, 1);
   foundLogin = logins[0].QueryInterface(Ci.nsILoginMetaInfo);
   assertMetaInfoEqual(foundLogin, gLoginMetaInfo2);
 
   // Find using two properties at the same time.
-  logins = Services.logins.searchLogins({}, newPropertyBag({
+  logins = Services.logins.searchLogins(newPropertyBag({
     guid: gLoginMetaInfo3.guid,
     timePasswordChanged: gLoginMetaInfo3.timePasswordChanged,
   }));
   Assert.equal(logins.length, 1);
   foundLogin = logins[0].QueryInterface(Ci.nsILoginMetaInfo);
   assertMetaInfoEqual(foundLogin, gLoginMetaInfo3);
 });
 
--- a/toolkit/components/passwordmgr/test/unit/test_logins_search.js
+++ b/toolkit/components/passwordmgr/test/unit/test_logins_search.js
@@ -36,19 +36,17 @@ function buildExpectedLogins(aQuery) {
  *        don't make the current test meaningless.
  */
 function checkSearchLogins(aQuery, aExpectedCount) {
   info("Testing searchLogins for " + JSON.stringify(aQuery));
 
   let expectedLogins = buildExpectedLogins(aQuery);
   Assert.equal(expectedLogins.length, aExpectedCount);
 
-  let outCount = {};
-  let logins = Services.logins.searchLogins(outCount, newPropertyBag(aQuery));
-  Assert.equal(outCount.value, expectedLogins.length);
+  let logins = Services.logins.searchLogins(newPropertyBag(aQuery));
   LoginTestUtils.assertLoginListsEqual(logins, expectedLogins);
 }
 
 /**
  * Tests findLogins, searchLogins, and countLogins with the same query.
  *
  * @param aQuery
  *        The "hostname", "formSubmitURL", and "httpRealm" properties of this
@@ -164,18 +162,17 @@ add_task(function test_searchLogins() {
   checkSearchLogins({ hostname: "http://www6.example.com",
                       usernameField: "" }, 1);
 });
 
 /**
  * Tests searchLogins with invalid arguments.
  */
 add_task(function test_searchLogins_invalid() {
-  Assert.throws(() => Services.logins.searchLogins({},
-                                                   newPropertyBag({ username: "value" })),
+  Assert.throws(() => Services.logins.searchLogins(newPropertyBag({ username: "value" })),
                 /Unexpected field/);
 });
 
 /**
  * Tests that matches are case-sensitive, compare the full field value, and are
  * strict when interpreting the prePath of URIs.
  */
 add_task(function test_search_all_full_case_sensitive() {
--- a/toolkit/components/passwordmgr/test/unit/test_search_schemeUpgrades.js
+++ b/toolkit/components/passwordmgr/test/unit/test_search_schemeUpgrades.js
@@ -42,19 +42,17 @@ function buildExpectedLogins(aQuery) {
  *        don't make the current test meaningless.
  */
 function checkSearch(aQuery, aExpectedCount) {
   info("Testing searchLogins for " + JSON.stringify(aQuery));
 
   let expectedLogins = buildExpectedLogins(aQuery);
   Assert.equal(expectedLogins.length, aExpectedCount);
 
-  let outCount = {};
-  let logins = Services.logins.searchLogins(outCount, newPropertyBag(aQuery));
-  Assert.equal(outCount.value, expectedLogins.length);
+  let logins = Services.logins.searchLogins(newPropertyBag(aQuery));
   LoginTestUtils.assertLoginListsEqual(logins, expectedLogins);
 }
 
 /**
  * Prepare data for the following tests.
  */
 add_task(function test_initialize() {
   for (let login of TestData.loginList()) {