Closed Bug 1587927 Opened 10 months ago Closed 10 months ago

Multiple conflicting master password prompts at launch when Sync is configured and login autocomplete starts

Categories

(Toolkit :: Password Manager, defect, P2)

69 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla72
Tracking Status
firefox-esr68 --- wontfix
firefox67 --- wontfix
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- wontfix
firefox71 --- verified
firefox72 --- verified

People

(Reporter: commerce, Assigned: MattN)

References

Details

(Keywords: regression, Whiteboard: [passwords:master-password])

Attachments

(3 files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:69.0) Gecko/20100101 Firefox/69.0

Steps to reproduce:

It isn't 100% reproducible. The problem appears either immediately at launch, or a few seconds later. It's more likely to happen as the number of tabs being loaded grows.

I usually have at least two two things requiring access to stored passwords at launch: a pinned tab and the Firefox Sync service. It's enough to trigger the bug usually within 5-10 seconds after launch.

I have a master password set up.

Actual results:

At first a single master password prompt appears. While I'm still typing the password, additional prompts sometimes appear, stealing focus and interrupting the typing.

If I manage to type a full password in one of the prompts, the other prompts become useless but they don't disappear automatically.

On Mac, I also notice that sometimes the prompt is a separate window, and sometimes it's a modal dialog. It seems random and I wasn't able to correlate it to the aforementioned bug with any degree of certainty.

Expected results:

Firefox should control its own UI and be aware that a password prompt is already opened, to avoid opening additional ones needlessly.

This problem isn't new, it was already reported a few years ago, but it finally went away. But it seems to be back.

Hi,

I wasn't able to reproduce the issue, however, I'm going to set a component in order to involve the development team in reviewing it.

Thank you for reporting!

Component: Untriaged → Password Manager
Product: Firefox → Toolkit

This should have been fixed in bug 1382937 but maybe something else regressed it. Maybe the recent decoupling of Sync and FxA caused this?

We're seeing a few reports of this so it may be a real regression.

Blocks: 1382937
Component: Password Manager → Sync
Product: Toolkit → Firefox
Summary: Firefox: multiple conflicting master password prompts at launch → Firefox: multiple conflicting master password prompts at launch when Sync is configured

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

Keywords: regression

Maybe the recent decoupling of Sync and FxA caused this?

This report is against 69, so I don't think any of the recent sync changes can explain this - however, I can reproduce:

  • wait for sync to prompt for the master-password. Even though sync's unlock request ends up calling openModalWindow() in nsPrompter.js, the window opened is not modal. This might be fixable (this.domWin is null at this time but we can almost certainly arrange for one), but I'm not 100% convinced that will solve the underlying problem, but just make the STR more difficult.

  • Because the prompt isn't modal, we can interact with the browser. Switch-to/open a tab with a saved password and focus the password field. This also tries to unlock via this stack:

nsIPrompt_promptPassword@resource://gre/modules/Prompter.jsm:933:10
promptPassword@resource://gre/modules/Prompter.jsm:720:44
decrypt@resource://gre/modules/crypto-SDR.js:181:38
_decryptLogins@resource://gre/modules/storage-json.js:735:39
searchLogins@resource://gre/modules/storage-json.js:415:19
searchLogins@resource://gre/modules/LoginManager.jsm:483:26
searchLoginsWithObject@resource://gre/modules/LoginHelper.jsm:211:28
_searchAndDedupeLogins@resource://gre/modules/LoginManagerParent.jsm:165:28
doAutocompleteSearch@resource://gre/modules/LoginManagerParent.jsm:428:21
receiveMessage@resource://gre/modules/LoginManagerParent.jsm:222:21
openModalWindow@resource://gre/modules/Prompter.jsm:452:15
openPrompt@resource://gre/modules/Prompter.jsm:689:20
nsIPrompt_promptPassword@resource://gre/modules/Prompter.jsm:933:10
promptPassword@resource://gre/modules/Prompter.jsm:720:44
encrypt@resource://gre/modules/crypto-SDR.js:83:38
[snip the original sync stack causing the first window]

IOW, while the first dialog is opened, the loginmanager parent got a message indicating an autocomplete search should be started, which ends up recursively opening a prompt.

The reason I'm not 100% sure that fixing sync's dialog to be modal would work is that I'm not convinced that a modal dialog being opened is enough to prevent this loginmanager message from arriving in all cases - eg, I don't think a modal will completely stall any other background tabs that happen to be loading. LoginManagerParent does have a check that we haven't prompted for a master password recently, but I don't see a check to Services.logins.uiBusy anywhere - it correctly returns true as we recursively open the second dialog.

MattN, thoughts?

Flags: needinfo?(MattN+bmo)

:commerce, 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?(commerce)

I'd love to, but I'm not sure what the field is for. The documentation you linked doesn't explain much, and the report that is given as an example doesn't have anything in the regressed_by field. Some additional help would be welcome.

Flags: needinfo?(commerce)

Mark, I think you're right that it's actually the password manager missing the uiBusy check this time. I could have sworn that it wasn't possible for that to happen but I think I was thinking about FormAutofill and MasterPassword.jsm (which I think I should bring back).

