1787619 - firefox view tab pickup setup flow gets stuck if primary password prompt was cancelled

Status: VERIFIED FIXED

(Firefox :: Firefox View, defect, P1)

Description

[Doug Henderson]

Attachment: TroubleshootingInfo.txt

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:106.0) Gecko/20100101 Firefox/106.0

Steps to reproduce:

• Observed the Firefox view icon at the left end of the tab bar.
• selected it, and observed a button to start using it.
• received dialog to enter primary password (I had cancelled the entry when I started nightly.)
• Tried to enter password several times, very carefully, but unsuccessfully. This may be a problem as I almost always get the password correct when I am careful.
• Tried to cancel the password dialog, hoping to cancel setting up the tab pickup feature. This did not work.
• Finally my primary password was accepted after several more attempts during which I was watching which keys I pressed.

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:106.0) Gecko/20100101 Firefox/106.0

Computer network name: mars
Machine type: AMD64
Processor type: Intel64 Family 6 Model 158 Stepping 9, GenuineIntel
Platform type: Windows-10-10.0.19043-SP0
Operating system: Windows
Operating system release: 10
Operating system version: 10.0.19043

Actual results:

• Entering the password seemed to fail even when entered correctly.
• Cancelling the primary password dialog did not cancel setting up the tab pickup feature.

Expected results:

• Entering the primary password should have worked as reliably as as when the dialog is presented at other times.
• Cancelling the primary password dialog should have cancelled setting up the tab pickup feature.

Comment 1

[Doug Henderson]

Attachment: TroubleshootingInfo.json

Comment 2

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

The Bugbug bot thinks this bug should belong to the 'Core::Networking' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 3

[:Gijs (he/him)]

