1243722 - Wrong doorhanger when updating password on Facebook

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect, P1)

Description

[Simona Badau, Desktop QA]

Mozilla/5.0 (X11; Linux i686; rv:47.0) Gecko/20100101 Firefox/47.0 Build ID: 20160127030236
Mozilla/5.0 (X11; Linux i686; rv:43.0) Gecko/20100101 Firefox/43.0 Build ID: 20160105164030

When updating the password from the website's setting page, a new username is added instead of just updating the password for the existing one.

Steps to reproduce:
1. Launch the latest Nightly with a new profile.
2. Navigate to https://www.facebook.com/
3. Enter your e-mail address and password 
4. Click on the Log-in button
5. In the password doorhanger choose to Remember your log-in 
6. On your Facebook page -> go to Settings -> Click Edit in the Password section
7. Add the new password, re-type it and click on Save Changes.

Expected results:
The password doorhanger with the same username should prompt with the option to Update the password. In the Saved logins list I should have only one username with the updated password.

Actual results:
The password doorhanger with a changed username (instead of the e-mail address used in step 3 I got "2") prompts me to Remember (instead of Update) the log-in as if this is a new log-in when in reality is the same user but with an updated password. A new login is entered in the Saved Logins list.

If I log out first and then I enter my e-mail address with a different password I get the right information in the password doorhanger: the user field is completed with my e-mail address and I get the option to Update the password.

This is also reproducible with other websites: Yahoo for instance - If I update the password while being logged in, I get an empty username and the Remember option in the password doorhanger. 

Reproducible on all platforms (Windows, Mac OS X and Ubuntu) on the latest Nightly 47 and Firefox 43.0.4.

Comment 1

[Matthew N. [:MattN]]

Perhaps another regression from bug 1016051?

Comment 2

[Kanchan Kumari QA]

Regression Range:

Pushlog
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8122ffa9e1aa&tochange=b0e7f63c2138

I couldn't bisect further because these builds are probably too old. I guess changeset https://hg.mozilla.org/mozilla-central/rev/27061dc242e4 might have caused the issue. 

Probably a regression from https://bugzilla.mozilla.org/show_bug.cgi?id=956906

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

The login form comes with <input name="email"> and <input name="pass">, and the change password form comes with <input name="password_old">, <input name="password_new">, and <input name="password_confirm">. I wonder if our password manager heustic support this?

Comment 4

[Tim Guan-tin Chien [:timdream] (please needinfo)]

After attempting to change my own password, I found that we incorrectly capture a

<input class="inputtext" name="password_strength" id="password_strength" type="text">

input box as the username field in that form. The input is inside an `display: hidden` <tr>.

(Hint for testing: you can prevent actually change your password by typing your old password into the new password field)

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

The username field was located by this:

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/LoginManagerContent.jsm#659-671

I wonder if the easiest solution here is just create a recipe for facebook.com? Would that hard to maintain in the long run? I am afraid that adding more checks (e.g. see if the <input> is visible) will break more sites.

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: MozReview Request: Bug 1243722 - Introduce notUsernameSelector in passwordmgr recipes, r=MattN

Additionally, add a recipe for Facebook.

Review commit: https://reviewboard.mozilla.org/r/42917/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42917/

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8735698 [details]
MozReview Request: Bug 1243722 - Introduce notUsernameSelector in passwordmgr recipes, r=MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42917/diff/1-2/

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8735698 [details]
MozReview Request: Bug 1243722 - Introduce notUsernameSelector in passwordmgr recipes, r=MattN

This is not the best approach. I am touching the logic specified in test_recipe_login_fields.html, so if we want to alter the behavior, I would need to fix the test too.

Alternative would be create another property in the recipe like "skipUsernameFieldIfNotFound" and have the if block in question skip locating the field completely if there isn't one found from the "usernameSelector".

Of course, the new flag need it's own set of test(s).

:MattN, what do you think?

Comment 9

