1627658 - Save doorhanger on form submit is not displayed for fields where new credentials were filled in after deleting autofilled ones

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect, P1)

Description

[Timea Cernea [:tbabos][inactive]]

Attachment: Recording of the issue

Affected Versions
Nightly 76.0a1 (2020-04-05)

Tested on
Windows 10 x64

Steps to reproduce

1. Have 1 set of credentials saved on facebook.com
2. Load the Log in Form (ensure credentials are autofilled)
3. Delete the autofilled credentials and type in new ones (observe the grey key icon)
4. Click on Log In

Expected
The save doorhanger should be displayed on form submit (replaces the dismissed one)

Actual
Save doorhanger on form submit is not displayed, the grey key for the dismissed doorhanger remains displayed

Regression-Range
Introduced via the implementation of Dismissed Doorhanger, Bug 1536728.

Note
Beta 75 has the pref disabled for the dismiss doorhanger.

Comment 1

[Timea Cernea [:tbabos][inactive]]

Attachment: Log.txt

Comment 2

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

Confirmed, I see this in the log (and when I reproduced locally)
_compareAndUpdatePreviouslySentValues: values are equivalent, returning true

...which would prevent the parent process from knowing about and handling the form submission and showing the capture UI. I'll need to dig deeper to see what is going on, and how bug 1536728 might have regressed this.

Comment 3

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

I think what's going on here is that

• facebook is using pushState onload, we are treating that as navigation and as the login form has been autofilled and the username and password have values, we end up treating this as an actual submit - populating our lastSubmittedValuesByRootElement cache with the autofilled values, with the dismissed value of false. The parent process ends up ignoring this message as the username and password values match the existing saved login which we autofilled.
• Later, when you clear and put new values in the username and password fields, the change event goes down the same code path. We have logic in _compareAndUpdatePreviouslySentValues which causes the previous value of dismissed to override the current value. The cache is updated with the new values and the message gets sent to the parent process. LoginManagerParent ignores the dismissed:false property as this is a onPasswordEditedOrGenerated event for which all doorhangers will be dismissed, so we get the expected result at this point.
• When you click submit to login, we go back down the same code path. But the exact same message was already sent - same username, password and dismissed value, so it is ignored and the parent process never hears about it; no doorhanger is created.

Comment 4

[Matthew N. [:MattN]]

(In reply to Sam Foster [:sfoster] (he/him) from comment #3)

I think what's going on here is that

• facebook is using pushState onload, we are treating that as navigation and as the login form has been autofilled and the username and password have values, we end up treating this as an actual submit - populating our lastSubmittedValuesByRootElement cache with the autofilled values, with the dismissed value of false.

Maybe we can add a check for form.document.userHasInteracted at the top of _formHasModifiedFields?

Comment 5

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

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Comment 6

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

:sfoster, since this bug is a regression, could you fill (if possible) the regressed_by field?
For more information, please visit auto_nag documentation.

Comment 7

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

(In reply to Release mgmt bot [:sylvestre / :calixte / :marco for bugbug] from comment #6)

:sfoster, since this bug is a regression, could you fill (if possible) the regressed_by field?

This bug comes out of bug 1603226 (which enables behavior added in bug 1388674), and bug 1536728. If it was necessary to back something out to undo the regression, I guess 1603226 would be the best bet - so I've got the regressed_by field pointing at that. I have a working patch that should be able to be uplifted to fix this however.

Comment 8

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

Attachment: Bug 1627658 - Avoid login capture until user interacts with the document. r?MattN

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/030ff13c60d1
Avoid login capture until user interacts with the document. r=MattN

Comment 10

[Stefan Hindli [:stefan_hindli]]

Backed out changeset 030ff13c60d1 (Bug 1627658) for windows mochitest failures in browser/tools/mozscreenshots/permissionPrompts/browser_permissionPrompts.js

https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=297310941

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=297310941&repo=autoland&lineNumber=2271

Comment 11

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/da2b1829e1fd
Backed out changeset 030ff13c60d1 for windows mochitest failures in browser/tools/mozscreenshots/permissionPrompts/browser_permissionPrompts.js

Comment 12

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

thanks, I've broken this test before, I always forget about it :/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f1d1fe7d1a84
Avoid login capture until user interacts with the document. r=MattN

Comment 14

[Cristina Coroiu [:ccoroiu]]

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

Comment 15

[Timea Cernea [:tbabos][inactive]]

Verified-fixed on Nightly 77.0a1 (2020-04-15) (Build ID: 20200415040917) - from Treeherder
Verified on Windows 10, MacOS 10.13 and Ubuntu 18.04.
Also submitted follow-up Bug 1630191 for the "Show password" toggle, more info in that bug.

Comment 16

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

Comment on attachment 9139354 [details]
Bug 1627658 - Avoid login capture until user interacts with the document. r?MattN

Beta/Release Uplift Approval Request

• User impact if declined: No prompt to save/update password when logging into e.g. facebook.com with changed credentials
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: (see comment #0)
• Have 1 set of credentials saved on facebook.com
• Load the Log in Form (ensure credentials are autofilled)
• Delete the autofilled credentials and type in new ones (observe the grey key icon)
• Click on Log In

Expected
The save doorhanger should be displayed on form submit (replaces the dismissed one)

• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Small change to existing logic in the password manager code.
• String changes made/needed: None

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9139354 [details]
Bug 1627658 - Avoid login capture until user interacts with the document. r?MattN

This has conflicts with Beta. Please attach a rebased patch or request approval on any other dependent bugs.

Comment 18

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

Attachment: (rebased for beta) Bug 1627658 - Avoid login capture until user interacts with the document. r?MattN

Beta/Release Uplift Approval Request

• User impact if declined: No prompt to save/update password when logging into e.g. facebook.com with changed credentials
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: (see comment #0)
• Have 1 set of credentials saved on facebook.com
  • Load the Log in Form (ensure credentials are autofilled)
  • Delete the autofilled credentials and type in new ones (observe the grey key icon)
  • Click on Log In
• Expected
  • The save doorhanger should be displayed on form submit (replaces the dismissed one)
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Small change to existing logic in the password manager code.
• String changes made/needed: None

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9140907 [details]
(rebased for beta) Bug 1627658 - Avoid login capture until user interacts with the document. r?MattN

Approved for 76.0b6.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/28fc014e0546

Comment 21

[Timea Cernea [:tbabos][inactive]]

Verified fixed on Windows 10 x64, MacOS 10.13 and Ubuntu 16.04 on latest Beta 76.0b6.