(In reply to Doug Henderson from comment #0)

Steps to reproduce:

• Observed the Firefox view icon at the left end of the tab bar.
• selected it, and observed a button to start using it.

What state was the setup in at this point? Did you still need to sign into a Firefox Account?

• received dialog to enter primary password (I had cancelled the entry when I started nightly.)
• Tried to enter password several times, very carefully, but unsuccessfully. This may be a problem as I almost always get the password correct when I am careful.

If there was a primary password prompt it wasn't created directly by Firefox View so it's almost impossible for this to be any different from any of the other prompts.

I tried to reproduce these steps but can only see a primary password prompt if I have a profile that fulfills all of these criteria:

1. has a primary password set
2. has a password saved for https://accounts.firefox.com/
3. has not yet input the primary password
4. isn't currently signed in to their Firefox account

and then try to sign in to a Firefox account.

That sign in happens in a different tab and if I cancel the primary password prompt I can just close that tab to stop progressing through the setup flow. Going back to the Firefox View tab doesn't restart the setup flow or prompt for a primary password.

It sounds like maybe you're seeing the prompt because of something else, but it is very difficult to tell. Can you provide more details about what things look like when you're prompted for the password, and what exact part of the setup flow you're trying to use?

Comment 4

[Doug Henderson]

(In reply to :Gijs (he/him) from comment #3)

(In reply to Doug Henderson from comment #0)

Steps to reproduce:

1. has a primary password set
2. has a password saved for https://accounts.firefox.com/
3. has not yet input the primary password
4. isn't currently signed in to their Firefox account

This seems to describe the state I was in when I first saw and clicked on the Firefox view icon in the tab bar. I do not know how to also get this icon to appear on the task bar. Maybe I don't need to because the Firefox view now appears under my More tools... at the left end of the the address bar.

I had a button on the Firefox view page that said IIRC Tab sync instead of the current box that says "Nothing to see yet
The next time you open a page in Firefox on another device, grab it here like magic."

When I pressed that button, the primary password dialog appeared. As usual with this dialog, I could not interact with the current tab or the tab bar.

As I am very certain of this password, and a fairly fast touch typist (except first thing in the morning when my hands are shaky and clumsy), my first attempt to enter my password is (as usually) quite fast. When an attempt fails, my habit is to reenter with increasing care. In this case about four attempts failed before I tried to cancel the primary password dialog with the Cancel button. The primary password dialog came back immediately. I expected it the fail back to whatever wanted the primary password, and cause it to fail. It then took me at least two more attempt to correctly enter the primary password.

I do not know how to reach a state in which the Firefox view icon appears on the tab bar. I do not know how to turn off tab sync so I can try to repeat this process myself.

I'd be happy to follow up myself if I know how to get back to the same state where the problem occurred.

I use Nightly on this laptop and an Android tablet, and Firefox on an Android phone. I have an LTS Firefox on a Linux laptop. I only use Chrome when I must in order to access that will not work with Firefox Nightly.

Comment 5

[Bob Owen (:bobowen)]

gijs - I can reproduce this, I too had the tab appear (I assume on restart).

Clicking on the tab caused the Primary Password dialog, which just reappeared when cancelled.
So, I had to kill the browser.

On restart I removed the tab/button from the tab list.
If I add it back in to the toolbar and click it I get the same result.

Is there something specific I can look at to debug this?

Comment 6

[:Gijs (he/him)]

(In reply to Bob Owen (:bobowen) from comment #5)

gijs - I can reproduce this, I too had the tab appear (I assume on restart).

Clicking on the tab caused the Primary Password dialog, which just reappeared when cancelled.

Clicking on which tab? The FxView tab or an FxA sign in tab? And are you signed into FxA / sync or not?

So, I had to kill the browser.

On restart I removed the tab/button from the tab list.
If I add it back in to the toolbar and click it I get the same result.

Is there something specific I can look at to debug this?

A stack trace for what pops up the dialog, I guess (might need JS code from DumpJSStack() in order to be useful if using a C++ debugger). I don't know what would trigger this, and couldn't reproduce as described in my previous comment. We do not try to interact with passwords or logins directly in the FxView code, so I don't know why the primary password prompt would appear. See:

https://searchfox.org/mozilla-central/search?q=pass&path=firefoxview&case=false&regexp=false
https://searchfox.org/mozilla-central/search?q=log.%3Fin&path=firefoxview&case=false&regexp=true

Comment 7

[Bob Owen (:bobowen)]

(In reply to :Gijs (he/him) from comment #6)

(In reply to Bob Owen (:bobowen) from comment #5)

gijs - I can reproduce this, I too had the tab appear (I assume on restart).

Clicking on the tab caused the Primary Password dialog, which just reappeared when cancelled.

Clicking on which tab? The FxView tab or an FxA sign in tab? And are you signed into FxA / sync or not?

The FxView tab.
Not signed in to FxA or sync.

So, I had to kill the browser.

On restart I removed the tab/button from the tab list.
If I add it back in to the toolbar and click it I get the same result.

Is there something specific I can look at to debug this?

A stack trace for what pops up the dialog, I guess (might need JS code from DumpJSStack() in order to be useful if using a C++ debugger). I don't know what would trigger this, and couldn't reproduce as described in my previous comment. We do not try to interact with passwords or logins directly in the FxView code, so I don't know why the primary password prompt would appear. See:

OK I'll see what I can produce.
Leaving needinfo.

Comment 8

[Doug Henderson]

Clicking on which tab? The FxView tab or an FxA sign in tab? And are you signed into FxA / sync or not?

Could you clarify those short names (FxView, FxA) for those of us who are just Firefox browser users? TIA

Comment 9

[:Gijs (he/him)]

(In reply to Doug Henderson from comment #8)

Clicking on which tab? The FxView tab or an FxA sign in tab? And are you signed into FxA / sync or not?

Could you clarify those short names (FxView, FxA) for those of us who are just Firefox browser users? TIA

The tab that is really a button and looks like a pinned tab on the far end of the tabstrip and has just a nightly icon as the logo is the Firefox View tab (this is the title of the page on the top left as well, if you select it - the URL bar is empty when this tab is selected) - though per your earlier comment, perhaps you removed it:

I do not know how to reach a state in which the Firefox view icon appears on the tab bar. I do not know how to turn off tab sync so I can try to repeat this process myself.

The FxA tab would be the tab that opens when you click a button to try to sign into Firefox Accounts (aka FxA) and will have a URL bar value including https://accounts.firefox.com/.

Comment 10

[Bob Owen (:bobowen)]

(In reply to :Gijs (he/him) from comment #6)
...

A stack trace for what pops up the dialog, I guess (might need JS code from DumpJSStack() in order to be useful if using a C++ debugger). I don't know what would trigger this, and couldn't reproduce as described in my previous comment.

Here's a JS stack trace copied from the Browser Toolbox multiprocess debugger:
nsIPrompt_promptPassword (resource://gre/modules/Prompter.jsm#1543)
promptPassword (resource://gre/modules/Prompter.jsm#1306)
encrypt (resource://gre/modules/crypto-SDR.js#84)
ensureMPUnlocked (resource://services-sync/util.js#648)
unlockAndVerifyAuthState (resource://services-sync/sync_auth.js#331)
verifyLogin (resource://services-sync/service.js#814)
onNotify (resource://services-sync/service.js#1037)
WrappedNotify (resource://services-sync/util.js#200)
WrappedLock (resource://services-sync/util.js#156)
WrappedCatch (resource://services-sync/util.js#123)
login (resource://services-sync/service.js#1050)
sync (resource://services-sync/service.js#1328)
WrappedCatch (resource://services-sync/util.js#123)
sync (resource://services-sync/service.js#1336)
syncTabs (resource://services-sync/SyncedTabs.jsm#162)
syncTabs (resource://services-sync/SyncedTabs.jsm#260)
enter (resource:///modules/firefox-view-tabs-setup-manager.sys.mjs#107)
maybeUpdateUI (resource:///modules/firefox-view-tabs-setup-manager.sys.mjs#398)
<anonymous> (resource:///modules/firefox-view-tabs-setup-manager.sys.mjs#165)
<anonymous> (resource:///modules/firefox-view-tabs-setup-manager.sys.mjs#55)
<anonymous> (chrome://browser/content/tab-pickup-container.mjs#9)

Comment 11

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

This reproduces easily for me with a master-password set. One prompt is annoying but expected (and happens when interacting with the fxa hamburger menu too), but something in firefoxview seems to be not be "failing" on cancel correctly.

Comment 12

[Bob Owen (:bobowen)]

Attachment: Stack traces on second and third prompts

.

Comment 13

[Bob Owen (:bobowen)]

Attachment: Stack Traces on second and third prompts.txt

(In reply to Mark Hammond [:markh] [:mhammond] from comment #11)

This reproduces easily for me with a master-password set. One prompt is annoying but expected (and happens when interacting with the fxa hamburger menu too), but something in firefoxview seems to be not be "failing" on cancel correctly.

With this in mind I've attached the next two stack traces on subsequent prompts, in case they're useful.

Comment 14

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

Attachment: simple fix

SyncedTabs.jsm already does de-throttling, so this makes it a small bit better. There's something else that probably needs to handle this error state better but it's too late for me.

Comment 15

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

Attachment: correct patch

oops :)

Comment 16

[:Gijs (he/him)]

Thanks for figuring all this out!

Hm. I think the patch to make exit just always returns true will mean we don't stay in the "loading" state when the user cancels the pw prompt, even though we'll have no content, which seems bad. We should probably have a dedicated error state of some sort...

Looking at the second/third prompts, it looks like we re-enter maybeUpdateUI because the sync code sends an observer notification that we listen for, while we're already in maybeUpdateUI, so we'll just recurse infinitely. That seems like something we should just avoid, full stop. I'm not sure if just the try catch is sufficient for that...

Comment 17

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

Yeah, that state machine getting stuck certainly isn't ideal, but I certainly wasn't going to take that on :)

Comment 18

[:Gijs (he/him)]

Let's make sure we track this.

Comment 19

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

Thanks for all the details, I should be able to take this from here.

Comment 20

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

Attachment: Bug 1787619 - Handle primary password and sync login errors. r?Gijs!,markh

Comment 21

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

Here's what I have so far. I've been able to work around the really heinous outcomes here, but I'm still getting 2 primary password prompts if the user cancels the first:

• When fx-view is opened, the tab-pickup-container CE imports the TabsSetupFlowManager and its constructor runs
• this.fxaSignedIn is true because UIState.get().status says we're logged in (or at least login hasnt failed)
• So we pass through the error state exitConditions and get to the loading state where we call SyncTabs.syncTabs() as there's no recent tab sync according to that pref.
• That causes sync to require an actual fxa login which trips the primary password prompt.
• If the user enters the correct password here, everything works fine.
• If the user enters the incorrect password, they'll be repeatedly prompted for the password again. I think that's expected.
• If the user closes or cancels the PP prompt, a weave:service:login:error observer notification happens.
• As UIState.jsm inits much earlier than us, it will handle it first, causing refreshState() to happen, which results in a UIState.ON_UPDATE notification
• TabsSetupFlowManager handles the UIState update, we've still had no reason to think login has failed so we handle it in the synced-tabs-not-ready state entry, which again calls SyncedTabs.syncTabs()
• Again we try to login, again the primary password prompt.
• If the user cancels here, finally, I think at this point we have an opportunity to handle the weave:service:login:errornotification so we can set a syncLoginFailed flag to avoid repeating the same loop.

I added a this._updateInFlight flag to avoid recursing in there, but I'm not actually seeing that ever happen FWIW. (It might have been useful to separate the state updates from the UI updates, so we can update state potentially many times per tick, and just notify UI of any state change in the next tick. Doing that now would likely mean adjusting tests and other expectations so I'm loathe to do it unless it seems necessary.)

To avoid that 2nd primary password prompt after the user cancels the first, I think we'd need UIState.jsm to maintain a flag or status we can read when we get the update observer notification

Comment 22

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

UIState.jsm should reflect this state better. It currently maintains a state of STATUS_LOGIN_FAILED, but that's used when the user needs to reauth with fxa, and setting that to false when in this state would cause the rest of the browser UI to get into a bad state.

But what we might be able to do is add a new state attribute - say a bool accountCanSync, which would be false if we aren't signed in, login failed, or in this state. SyncedTabs.jsm itself should probably decline to even try syncing while in this state, meaning fxview probably doesn't need to handle this specific case unless it chose to.

wdyt?

Comment 23

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

(eg, something like the patch below makes sense to me. I think UIState still needs to grow support for this state so that fxview can choose to handle this state in a special way, but this patch alone probably makes life much better in terms of the existing sync-tabs UI in the hamburger menu etc.

index bdfb95daeae44..65a5deb9c0806 100644
--- services/sync/modules/SyncedTabs.jsm
+++ services/sync/modules/SyncedTabs.jsm
@@ -156,6 +156,11 @@ let SyncedTabsInternal = {
       );
       return false;
     }
+    if (Weave.Status.login != Weave.STATUS_OK) {
+      lazy.log.info("Can't sync due to the login status", Weave.Status.login);
+      return;
+    }
+
     // Ask Sync to just do the tabs engine if it can.
     try {
       lazy.log.info("Doing a tab sync.");

Comment 24

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

(In reply to Mark Hammond [:markh] [:mhammond] from comment #22)

UIState.jsm should reflect this state better. It currently maintains a state of STATUS_LOGIN_FAILED, but that's used when the user needs to reauth with fxa, and setting that to false when in this state would cause the rest of the browser UI to get into a bad state.

But what we might be able to do is add a new state attribute - say a bool accountCanSync, which would be false if we aren't signed in, login failed, or in this state. SyncedTabs.jsm itself should probably decline to even try syncing while in this state, meaning fxview probably doesn't need to handle this specific case unless it chose to.

wdyt?

Are you still proposing something like .accountCanSync in addition to the changes in bug 1789341?

There's 2 problems to solve in this bug. The first is the endless loop of primary password prompts. I think with bug 1789341 that is solved. The 2nd is communicating to the user and allowing them to recover when they cancelled or failed to unlock with primary password and are presented with the "Sit tight while your tabs sync. It’ll be just a moment." loading state in fx-view. Which is a lie in this case - it will not be a moment :) If we can detect that state in the state transition checks, we can show the error card which gives them a "try again" button which I think should result in them being re-prompted for their primary password - which is the best outcome. SyncedTabs.syncTabs() returns a promise which will eventually return false, but I'm trying to avoid those state transition check being asynchronous if at all possible. I think an .accountCanSync property might allow me to solve this?

I'm happy to include that in my patch if you can sketch out what and where the changes need to be made.

Comment 25

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

It would be great if you could pick that up.

I'm quite uncertain on the semantics we should expose here though - even if we look past the fact that accountCanSync is a bad name, it would accurately reflect what we did with SyncedTabs - ie, that Weave.Status.login != Weave.STATUS_OK. But from the point of view of the UI, that does not necessarily imply that the actual status is MASTER_PASSWORD_LOCKED - it almost certainly is if UIState.get() == UIState.STATUS_SIGNED_IN, but that's not certain.

From the POV of the UI, I assume that you will also want to offer some way to unlock - but I don't think it makes sense for UIState to offer that. So instead of UIState exposing a proxy for "is the primary password unlocked", it seems like getting the info from the source of truth is better - ie, ignore sync itself here and just use the password manager itself - ask it if the primary password is locked (sync will not work if it is) and ask it to unlock if when the user requests via whatever UI affordance you come up with.

https://searchfox.org/mozilla-central/source/toolkit/components/passwordmgr/LoginHelper.jsm seems to offer an unlock function - it doesn't seem to offer a simple "is locked" function, but you could just copy how sync does it here

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9239a20afb93
Handle primary password and sync login errors. r=Gijs,markh

Comment 27

[Noemi Erli[:noemi_erli]]

Backed out changeset 9239a20afb93 (Bug 1787619) for causing failures in browser_setup_errors.js CLOSED TREE

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&collapsedPushes=842382&selectedTaskRun=dCxQj3RGSDGOv64581q-ZQ.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=9239a20afb93cb8ef1aac08724a94d753880715d

Log: https://treeherder.mozilla.org/logviewer?job_id=390956686&repo=autoland&lineNumber=6612

Backout: https://hg.mozilla.org/integration/autoland/rev/950d39075e532edd4a16ea77c3bae2711f39cc07

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9254adef5dc4
Handle primary password and sync login errors. r=Gijs,markh

Comment 29

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

Thanks for the backout - looks like I didn't re-run the tests after renaming that function. Or I did and the change got lost in a rebase. Anyhow, fixed and re-queued for landing.

Comment 30

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/9254adef5dc4

Comment 31

[:Gijs (he/him)]

Updating the summary because bug 1789341 made this less terrible already.

Comment 32

[:Gijs (he/him)]

Comment on attachment 9292943 [details]
Bug 1787619 - Handle primary password and sync login errors. r?Gijs!,markh

Beta/Release Uplift Approval Request

• User impact if declined: Confusing UX if the user cancels a primary password prompt and then tries to use Firefox View
• 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: 1. have primary password set up

2. sign into a sync account
3. restart Firefox
4. cancel any primary password prompt
5. open Firefox View

There should be an error shown, and clicking "try again" should prompt for the primary password. There should not be an infinite loop of primary password prompts...

• List of other uplifts needed: Bug 1791652
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): This isn't the really lowest-risk uplift I'm asking for today, but I think between QE efforts, automated testing, and scrutiny from several engineers here, this is a reasonable thing to uplift at this stage in the beta cycle.
• String changes made/needed: Nope
• Is Android affected?: No

Comment 33

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9292943 [details]
Bug 1787619 - Handle primary password and sync login errors. r?Gijs!,markh

Approved for 106.0b4, thanks.

Comment 34

[Pascal Chevrel:pascalc (PTO until April 26)]

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

Comment 35

[Bogdan Maris, Desktop QA]

(In reply to :Gijs (he/him) from comment #32)

• If yes, steps to reproduce:

1. have primary password set up
2. sign into a sync account
3. restart Firefox
4. cancel any primary password prompt
5. open Firefox View

I verified that using latest Nightly 107.0a1 and Firefox 106.0b4 the Try Again button is available and clicking it will result in the Primary Password being prompted but, I did noticed two things:
A) After step 2, If I don't restart the browser and I open Firefox view the Try again button is clickable but does nothing.
B) After step 3 the user is still logged in into FxA, but once I go to Firefox View, click the Try Again button and enter the correct password the user will be automatically disconnected from the account.

Are these any concerns that need to be addressed into separate bugs?

Comment 36

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

(In reply to Bogdan Maris [:bogdan_maris], Release Desktop QA from comment #35)

A) After step 2, If I don't restart the browser and I open Firefox view the Try again button is clickable but does nothing.

Clicking the button should re-attempt tab-sync, which should require re-authentication, which should trigger the primary password prompt. If it doesn't in some cases, we need to figure out why...

B) After step 3 the user is still logged in into FxA, but once I go to Firefox View, click the Try Again button and enter the correct password the user will be automatically disconnected from the account.

This is unexpected, and I've not ever seen happen in working on this issue, so I'm very curious about what is happening there..

Can you file both (separately) and attach any associated browser console and about:sync logs. You can get verbose logging on firefox-view by setting the pref browser.tabs.firefox-view.logLevel to "All".

Comment 37

[Bogdan Maris, Desktop QA]

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

(In reply to Bogdan Maris [:bogdan_maris], Release Desktop QA from comment #35)

A) After step 2, If I don't restart the browser and I open Firefox view the Try again button is clickable but does nothing.

Clicking the button should re-attempt tab-sync, which should require re-authentication, which should trigger the primary password prompt. If it doesn't in some cases, we need to figure out why...

B) After step 3 the user is still logged in into FxA, but once I go to Firefox View, click the Try Again button and enter the correct password the user will be automatically disconnected from the account.

This is unexpected, and I've not ever seen happen in working on this issue, so I'm very curious about what is happening there..

Can you file both (separately) and attach any associated browser console and about:sync logs. You can get verbose logging on firefox-view by setting the pref browser.tabs.firefox-view.logLevel to "All".

Thanks Sam, I logged Bug 1792550 and Bug 1792553 for the above issues. I'll go ahead and close this one as verified based on my verification.