Closed Bug 1627658 Opened 1 year ago Closed 1 year ago

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

Categories

(Toolkit :: Password Manager, defect, P1)

Desktop
All
defect

Tracking

()

VERIFIED FIXED
mozilla77
Tracking Status
firefox-esr68 --- unaffected
firefox74 --- unaffected
firefox75 --- disabled
firefox76 --- verified
firefox77 --- verified

People

(Reporter: tbabos, Assigned: sfoster)

References

(Regression)

Details

(Keywords: regression)

Attachments

(4 files)

Attached video 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.

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.

Priority: -- → P1
Assignee: nobody → sfoster
Status: NEW → ASSIGNED

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.

(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?

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

Keywords: regression

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

Flags: needinfo?(sfoster)

(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.

Flags: needinfo?(sfoster)
Regressed by: 1603226
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/030ff13c60d1
Avoid login capture until user interacts with the document. r=MattN
Backout by shindli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/da2b1829e1fd
Backed out changeset 030ff13c60d1 for windows mochitest failures in browser/tools/mozscreenshots/permissionPrompts/browser_permissionPrompts.js

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

Flags: needinfo?(sfoster)
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f1d1fe7d1a84
Avoid login capture until user interacts with the document. r=MattN
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla77

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 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
Attachment #9139354 - Flags: approval-mozilla-beta?
Flags: qe-verify+

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.

Flags: needinfo?(sfoster)
Attachment #9139354 - Flags: approval-mozilla-beta?

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
Flags: needinfo?(sfoster)
Attachment #9140907 - Flags: approval-mozilla-beta?
QA Whiteboard: [qa-triaged]
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.
Attachment #9140907 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.