Bug 1632405 - Fix login autofill-> delete-> fill-> save flow;r=MattN
authorSeverin <srudie@mozilla.com>
Tue, 28 Apr 2020 23:37:56 +0000
changeset 526586 87f89c55ae64fbafc3c23fe1d1190e0180a05660
parent 526585 2f45a8fc88cbe09111de12ccee4dc2b05ceb4c8f
child 526587 6bb8423186c1bb8c1229249454de46efb7d4d584
push id37358
push useropoprus@mozilla.com
push dateWed, 29 Apr 2020 03:05:14 +0000
treeherdermozilla-central@6bb8423186c1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersMattN
bugs1632405, 1532377
milestone77.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 1632405 - Fix login autofill-> delete-> fill-> save flow;r=MattN RCA: Bug 1532377 moved `docState` (inside `_maybeSendFormInteractionMessage`) closer to its use site. This accidentally changed the order of a short, which resulted in site-prompted changes being saved in `_compareAndUpdatePreviouslySentValues`, later causing `_compareAndUpdatePreviouslySentValues` to return false on true form submit events. Differential Revision: https://phabricator.services.mozilla.com/D72270
toolkit/components/passwordmgr/LoginManagerChild.jsm
toolkit/components/passwordmgr/test/browser/browser.ini
toolkit/components/passwordmgr/test/browser/browser_doorhanger_autofill_then_save_password.js
toolkit/components/passwordmgr/test/browser/browser_doorhanger_save_password.js
--- a/toolkit/components/passwordmgr/LoginManagerChild.jsm
+++ b/toolkit/components/passwordmgr/LoginManagerChild.jsm
@@ -1600,43 +1600,43 @@ this.LoginManagerChild = class LoginMana
           CreditCard.isValidNumber(usernameValue) &&
           newPasswordFieldValue.trim().match(/^[0-9]{3}$/)) ||
         (CreditCard.isValidNumber(newPasswordFieldValue) &&
           newPasswordField.getAutocompleteInfo().fieldName == "cc-number")
       ) {
         dismissedPrompt = true;
       }
 
+      let docState = this.stateForDocument(doc);
+      let fieldsModified = this._formHasModifiedFields(form);
+      if (!fieldsModified && LoginHelper.userInputRequiredToCapture) {
+        if (targetField) {
+          throw new Error("No user input on targetField");
+        }
+        // we know no fields in this form had user modifications, so don't prompt
+        log(
+          `(${logMessagePrefix} ignored -- submitting values that are not changed by the user)`
+        );
+        return;
+      }
+
       if (
         this._compareAndUpdatePreviouslySentValues(
           form.rootElement,
           usernameValue,
           newPasswordField.value,
           dismissedPrompt
         )
       ) {
         log(
           `(${logMessagePrefix} ignored -- already submitted with the same username and password)`
         );
         return;
       }
 