[Matthew N. [:MattN]]

I didn't look closely at this yet but one idea we have been planning to implement is a recipe property to indicate fields that aren't username or password fields e.g.:

> notUsername: "#password_strength"

Comment 10

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/42917/#review40199

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:659
(Diff revision 2)
> -    if (!usernameField) {
> +    // Only try to locate username field when it didn't specify a usernameSelector,
> +    // or when there isn't a recipe at all.

I think this approach will make recipes too fragile and I prefer the `notUsernameSelector` approach.

Comment 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8735698 [details]
MozReview Request: Bug 1243722 - Introduce notUsernameSelector in passwordmgr recipes, r=MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42917/diff/2-3/

Comment 12

[Tim Guan-tin Chien [:timdream] (please needinfo)]

try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8bcab3aa78c

Comment 13

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8735698 [details]
MozReview Request: Bug 1243722 - Introduce notUsernameSelector in passwordmgr recipes, r=MattN

The test is not valid -- we will only reach the loop if there isn't a usernameSelector.

Comment 14

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8735698 [details]
MozReview Request: Bug 1243722 - Introduce notUsernameSelector in passwordmgr recipes, r=MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42917/diff/3-4/

Comment 15

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a62b77c48145

The test was rearranged with SpawnTask.js because the number of async tasks needed -- I hope this does not make diff too hard to read!

Comment 16

[Matthew N. [:MattN]]

Comment on attachment 8735698 [details]
MozReview Request: Bug 1243722 - Introduce notUsernameSelector in passwordmgr recipes, r=MattN

https://reviewboard.mozilla.org/r/42917/#review40931

Great, thanks!

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:666
(Diff revision 4)
>          if (LoginHelper.isUsernameFieldType(element)) {
> +          if (fieldOverrideRecipe && fieldOverrideRecipe.notUsernameSelector) {
> +            if (!element.matches(fieldOverrideRecipe.notUsernameSelector)) {
> -          usernameField = element;
> +              usernameField = element;
> -          break;
> +              break;
> -        }
> +            }
> +          } else {
> +            usernameField = element;
> +            break;
> +          }
> +        }
>        }

