Bug 1135451 - fillForm() cleanup part G: move passwordmgr-processed-form notification into a finally-block. r=MattN
authorJustin Dolske <dolske@mozilla.com>
Fri, 27 Feb 2015 15:47:37 -0800
changeset 231215 c918b8a2b9c0aa6277728f36882db829cd1aa7d8
parent 231214 a71d846d255caff98022b0ef2e8267b529adf3d0
child 231216 a80f93233b34c0ffea8e8ba0668097a40c7d0970
push id11542
push userjdolske@mozilla.com
push dateFri, 27 Feb 2015 23:48:29 +0000
treeherderfx-team@a80f93233b34 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersMattN
bugs1135451
milestone39.0a1
Bug 1135451 - fillForm() cleanup part G: move passwordmgr-processed-form notification into a finally-block. r=MattN
toolkit/components/passwordmgr/LoginManagerContent.jsm
--- a/toolkit/components/passwordmgr/LoginManagerContent.jsm
+++ b/toolkit/components/passwordmgr/LoginManagerContent.jsm
@@ -262,17 +262,16 @@ var LoginManagerContent = {
         .then(null, Cu.reportError);
   },
 
   loginsFound: function({ form, loginsFound }) {
     let doc = form.ownerDocument;
     let autofillForm = gAutofillForms && !PrivateBrowsingUtils.isContentWindowPrivate(doc.defaultView);
 
     this._fillForm(form, autofillForm, false, false, loginsFound);
-    Services.obs.notifyObservers(form, "passwordmgr-processed-form", null);
   },
 
   /*
    * onUsernameInput
    *
    * Listens for DOMAutoComplete and blur events on an input field.
    */
   onUsernameInput : function(event) {
@@ -306,17 +305,16 @@ var LoginManagerContent = {
     // Make sure the username field fillForm will use is the
     // same field as the autocomplete was activated on.
     var [usernameField, passwordField, ignored] =
         this._getFormFields(acForm, false);
     if (usernameField == acInputField && passwordField) {
       this._asyncFindLogins(acForm, { showMasterPassword: false })
           .then(({ form, loginsFound }) => {
               this._fillForm(form, true, true, true, loginsFound);
-              Services.obs.notifyObservers(form, "passwordmgr-processed-form", null);
           })
           .then(null, Cu.reportError);
     } else {
       // Ignore the event, it's for some input we don't care about.
     }
   },
 
 
@@ -605,182 +603,186 @@ var LoginManagerContent = {
       if (userTriggered) {
         // Ignore fills as a result of user action.
         return;
       }
       const autofillResultHist = Services.telemetry.getHistogramById("PWMGR_FORM_AUTOFILL_RESULT");
       autofillResultHist.add(result);
     }
 
-    // Nothing to do if we have no matching logins available.
-    if (foundLogins.length == 0) {
-      // We don't log() here since this is a very common case.
-      recordAutofillResult(AUTOFILL_RESULT.NO_SAVED_LOGINS);
-      return;
-    }
-
-    // Heuristically determine what the user/pass fields are
-    // We do this before checking to see if logins are stored,
-    // so that the user isn't prompted for a master password
-    // without need.
-    var [usernameField, passwordField, ignored] =
-        this._getFormFields(form, false);
-
-    // Need a valid password field to do anything.
-    if (passwordField == null) {
-      log("not filling form, no password field found");
-      recordAutofillResult(AUTOFILL_RESULT.NO_PASSWORD_FIELD);
-      return;
-    }
+    try {
+      // Nothing to do if we have no matching logins available.
+      if (foundLogins.length == 0) {
+        // We don't log() here since this is a very common case.
+        recordAutofillResult(AUTOFILL_RESULT.NO_SAVED_LOGINS);
+        return;
+      }
 
-    // If the password field is disabled or read-only, there's nothing to do.
-    if (passwordField.disabled || passwordField.readOnly) {
-      log("not filling form, password field disabled or read-only");
-      recordAutofillResult(AUTOFILL_RESULT.PASSWORD_DISABLED_READONLY);
-      return;
-    }
+      // Heuristically determine what the user/pass fields are
+      // We do this before checking to see if logins are stored,
+      // so that the user isn't prompted for a master password
+      // without need.
+      var [usernameField, passwordField, ignored] =
+          this._getFormFields(form, false);
 
-    // Discard logins which have username/password values that don't
-    // fit into the fields (as specified by the maxlength attribute).
-    // The user couldn't enter these values anyway, and it helps
-    // with sites that have an extra PIN to be entered (bug 391514)
-    var maxUsernameLen = Number.MAX_VALUE;
-    var maxPasswordLen = Number.MAX_VALUE;
+      // Need a valid password field to do anything.
+      if (passwordField == null) {
+        log("not filling form, no password field found");
+        recordAutofillResult(AUTOFILL_RESULT.NO_PASSWORD_FIELD);
+        return;
+      }
 
-    // If attribute wasn't set, default is -1.
-    if (usernameField && usernameField.maxLength >= 0)
-      maxUsernameLen = usernameField.maxLength;
-    if (passwordField.maxLength >= 0)
-      maxPasswordLen = passwordField.maxLength;
-
-    var logins = foundLogins.filter(function (l) {
-      var fit = (l.username.length <= maxUsernameLen &&
-                 l.password.length <= maxPasswordLen);
-      if (!fit)
-        log("Ignored", l.username, "login: won't fit");
+      // If the password field is disabled or read-only, there's nothing to do.
+      if (passwordField.disabled || passwordField.readOnly) {
+        log("not filling form, password field disabled or read-only");
+        recordAutofillResult(AUTOFILL_RESULT.PASSWORD_DISABLED_READONLY);
+        return;
+      }
 
-      return fit;
-    }, this);
-
-    if (logins.length == 0) {
-      log("form not filled, none of the logins fit in the field");
-      recordAutofillResult(AUTOFILL_RESULT.NO_LOGINS_FIT);
-      return;
-    }
+      // Discard logins which have username/password values that don't
+      // fit into the fields (as specified by the maxlength attribute).
+      // The user couldn't enter these values anyway, and it helps
+      // with sites that have an extra PIN to be entered (bug 391514)
+      var maxUsernameLen = Number.MAX_VALUE;
+      var maxPasswordLen = Number.MAX_VALUE;
 
-    // Attach autocomplete stuff to the username field, if we have
-    // one. This is normally used to select from multiple accounts,
-    // but even with one account we should refill if the user edits.
-    if (usernameField)
-      this._formFillService.markAsLoginManagerField(usernameField);
+      // If attribute wasn't set, default is -1.
+      if (usernameField && usernameField.maxLength >= 0)
+        maxUsernameLen = usernameField.maxLength;
+      if (passwordField.maxLength >= 0)
+        maxPasswordLen = passwordField.maxLength;
 
-    // Don't clobber an existing password.
-    if (passwordField.value && !clobberPassword) {
-      log("form not filled, the password field was already filled");
-      recordAutofillResult(AUTOFILL_RESULT.EXISTING_PASSWORD);
-      return;
-    }
+      var logins = foundLogins.filter(function (l) {
+        var fit = (l.username.length <= maxUsernameLen &&
+                   l.password.length <= maxPasswordLen);
+        if (!fit)
+          log("Ignored", l.username, "login: won't fit");
 
-    // If the form has an autocomplete=off attribute in play, don't
-    // fill in the login automatically. We check this after attaching
-    // the autocomplete stuff to the username field, so the user can
-    // still manually select a login to be filled in.
-    var isFormDisabled = false;
-    if (!ignoreAutocomplete &&
-        (this._isAutocompleteDisabled(form) ||
-         this._isAutocompleteDisabled(usernameField) ||
-         this._isAutocompleteDisabled(passwordField))) {
+        return fit;
+      }, this);
+
+      if (logins.length == 0) {
+        log("form not filled, none of the logins fit in the field");
+        recordAutofillResult(AUTOFILL_RESULT.NO_LOGINS_FIT);
+        return;
+      }
 
-      isFormDisabled = true;
-      log("form not filled, has autocomplete=off");
-    }
+      // Attach autocomplete stuff to the username field, if we have
+      // one. This is normally used to select from multiple accounts,
+      // but even with one account we should refill if the user edits.
+      if (usernameField)
+        this._formFillService.markAsLoginManagerField(usernameField);
 
-    // Select a login to use for filling in the form.
-    var selectedLogin;
-    if (usernameField && (usernameField.value || usernameField.disabled || usernameField.readOnly)) {
-      // If username was specified in the field, it's disabled or it's readOnly, only fill in the
-      // password if we find a matching login.
-      var username = usernameField.value.toLowerCase();
-
-      let matchingLogins = logins.filter(function(l)
-                                         l.username.toLowerCase() == username);
-      if (matchingLogins.length == 0) {
-        log("Password not filled. None of the stored logins match the username already present.");
-        recordAutofillResult(AUTOFILL_RESULT.EXISTING_USERNAME);
+      // Don't clobber an existing password.
+      if (passwordField.value && !clobberPassword) {
+        log("form not filled, the password field was already filled");
+        recordAutofillResult(AUTOFILL_RESULT.EXISTING_PASSWORD);
         return;
       }
 
-      // If there are multiple, and one matches case, use it
-      for (let l of matchingLogins) {
-        if (l.username == usernameField.value) {
-          selectedLogin = l;
+      // If the form has an autocomplete=off attribute in play, don't
+      // fill in the login automatically. We check this after attaching
+      // the autocomplete stuff to the username field, so the user can
+      // still manually select a login to be filled in.
+      var isFormDisabled = false;
+      if (!ignoreAutocomplete &&
+          (this._isAutocompleteDisabled(form) ||
+           this._isAutocompleteDisabled(usernameField) ||
+           this._isAutocompleteDisabled(passwordField))) {
+
+        isFormDisabled = true;
+        log("form not filled, has autocomplete=off");
+      }
+
+      // Select a login to use for filling in the form.
+      var selectedLogin;
+      if (usernameField && (usernameField.value || usernameField.disabled || usernameField.readOnly)) {
+        // If username was specified in the field, it's disabled or it's readOnly, only fill in the
+        // password if we find a matching login.
+        var username = usernameField.value.toLowerCase();
+
+        let matchingLogins = logins.filter(function(l)
+                                           l.username.toLowerCase() == username);
+        if (matchingLogins.length == 0) {
+          log("Password not filled. None of the stored logins match the username already present.");
+          recordAutofillResult(AUTOFILL_RESULT.EXISTING_USERNAME);
+          return;
         }
-      }
-      // Otherwise just use the first
-      if (!selectedLogin) {
+
+        // If there are multiple, and one matches case, use it
+        for (let l of matchingLogins) {
+          if (l.username == usernameField.value) {
+            selectedLogin = l;
+          }
+        }
+        // Otherwise just use the first
+        if (!selectedLogin) {
+          selectedLogin = matchingLogins[0];
+        }
+      } else if (logins.length == 1) {
+        selectedLogin = logins[0];
+      } else {
+        // We have multiple logins. Handle a special case here, for sites
+        // which have a normal user+pass login *and* a password-only login
+        // (eg, a PIN). Prefer the login that matches the type of the form
+        // (user+pass or pass-only) when there's exactly one that matches.
+        let matchingLogins;
+        if (usernameField)
+          matchingLogins = logins.filter(function(l) l.username);
+        else
+          matchingLogins = logins.filter(function(l) !l.username);
+
+        if (matchingLogins.length != 1) {
+          log("Multiple logins for form, so not filling any.");
+          recordAutofillResult(AUTOFILL_RESULT.MULTIPLE_LOGINS);
+          return;
+        }
+
         selectedLogin = matchingLogins[0];
       }
-    } else if (logins.length == 1) {
-      selectedLogin = logins[0];
-    } else {
-      // We have multiple logins. Handle a special case here, for sites
-      // which have a normal user+pass login *and* a password-only login
-      // (eg, a PIN). Prefer the login that matches the type of the form
-      // (user+pass or pass-only) when there's exactly one that matches.
-      let matchingLogins;
-      if (usernameField)
-        matchingLogins = logins.filter(function(l) l.username);
-      else
-        matchingLogins = logins.filter(function(l) !l.username);
+
+      // We will always have a selectedLogin at this point.
 
-      if (matchingLogins.length != 1) {
-        log("Multiple logins for form, so not filling any.");
-        recordAutofillResult(AUTOFILL_RESULT.MULTIPLE_LOGINS);
+      if (!autofillForm) {
+        log("autofillForms=false but form can be filled; notified observers");
+        recordAutofillResult(AUTOFILL_RESULT.NO_AUTOFILL_FORMS);
         return;
       }
 
-      selectedLogin = matchingLogins[0];
-    }
-
-    // We will always have a selectedLogin at this point.
+      if (isFormDisabled) {
+        log("autocomplete=off but form can be filled; notified observers");
+        recordAutofillResult(AUTOFILL_RESULT.AUTOCOMPLETE_OFF);
+        return;
+      }
 
-    if (!autofillForm) {
-      log("autofillForms=false but form can be filled; notified observers");
-      recordAutofillResult(AUTOFILL_RESULT.NO_AUTOFILL_FORMS);
-      return;
-    }
+      // Fill the form
 
-    if (isFormDisabled) {
-      log("autocomplete=off but form can be filled; notified observers");
-      recordAutofillResult(AUTOFILL_RESULT.AUTOCOMPLETE_OFF);
-      return;
-    }
-
-    // Fill the form
+      if (usernameField) {
+      // Don't modify the username field if it's disabled or readOnly so we preserve its case.
+        let disabledOrReadOnly = usernameField.disabled || usernameField.readOnly;
 
-    if (usernameField) {
-      // Don't modify the username field if it's disabled or readOnly so we preserve its case.
-      let disabledOrReadOnly = usernameField.disabled || usernameField.readOnly;
+        let userNameDiffers = selectedLogin.username != usernameField.value;
+        // Don't replace the username if it differs only in case, and the user triggered
+        // this autocomplete. We assume that if it was user-triggered the entered text
+        // is desired.
+        let userEnteredDifferentCase = userTriggered && userNameDiffers &&
+               usernameField.value.toLowerCase() == selectedLogin.username.toLowerCase();
 
-      let userNameDiffers = selectedLogin.username != usernameField.value;
-      // Don't replace the username if it differs only in case, and the user triggered
-      // this autocomplete. We assume that if it was user-triggered the entered text
-      // is desired.
-      let userEnteredDifferentCase = userTriggered && userNameDiffers &&
-             usernameField.value.toLowerCase() == selectedLogin.username.toLowerCase();
+        if (!disabledOrReadOnly && !userEnteredDifferentCase && userNameDiffers) {
+          usernameField.setUserInput(selectedLogin.username);
+        }
+      }
+      if (passwordField.value != selectedLogin.password) {
+        passwordField.setUserInput(selectedLogin.password);
+      }
 
-      if (!disabledOrReadOnly && !userEnteredDifferentCase && userNameDiffers) {
-        usernameField.setUserInput(selectedLogin.username);
-      }
+      recordAutofillResult(AUTOFILL_RESULT.FILLED);
+    } finally {
+      Services.obs.notifyObservers(form, "passwordmgr-processed-form", null);
     }
-    if (passwordField.value != selectedLogin.password) {
-      passwordField.setUserInput(selectedLogin.password);
-    }
-
-    recordAutofillResult(AUTOFILL_RESULT.FILLED);
   },
 
 };
 
 var LoginUtils = {
   /*
    * _getPasswordOrigin
    *