-      let docState = this.stateForDocument(doc);
-      let fieldsModified = this._formHasModifiedFields(form);
-      if (!fieldsModified && LoginHelper.userInputRequiredToCapture) {
-        if (targetField) {
-          throw new Error("No user input on targetField");
-        }
-        // we know no fields in this form had user modifications, so don't prompt
-        log(
-          `(${logMessagePrefix} ignored -- submitting values that are not changed by the user)`
-        );
-        return;
-      }
-
       let { login: autoFilledLogin } =
         docState.fillsByRootElement.get(form.rootElement) || {};
       let browsingContextId = win.windowGlobalChild.browsingContext.id;
 
       detail = {
         origin,
         browsingContextId,
         formActionOrigin,
--- a/toolkit/components/passwordmgr/test/browser/browser.ini
+++ b/toolkit/components/passwordmgr/test/browser/browser.ini
@@ -34,16 +34,17 @@ support-files =
 [browser_basicAuth_rateLimit.js]
 [browser_basicAuth_switchTab.js]
 skip-if = (debug && os == "mac") # Bug 1530566
 [browser_context_menu.js]
 [browser_context_menu_autocomplete_interaction.js]
 skip-if = verify
 [browser_context_menu_generated_password.js]
 [browser_context_menu_iframe.js]
+[browser_doorhanger_autofill_then_save_password.js]
 [browser_doorhanger_crossframe.js]
 support-files =
   form_crossframe.html
   form_crossframe_inner.html
 [browser_doorhanger_dismissed_for_ccnumber.js]
 [browser_doorhanger_empty_password.js]
 [browser_doorhanger_form_password_edit.js]
 [browser_doorhanger_generated_password.js]
copy from toolkit/components/passwordmgr/test/browser/browser_doorhanger_save_password.js
copy to toolkit/components/passwordmgr/test/browser/browser_doorhanger_autofill_then_save_password.js
--- a/toolkit/components/passwordmgr/test/browser/browser_doorhanger_save_password.js
+++ b/toolkit/components/passwordmgr/test/browser/browser_doorhanger_autofill_then_save_password.js
@@ -1,163 +1,149 @@
 /**
- * Test that the doorhanger notification for password saving is populated with
- * the correct values in various password capture cases.
+ * Test that after we autofill, the site makes changes to the login, and then the
+ * user modifies their login, a save/update doorhanger is shown.
+ *
+ * This is a regression test for Bug 1632405.
  */
 
 const testCases = [
   {
-    name: "No saved logins, username and password",
-    username: "username",
-    password: "password",
-    expectOutcome: [
+    name: "autofill, then delete u/p, then fill new u/p should show 'save'",
+    oldUsername: "oldUsername",
+    oldPassword: "oldPassword",
+    actions: [
       {
-        username: "username",
-        password: "password",
+        setUsername: "newUsername",
+      },
+      {
+        setPassword: "newPassword",
       },
     ],
-  },
-  {
-    name: "No saved logins, password with empty username",
-    username: "",
-    password: "password",
-    expectOutcome: [
-      {
-        username: "",
-        password: "password",
-      },
-    ],
+    expectedNotification: "addLogin",
+    expectedDoorhanger: "password-save",
   },
   {
-    name: "Saved login with username, update password",
-    username: "username",
-    oldPassword: "password",
-    password: "newPassword",
-    expectOutcome: [
+    name:
+      "autofill, then delete password, then fill new password should show 'update'",
+    oldUsername: "oldUsername",
+    oldPassword: "oldPassword",
+    actions: [
       {
-        username: "username",
-        password: "newPassword",
+        setPassword: "newPassword",
       },
     ],
-  },
-  {
-    name: "Saved login with no username, update password",
-    username: "",
-    oldPassword: "password",
-    password: "newPassword",
-    expectOutcome: [
-      {
-        username: "",
-        password: "newPassword",
-      },
-    ],
-  },
-  {
-    name: "Saved login with no username, add username and different password",
-    oldUsername: "",
-    username: "username",
-    oldPassword: "password",
-    password: "newPassword",
-    expectOutcome: [
-      {
-        username: "",
-        password: "password",
-      },
-      {
-        username: "username",
-        password: "newPassword",
-      },
-    ],
+    expectedNotification: "modifyLogin",
+    expectedDoorhanger: "password-change",
   },
 ];
 
 for (let testData of testCases) {
   let tmp = {
     async [testData.name]() {
       info("testing with: " + JSON.stringify(testData));
       await test_save_change(testData);
     },
   };
   add_task(tmp[testData.name]);
 }
 
-async function test_save_change(testData) {
-  let {
-    oldUsername,
-    username,
-    oldPassword,
-    password,
-    expectOutcome,
-  } = testData;
-  // Add a login for the origin of the form if testing a change notification.
-  if (oldPassword) {
-    Services.logins.addLogin(
-      LoginTestUtils.testData.formLogin({
-        origin: "https://example.com",
-        formActionOrigin: "https://example.com",
-        username: typeof oldUsername !== "undefined" ? oldUsername : username,
-        password: oldPassword,
-      })
-    );
-  }
+async function test_save_change({
+  name,
+  oldUsername,
+  oldPassword,
+  actions,
+  expectedNotification,
+  expectedDoorhanger,
+}) {
+  let originalPrefValue = await Services.prefs.getBoolPref(
+    "signon.testOnlyUserHasInteractedByPrefValue"
+  );
+  Services.prefs.setBoolPref(
+    "signon.testOnlyUserHasInteractedByPrefValue",
+    false
+  );
+
+  info("Starting test: " + name);
+
+  Services.logins.addLogin(
+    LoginTestUtils.testData.formLogin({
+      origin: "https://example.com",
+      formActionOrigin: "https://example.com",
+      username: oldUsername,
+      password: oldPassword,
+    })
+  );
 
   await BrowserTestUtils.withNewTab(
     {
       gBrowser,
       url:
         "https://example.com/browser/toolkit/components/" +
         "passwordmgr/test/browser/form_basic.html",
     },
     async function(browser) {
       await SimpleTest.promiseFocus(browser.ownerGlobal);
 
-      // Submit the form in the content page with the credentials from the test
-      // case. This will cause the doorhanger notification to be displayed.
-      info(`update form with username: ${username}, password: ${password}`);
-      await changeContentFormValues(browser, {
-        "#form-basic-username": username,
-        "#form-basic-password": password,
-      });
+      await ContentTask.spawn(
+        browser,
+        [oldPassword],
+        async function awaitAutofill() {
+          await ContentTaskUtils.waitForCondition(
+            () =>
+              !!content.document.querySelector("#form-basic-username").value,
+            "await autofill"
+          );
+
+          info(
+            "Triggering a page navigation that is not initiated by the user"
+          );
+          content.history.replaceState({}, "", "");
+        }
+      );
+
+      Services.prefs.setBoolPref(
+        "signon.testOnlyUserHasInteractedByPrefValue",
+        true
+      );
+
+      for (let action of actions) {
+        info(`As the user, update form with action: ${JSON.stringify(action)}`);
+        if (typeof action.setUsername !== "undefined") {
+          await changeContentFormValues(browser, {
+            "#form-basic-username": action.setUsername,
+          });
+        }
+        if (typeof action.setPassword !== "undefined") {
+          await changeContentFormValues(browser, {
+            "#form-basic-password": action.setPassword,
+          });
+        }
+      }
 
       let formSubmittedPromise = listenForTestNotification("FormSubmit");
       await SpecialPowers.spawn(browser, [], async function() {
         let doc = this.content.document;
         doc.getElementById("form-basic").submit();
       });
       await formSubmittedPromise;
 
-      // Simulate the action on the notification to request the login to be
-      // saved, and wait for the data to be updated or saved based on the type
-      // of operation we expect.
-      let expectedNotification, expectedDoorhanger;
-      if (oldPassword !== undefined && oldUsername !== undefined) {
-        expectedNotification = "addLogin";
-        expectedDoorhanger = "password-save";
-      } else if (oldPassword !== undefined) {
-        expectedNotification = "modifyLogin";
-        expectedDoorhanger = "password-change";
-      } else {
-        expectedNotification = "addLogin";
-        expectedDoorhanger = "password-save";
-      }
-
       info("Waiting for doorhanger of type: " + expectedDoorhanger);
       let notif = await waitForDoorhanger(browser, expectedDoorhanger);
 
-      // Check the actual content of the popup notification.
-      await checkDoorhangerUsernamePassword(username, password);
-
       let promiseLogin = TestUtils.topicObserved(
         "passwordmgr-storage-changed",
         (_, data) => data == expectedNotification
       );
       await clickDoorhangerButton(notif, REMEMBER_BUTTON);
       await promiseLogin;
       await cleanupDoorhanger(notif); // clean slate for the next test
 
-      // Check that the values in the database match the expected values.
-      verifyLogins(expectOutcome);
+      Services.prefs.setBoolPref(
+        "signon.testOnlyUserHasInteractedByPrefValue",
+        originalPrefValue
+      );
     }
   );
 
   // Clean up the database before the next test case is executed.
   Services.logins.removeAllLogins();
 }
--- a/toolkit/components/passwordmgr/test/browser/browser_doorhanger_save_password.js
+++ b/toolkit/components/passwordmgr/test/browser/browser_doorhanger_save_password.js
@@ -104,24 +104,25 @@ async function test_save_change(testData
       gBrowser,
       url:
         "https://example.com/browser/toolkit/components/" +
         "passwordmgr/test/browser/form_basic.html",
     },
     async function(browser) {
       await SimpleTest.promiseFocus(browser.ownerGlobal);
 
-      // Submit the form in the content page with the credentials from the test
-      // case. This will cause the doorhanger notification to be displayed.
+      // Update the form with credentials from the test case.
       info(`update form with username: ${username}, password: ${password}`);
       await changeContentFormValues(browser, {
         "#form-basic-username": username,
         "#form-basic-password": password,
       });
 
+      // Submit the form with the new credentials. This will cause the doorhanger
+      // notification to be displayed.
       let formSubmittedPromise = listenForTestNotification("FormSubmit");
       await SpecialPowers.spawn(browser, [], async function() {
         let doc = this.content.document;
         doc.getElementById("form-basic").submit();
       });
       await formSubmittedPromise;
 
       // Simulate the action on the notification to request the login to be