1638297 - Edits are not saved on the fly for the generated password non-confirm field if a confirm password field is present

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect, P1)

Description

[Timea Cernea [:tbabos][inactive]]

Attachment: Recording of the issue

Tested on:
Windows 10
MacOS

Affected builds:
Nightly 78.0a1 (2020-05-14)
Beta 77.0b7

Steps to reproduce:

1. Go on a register form, ex: Wikipedia
2. Click on the Password field and select Use a Securely Generated password
   **the generated password will be filled in the Password (unmasked) and Confirm password field as well (masked)
3. Edit the generated password (in the Password field) and focus out of the field

Expected:
The "Password Saved!" toast should be displayed and edits autosaved.

Actual result:
The toast is not displayed and edits are not auto-saved.

Notes:
Only the edits for the Confirm password field are auto-saved. Maybe the edits of the second field are captured as it is seen as the last field that was autofilled with a generated password?

Regression-range:
This seems to be because of Bug 1576490 , as now we also autofill the second password field with the generated password.
On Release 76 (where we don't have the above functionality) the edits are saved on the fly correctly for both fields.

Comment 1

[Timea Cernea [:tbabos][inactive]]

Attachment: LogText.txt

Comment 2

[Pascal Chevrel:pascalc]

I am assuming that this is not enough of a worry for a fix this week and a late uplift in 77, Matt wdyt?

Comment 3

[Matthew N. [:MattN]]

Sam, can you look at this regression?

(In reply to Pascal Chevrel:pascalc from comment #2)

I am assuming that this is not enough of a worry for a fix this week and a late uplift in 77, Matt wdyt?

Originally I thought the bug was describing what we expected for now but I see that's not the case when I read it closer. I don't know how fast Sam can fix it and how risky the fix will be. This may be worth backing out bug 1576490 for though. Sam, what do you think?

Comment 4

[Sam Foster [:sfoster] (he/him)]

It looks like we are treating the registration form with 2 password fields (password, confirm password) as a change password form (old password, new password) so yeah we don't get the value of the correct field, so we don't see a change, so we don't message the parent, so we don't update the login and see the toast.

I've not yet figured out why bug 1576490 caused this regression though, so I'm not ready to say we should back it out. I can look closer tomorrow.

Comment 5

[Sam Foster [:sfoster] (he/him)]

The return value from _getFormFields is pretty ambiguous in this case. We assume 2 password fields must be old-password, new-password. I wonder if _getFormFields should return a dictionary and fill in oldPassword, newPassword, confirmPassword properties so the caller doesnt have to guess?

I guess by filling both password fields, we changed the output of heuristics around https://searchfox.org/mozilla-central/rev/df4c90d4b8c92c99f76334acfe4813c573c12661/toolkit/components/passwordmgr/LoginManagerChild.jsm#1338 where we compare field values. In this specific case the field values differ as the user just edited one of them. We know exactly which fields we identified and filled as the password and confirm-password field, so we can definitely fix this with low risk.

Comment 6

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1638297 - LMC._getFormFields returns a dictionary with usernameField, newPasswordField, oldPasswordField. r?MattN

Comment 7

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1638297 - Look at fields filled with generated passwords when figuring out newPasswordField and oldPasswordField. r?MattN

Depends on D76412

Comment 8

[Matthew N. [:MattN]]

After talking about it more, since we are out of time to fix this in pre-RC due to the shorter cycle, we will accept this regression for 77.

Comment 9

[Sam Foster [:sfoster] (he/him)]

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8a8749e8adba
LMC._getFormFields returns a dictionary with usernameField, newPasswordField, oldPasswordField. r=MattN
https://hg.mozilla.org/integration/autoland/rev/09bf5aac5810
Look at fields filled with generated passwords when figuring out newPasswordField and oldPasswordField. r=MattN,severin

Comment 11

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/8a8749e8adba
https://hg.mozilla.org/mozilla-central/rev/09bf5aac5810

Comment 12

[Timea Cernea [:tbabos][inactive]]

Verified-fixed on latest Nightly 79.0a1 (2020-06-11) (64-bit) on WIndows 10, MacOS 10.13 and Ubuntu 16.04.

Comment 13

[Julien Cristau [:jcristau]]

Did you want to uplift this to 78?

Comment 14

[Sam Foster [:sfoster] (he/him)]

(In reply to Julien Cristau [:jcristau] from comment #13)

Did you want to uplift this to 78?

In principle, yes, but we're going to let this bake on nightly for a few more days before making that decision.

Comment 15

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:sfoster, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 16

[Sam Foster [:sfoster] (he/him)]

Thanks Release mgmt bot. See comment #14, we'll be monitoring and making a decision this week I think.

Comment 17

[Julien Cristau [:jcristau]]

Might be better to let this ride the trains to 79, we're about to build the last 78 beta...

Comment 18

[Matthew N. [:MattN]]

Sad… burned again by the shorter release cycle :(

I haven't seen any fallout yet so I think it is ready for uplift.

Comment 19

[Matthew N. [:MattN]]

Comment on attachment 9150920 [details]
Bug 1638297 - LMC._getFormFields returns a dictionary with usernameField, newPasswordField, oldPasswordField. r?MattN

Beta/Release Uplift Approval Request

• User impact if declined: Edits to the generated password field won't be automatically saved, potentially resulting in a password without edits being saved.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): New logic that isn't trivial
• String changes made/needed: None

Comment 20

[Julien Cristau [:jcristau]]

Comment on attachment 9150920 [details]
Bug 1638297 - LMC._getFormFields returns a dictionary with usernameField, newPasswordField, oldPasswordField. r?MattN

approved for 78 rc1

Comment 21

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/757866c8d64d
https://hg.mozilla.org/releases/mozilla-beta/rev/c5a047653451

Comment 22

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/757866c8d64d0189237a81225ec2c7beaf0858e4
https://hg.mozilla.org/releases/mozilla-esr78/rev/c5a047653451b4fcf62918989ac0bfa6ab2c079d

Comment 23

[Timea Cernea [:tbabos][inactive]]

Verified-fixed on latest Firefox RC 78.0 and 78.0esr on Windows 10, MacOS 10.13 and Ubuntu 16.04.