Sorry about that

Assignee: nobody → MattN+bmo
Status: UNCONFIRMED → ASSIGNED
Component: Sync → Password Manager
Ever confirmed: true
Flags: needinfo?(MattN+bmo)
Priority: -- → P2
Product: Firefox → Toolkit
Summary: Firefox: multiple conflicting master password prompts at launch when Sync is configured → Multiple conflicting master password prompts at launch when Sync is configured and login autocomplete starts
Whiteboard: [passwords:master-password]
See Also: → 1583200
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/401fe1200b7f
Check Services.logins.uiBusy in doAutocompleteSearch. r=sfoster
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla72

Other than the obvious verify fix, what possible regression area should be covered while verifying this fix?

Flags: qe-verify+
Flags: needinfo?(MattN+bmo)
Flags: in-testsuite+

(In reply to Adrian Florinescu [:adrian_sv] from comment #11)

Other than the obvious verify fix, what possible regression area should be covered while verifying this fix?

You can look at how multiple windows requiring a master password at one time works e.g. autocompleting in two different windows at once. Also dealing with about:logins MP dialogs while dealing with login autocomplete.

Flags: needinfo?(MattN+bmo)

Comment on attachment 9104036 [details]
Bug 1587927 - Check Services.logins.uiBusy in doAutocompleteSearch. r=sfoster

Beta/Release Uplift Approval Request

  • User impact if declined: Users get multiple prompts for master password on startup or while loading tabs since login autocomplete can automatically popup when a username/password field is auto-focused. I've seen various duplicate reports over the last few months (one of which is probably bug 1583200).
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: See comment 12
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Fairly low risk since it's just adding a check to see if a MP dialog is already showing before requesting login autocomplete results. The main known issue would be that autocomplete will no longer trigger a master password dialog if one is already open on another window and that may confuse some users on platforms where the dialog isn't modal.
  • String changes made/needed: None
Attachment #9104036 - Flags: approval-mozilla-beta?

Comment on attachment 9104036 [details]
Bug 1587927 - Check Services.logins.uiBusy in doAutocompleteSearch. r=sfoster

Fix for a frequently reported regression, low risk patch with tests, uplift approved for 71 beta 6, thanks.

Attachment #9104036 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
QA Whiteboard: [qa-triaged]

Tried to uplift to beta but received conflict error message on toolkit/components/passwordmgr/test/browser/browser_master_password_autocomplete.js

Matthew can you please take a look?

Flags: needinfo?(MattN+bmo)
Attachment #9105680 - Attachment description: Bug 1587927 - Check Services.logins.uiBusy in doAutocompleteSearch. r=sfoster a=pascalc → Beta patch
Flags: needinfo?(MattN+bmo)
Attachment #9105680 - Attachment description: Beta patch → Bug 1587927 - Check Services.logins.uiBusy in doAutocompleteSearch. r=sfoster a=pascalc

~ tested on latest Nightly 72.0a1 - 20191031194708 ~
So, this was a bit more complicated than it looked at first glance :) ..but multiple master password prompt can be definitely triggered easily from easy steps to more complicated ones.

1. First simple scenario:

  • Have multiple (3-5) about:logins page opened and PINNED
  • Restore session enabled
  • Master Password set up
  • Restart Firefox

Actual result:
A Master Password prompt will be opened (overlapping each other) for each pinned about:logins tab: https://imgur.com/a/yc578Lc

2. Second Scenario:

  • Have at least one about:logins tab pinned
  • Master Password set up
  • Have a facebook tab pinned that has saved credentials (for autocomplete on page load)

Actual result:
Two master password prompts will be opened -> one for about:logins and one for facebook autofill: https://imgur.com/Q8AdIGt

3. The third scenario (involves Sync):

  • A combination of the two from above: facebook pinned tab, about:logins and sync enabled.

Actual result:
Besides the opened prompts for the pinned tabs, Firefox Sync may trigger another MP prompt after 10 seconds or so, while the other prompts are still active. Please see the recording.
That white master password prompt is a bonus...reproduced it twice and accidentally jumped into my recording.

TL:DR: Pinned tabs, about:logins, sync can still cause multiple MP prompts, this issue is not fixed, 80% reproducibility rate for 3rd scenario.
Matt, please take a look at this when you have the time, I know its a lot but I can break it down to different bugs if you wish.

Flags: needinfo?(MattN+bmo)

It looks like about:logins is the common thread in all of those STR so can you please file comment 19 as an about:logins bug. I doubt it's a regression, it's probably from the initial about:logins implementation. Can you repro any way without about:logins? I care more about that for this bug.

Thanks

Flags: needinfo?(MattN+bmo)

Thanks Matt,

I can't trigger multiple MP modals without about:logins, as you also noticed. Marking this as verified - fixed on latest Beta71.0b6 and Nightly on Windows 10 x64, MacOS 10.13 and Ubuntu 18.04.
Will log the above scenarios as new bug in about:logins. (edit: that would be Bug 1593983)

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