We normally prefer early-return style (though the old code wasn't doing so) and I think this would be clearer like so:
```
if (!LoginHelper.isUsernameFieldType(element)) {
  continue;
}

if (fieldOverrideRecipe && fieldOverrideRecipe.notUsernameSelector &&
    !element.matches(fieldOverrideRecipe.notUsernameSelector)) {
  continue;
}

usernameField = element;
break;
```

::: toolkit/components/passwordmgr/LoginRecipes.jsm:11
(Diff revision 4)
>  
>  this.EXPORTED_SYMBOLS = ["LoginRecipesContent", "LoginRecipesParent"];
>  
>  const { classes: Cc, interfaces: Ci, results: Cr, utils: Cu } = Components;
>  const REQUIRED_KEYS = ["hosts"];
> -const OPTIONAL_KEYS = ["description", "passwordSelector", "pathRegex", "usernameSelector"];
> +const OPTIONAL_KEYS = ["description", "passwordSelector", "pathRegex", "usernameSelector", "notUsernameSelector"];

Nit: keep these sorted alphabetically

::: toolkit/components/passwordmgr/test/mochitest/test_recipe_login_fields.html:13
(Diff revision 4)
>  </head>
>  <body>
> -<script type="application/javascript;version=1.8">
> +<script>
> -SimpleTest.waitForExplicitFinish();
>  
> +var inputEventDeferreds = [];

Nit: use `let`

::: toolkit/components/passwordmgr/test/mochitest/test_recipe_login_fields.html:15
(Diff revision 4)
> -<script type="application/javascript;version=1.8">
> +<script>
> -SimpleTest.waitForExplicitFinish();
>  
> +var inputEventDeferreds = [];
> +
> +document.addEventListener("input", function handleInputEvent(evt) {

Could you attach this to #content instead to avoid catching other fills from pwmgr_common in the future? I guess it would need to move in setup() then

::: toolkit/components/passwordmgr/test/mochitest/test_recipe_login_fields.html:26
(Diff revision 4)
> +  function Deferred() {
> +    this.promise = new Promise((resolve) => {
> +      this.resolve = resolve;
> +    });
> +  }

Interesting approach… I thought Deferred was frowned upon, at least it is in browser code and Promise.defer was removed. I don't really care though it would be nice to avoid the boilerplate in this test.

::: toolkit/components/passwordmgr/test/mochitest/test_recipe_login_fields.html:42
(Diff revision 4)
> +function loadRecipes(recipes) {
> +  pwmgrCommonScript.sendAsyncMessage("loadRecipes", recipes);
> +  return new Promise((resolve) => {
> +    pwmgrCommonScript.addMessageListener("loadedRecipes", resolve);
> +  });
> +}

Can you put this in pwmgr_common.js too (if there's not something like it there already)?

::: toolkit/components/passwordmgr/test/mochitest/test_recipe_login_fields.html:102
(Diff revision 4)
> -  }
> -}
> +  elements.forEach((element) => {
> +    is(element.dataset.expected, "true", `${element.name} was filled`);
> +  });

Nit for the future: for…of is generally preferred over forEach as it's a more natural control structure

::: toolkit/components/passwordmgr/test/mochitest/test_recipe_login_fields.html:107
(Diff revision 4)
> +add_task(function* resetRecipes() {
> +  pwmgrCommonScript.sendAsyncMessage("resetRecipes", {});
> +
> +  yield new Promise((resolve) => {
> +    pwmgrCommonScript.addMessageListener("doneResetRecipes", resolve);
> +  });
> +});

Could you put this function in pwmgr_common.js then call it with add_task here? I was planning on putting on there for other tests soon anyways.

::: toolkit/components/passwordmgr/test/mochitest/test_recipe_login_fields.html:115
(Diff revision 4)
> +  yield new Promise((resolve) => {
> +    pwmgrCommonScript.addMessageListener("doneResetRecipes", resolve);
> +  });
> +});
> +
> +add_task(function* loadSecondRecipes() {

Nit: loadNotUsernameSelectorRecipes

::: toolkit/components/passwordmgr/test/mochitest/test_recipe_login_fields.html:126
(Diff revision 4)
> +  });
> +});
> +
> +add_task(function* test_not_username_field() {
> +  document.getElementById("content").innerHTML = `
> +    <!-- The field matches notUsernameSelector should be skipped -->

Nit: s/matches/matching/

Comment 17

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/42917/#review40931

> Can you put this in pwmgr_common.js too (if there's not something like it there already)?

I thought it was already there but I guess it was the patch in bug 1258860 that's not landed yet. If you want to take it from that patch and put it in the same place in the file then it will be easy for me to rebase over your patch.

> Could you put this function in pwmgr_common.js then call it with add_task here? I was planning on putting on there for other tests soon anyways.

Same here. I put a helper in bug 1258860.

Comment 18

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8735698 [details]
MozReview Request: Bug 1243722 - Introduce notUsernameSelector in passwordmgr recipes, r=MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42917/diff/4-5/

Comment 19

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Review comment addressed.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e9685d8b55d

Comment 20

[Tim Guan-tin Chien [:timdream] (please needinfo)]

https://reviewboard.mozilla.org/r/42917/#review40931

> Interesting approach… I thought Deferred was frowned upon, at least it is in browser code and Promise.defer was removed. I don't really care though it would be nice to avoid the boilerplate in this test.

The `fillPromiseResolvers` array now saves only the `resolve` function.

> Nit: loadNotUsernameSelectorRecipes

I've also converted all function names to camalCase.

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/1bad1b961817

Comment 22

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=8472666&repo=fx-team

Comment 23

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/8d3c7fe68960

Comment 24

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Sigh. The document loads slower than add_task() on CI. I added a readyState check to guard it.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8acd98600cd6

Comment 25

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8735698 [details]
MozReview Request: Bug 1243722 - Introduce notUsernameSelector in passwordmgr recipes, r=MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42917/diff/5-6/

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea109c3f6b3c

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ea109c3f6b3c

Comment 28

[Matthew N. [:MattN]]

Comment on attachment 8735698 [details]
MozReview Request: Bug 1243722 - Introduce notUsernameSelector in passwordmgr recipes, r=MattN

Approval Request Comment
[Feature/regressing bug #]: This seems like mostly a Facebook change or perhaps longstanding behaviour. They have an <input> before the password field that we detect as the username.
[User impact if declined]: Firefox will ask to save the new password as a new login with a garbage username (some number related to password strength) instead of asking to update the existing password. This also adds support for recipes to fix other sites where we detect the wrong username field.
[Describe test coverage new/current, TreeHerder]: New mochitests
[Risks and why]: Low risk change to allow recipes to affect the chosen password field.
[String/UUID change made/needed]: None

Comment 29

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Simona, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

Comment 30

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8735698 [details]
MozReview Request: Bug 1243722 - Introduce notUsernameSelector in passwordmgr recipes, r=MattN

Fixes a regression when changing passwords on FB, Aurora47+

Comment 31

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

MattN, Liz, should we consider uplifting this to Beta46? I know the bar is super high as we are in RC week but the user impact here seems severe enough to consider taking a fix this late.

Comment 32

[Liz Henry (:lizzard) (relman/hg->git project)]

Yes. I just talked with Tim and he will check in a little while to make sure the patches apply correctly to beta in this and bug 1243729. We should uplift these to 46 for the RC2 build.

Comment 33

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Patch for m-b

Test changes don't apply to m-b.

Comment 34

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ca4b59788a9

Comment 35

[Matthew N. [:MattN]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f51382b3b967

This still needs to land on m-r but I'll make sure m-b doesn't burn first.

Comment 36

[Matthew N. [:MattN]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8210de2a2b4f

Comment 37

[Matthew N. [:MattN]]

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #35)
> https://hg.mozilla.org/releases/mozilla-beta/rev/f51382b3b967
> 
> This still needs to land on m-r but I'll make sure m-b doesn't burn first.

Tomcat, could you land this on m-r please? It looks like it's not going to bounce.

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-release/rev/b5e5521a6420

Comment 39

[Liz Henry (:lizzard) (relman/hg->git project)]

Simona - Would you mind testing this with a build from mozilla-release first? (Though it still would be nice to verify in nightly)

Comment 40

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8743077 [details] [diff] [review]
Patch for m-b

Belated approval. Thanks!

Comment 41

[Andrei Vaida [:avaida]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #39)
> Simona - Would you mind testing this with a build from mozilla-release
> first? (Though it still would be nice to verify in nightly)

We'll have this tested as well during the sign off of 46.0-build4. Leaving the ni? in place until we follow up with test results.

Comment 42

[Cornel Ionce [:noni] [Hubs QA]]

Confirming that the correct doorhanger is now displayed for facebook and other websites when updating the password for an account.
Tested during the Fx 46.0-build4 (build ID 20160420024437) sign-off across platform.

Additional details are available in the actual sign off email sent for Fx46.0 RC2 (build 4), on r-d.

Comment 43

[Simona Badau, Desktop QA]

Verified that the issue is no longer reproducible when following the steps from the Description - when the Facebook password is updated, the Update option and the same username are displayed in the password door hanger.

The verification was done on the latest Nightly 48.a1 (Build ID:20160420030213) and on the latest Aurora 47.0a2 (Build ID:20160421004015) on Windows 10(64bit), Mac OS X 10.9 and Ubuntu 15.04. 

I can still reproduce the Yahoo issue that was also mentioned in the Description - I logged Bug 1266381 to cover that problem.