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

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect, P2)

Description

[commerce]

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.

Comment 1

[Peter Magyari (Desktop QA)]

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!

Comment 2

[Matthew N. [:MattN]]

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.

Comment 3

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

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

Comment 4

[Mark Hammond [:markh] [:mhammond]]

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?

Comment 5

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

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

Comment 6

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

Comment 7

[Matthew N. [:MattN]]

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

Comment 8

[Matthew N. [:MattN]]

Attachment: Bug 1587927 - Check Services.logins.uiBusy in doAutocompleteSearch. r=sfoster

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/401fe1200b7f
Check Services.logins.uiBusy in doAutocompleteSearch. r=sfoster

Comment 10

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/401fe1200b7f

Comment 11

[Adrian Florinescu [:aflorinescu]]

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

Comment 12

[Matthew N. [:MattN]]

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

Comment 13

[Matthew N. [:MattN]]

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

Comment 14

[Pascal Chevrel:pascalc]

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.

Comment 15

[Arthur Iakab [arthur_iakab]]

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?

Comment 16

[Matthew N. [:MattN]]

Attachment: Bug 1587927 - Check Services.logins.uiBusy in doAutocompleteSearch. r=sfoster a=pascalc

Comment 17

[Matthew N. [:MattN]]

https://phabricator.services.mozilla.com/D51334 can be landed on beta

Comment 18

[Matthew N. [:MattN]]

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

Comment 19

[Timea Cernea [:tbabos][inactive]]

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

Comment 20

[Timea Cernea [:tbabos][inactive]]

Attachment: Recording in latest Nightly ( 2019 10 31 1947080) - Not Fixed - Scenario 3

Comment 21

[Matthew N. [:MattN]]

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

Comment 22

[Timea Cernea [:tbabos][inactive]]

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)