Closed Bug 1017224 Opened 9 years ago Closed 8 years ago

[B2G][Settings]Firefox Account Sign-in: Verification message does not display until killing and relaunching the settings app

Categories

(Firefox OS Graveyard :: FxA, defect, P2)

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:2.0+, b2g-v1.4 unaffected, b2g-v2.0 verified, b2g-v2.1 verified)

VERIFIED FIXED
2.0 S5 (4july)
blocking-b2g 2.0+
Tracking Status
b2g-v1.4 --- unaffected
b2g-v2.0 --- verified
b2g-v2.1 --- verified

People

(Reporter: astole, Assigned: jhirsch)

References

()

Details

(Whiteboard: [fxa4fxos2.0])

Attachments

(4 files)

After creating a new Firefox Account and verifying it, it isn't possible to completely log into the account. Settings has to be fully closed in order to proceed to the account page and get the verification message.

Repro Steps:
1) Update a Flame to BuildID: 20140528040205
2) Open Settings app and scroll down and tap on 'Firefox Accounts'
3) Enter an email address that has not been verified
4) Enter age over 13
5) Enter password and press Done
6) Press the 'X' to return to the Settings app
7) Verify account through email sent to the email address from step 3

Actual:
Verify message does not display until the Settings app has been killed and relaunched

Expected:
Settings app does not have to be force closed in order to receive the verification message

2.0 Environmental Variables:
Device: Flame 2.0 MOZ
BuildID: 20140528040205
Gaia: bc6f07c149770c6e6dfbea941ac65138dc364a15
Gecko: e017c15325ae
Version: 32.0a1
Firmware Version: v10G-2

Repro frequency: 100%
See attached: Video, logcat
User agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Issue DOES occur on 2.0 Buri. Verification message does not display until the Settings app has been killed and relaunched

Environmental Variables:
Device: Buri v2.0 Master M-C Mozilla RIL
BuildID: 20140528040205
Gaia: bc6f07c149770c6e6dfbea941ac65138dc364a15
Gecko: e017c15325ae
Version: 32.0a1
Firmware Version: v1.2-device.cfg

This option is NOT available on 1.4.
Attaching video of tapping on Firefox Accounts option in Settings app before and after force closing Settings.
Attached file logcat
Attaching logcat
Seems like this is something basic not working here.
blocking-b2g: --- → 2.0?
Component: Gaia::Settings → FxA
Verified. This is really unfortunate. Digging in to see if it's Gecko not sending the callbacks, or Gaia not receiving them.
Assignee: nobody → 6a68
Priority: -- → P2
Whiteboard: [fxa4fxos2.0]
Still digging, but I am pretty sure the problem is this:

The settings app gets fxa events ('login', 'logout') from gecko through /shared/js/fxa_iac_client. This helper is written as though it were a singleton: it registers/deregisters listeners, and pushes fxa events to each listener as it receives them from gecko.

So, what happens if >1 app instantiates an fxa_iac_client? Does gecko manage multiple listeners for these events? Does it simply overwrite the pointer to the most recent subscriber? Does it ignore all but the first subscriber? What happens if an app (like Settings) is minimized, then reopened?

Continuing to dig.
Update: I found one bug, not sure it's *the* bug.

Whenever an IAC helper instance calls getAccounts() for the first time, the 'verified' key is never present in the response. All we get is the email.

The Settings UI code expects the 'verified' key is *always* present, and only displays the verified logged-in state if event.verified is present and true.

Reproing the getAccounts error after signup, before verifying:

1. open fxa test client, logged out state
2. sign up for a new account
3. after dialog is dismissed, click 'get accounts', and you will see a JSON blob with no 'verified' field
4. click 'get accounts' again, and you *will* see the 'verified' field is present, and its value is 'false'
5. click the verify link in your browser, wait a few seconds
6. click 'get accounts' again, the 'verified' field is always present

Reproing the getAccounts error after signing up and verifying:

1. test client, click 'log out'
2. test client, open flow and sign up
3. click verify link in a browser
4. now, click 'get accounts'
5. the JSON packet only has an 'email' field
6. click 'get accounts' again
7. 'verified':'true' is now present

To be continued tomorrow.
blocking-b2g: 2.0? → 2.0+
Jared, I landed a couple of Gecko patches over the weekend that may have affected this bug. I am taking a look at exactly how the events flow in Gecko, which should help narrow down the problem.
With the latest mozilla-central, verifying the email causes Gecko to lob an event towards the system app:

  mozFxAccountsUnsolChromeEvent: fxaccounts:onverified

The last lines of fxa_manager.js are listening for:

  onverifiedlogin

when I edit them to catch the actual string, Gecko then immediately receives a getAccounts() call, so yay progress. The call returns a mozFxAccountsChromeEvent with {"email":"10172243@mailinator.com","verified":true}, but the Settings UI doesn't refresh. Happy to dig further; LMK.
Hey Sam - Sounds like you're already looking into this, so feel free to steal it if your schedule allows.
Assignee: 6a68 → spenrose
This has been sitting for a few days, stealing it back. Shout if you've got code ready to go.
Assignee: spenrose → 6a68
Jared I will totally let you steal this back. Here's a bit to smooth the message passing; you can of course unify the Gaia spelling to match Gecko.
Comment on attachment 8440188 [details] [review]
Github PR 20517

Hi again, gents!

Here's another straightforward b2g-blocking bug fix, please take a look when you have a moment.

Thanks!

Jared
Attachment #8440188 - Flags: review?(ferjmoreno)
Attachment #8440188 - Flags: review?(arthur.chen)
It seems the bug was due to that the event name was changed from "onverifiedlogin" to "onverified"?

As my email addresses were all registered to firefox accounts, is there any other way to test it (or ways that I can use to revoke my account)?
Flags: needinfo?(6a68)
Hi Arthur,

The way I generally test signups is to use an existing email address, like 6a68@mozilla.com, but append '+' and a random string to the end of the user part, like 6a68+123@mozilla.com. The email will still be delivered, but it will look like a new account to firefox accounts, so you can go through signup, click the emailed verification link, and check that onverified behavior works properly.
Flags: needinfo?(6a68)
Is the expected behavior like once I've verified the account, the item will then change to signed in as xxx@xxx.com? If that's the case, the patch does not fix the issue. I still need to relaunch the settings app to see the signed in message.
Flags: needinfo?(6a68)
(In reply to Arthur Chen [:arthurcc] from comment #20)
> Is the expected behavior like once I've verified the account, the item will
> then change to signed in as xxx@xxx.com? If that's the case, the patch does
> not fix the issue. I still need to relaunch the settings app to see the
> signed in message.

Yup, that's the expected behavior: after verification, the settings app automatically updated from unverified to verified screens.

I will try to repro on my Flame today and look for lingering bugs :-(
Flags: needinfo?(6a68)
Hey Arthur,

The reported bug seems fixed to me, but I found (and fixed) a different bug when trying to reproduce your problem :-)

Two cases that work reliably for me:

Panel auto-updating:
1. go to settings fxa panel, sign up, leave the fxa panel visible
2. click verification email link in a desktop email client (settings app stays visible)
3. fxa panel updates correctly
4. after closing out of the panel, the fxa menu item also is updated

Menu auto-updating:
1. go to settings fxa panel, sign up, exit the fxa panel, so that fxa menu item is visible.
2. click verification email link in a desktop email client
3. fxa menu item updates correctly
4. click into fxa panel, it is also updated

I always use a second device to click the verify link when manually testing, it's just quickest.

I realized that, if the settings app is not visible when the verify link is clicked, then the UI is incorrect:
1. sign up from settings
2. close the settings app, go to homescreen
3. click verification link in a desktop email client
4. reopen the settings app
5. the settings app shows the wrong state!

This is happening because we detach/re-attach the event listeners on visibility change, but don't refresh the UI state after the app is shown again.

I've fixed this and pushed the update. Would you mind having another look?
Flags: needinfo?(arthur.chen)
When I tried to create a new account I was stuck after entering the password. It kept displaying the busy icon and displayed "connecting". Note that I still received the confirm mail and was able to verify it. The account was created successfully.

Gaia      b4c7cc2de31a2d73bca75467cfd11cc3479ac2c0
Gecko     https://hg.mozilla.org/mozilla-central/rev/f78e532e8a10
BuildID   20140618160204
Version   33.0a1
Flags: needinfo?(arthur.chen)
Hey Arthur,

That's odd. There is a known bug (bug 998464) around setting a timer to dismiss the 'Connecting...' overlay, but it only seems to happen when you lose network connectivity *after* starting the signup flow. Do you think that might be what happened in your case?

No-Jun, would you mind trying to repro this bug today? I'll try as well.
Flags: needinfo?(npark)
(In reply to Jared Hirsch [:_6a68] [@6a68] from comment #24)
> Hey Arthur,
> 
> That's odd. There is a known bug (bug 998464) around setting a timer to
> dismiss the 'Connecting...' overlay, but it only seems to happen when you
> lose network connectivity *after* starting the signup flow. Do you think
> that might be what happened in your case?
> 
> No-Jun, would you mind trying to repro this bug today? I'll try as well.

This is not reproducible in 2.0, but looks like there is a gecko bug causing the issue:
Bug 1027874
Flags: needinfo?(npark)
Hi Arthur,

Would you mind taking another shot at verifying this fix?

Gecko bug 1027874 has a fix, but hasn't landed in m-c yet. For now, the pvt build from the previous day (20140618040513) works correctly for me.

Thanks!

Jared
Flags: needinfo?(arthur.chen)
Comment on attachment 8440188 [details] [review]
Github PR 20517

I think the patch is good but it only works on v2.0 but not on master, and that's why I'm not able to give r+ to this patch for now. I would prefer to test the patch with bug 1027874 landed in master. Could you request a review again when ready? Thank you!
Attachment #8440188 - Flags: review?(arthur.chen) → feedback+
Flags: needinfo?(arthur.chen)
Comment on attachment 8440188 [details] [review]
Github PR 20517

Hi Arthur,

The gecko bug 1027595 has been fixed, and this patch looks good to me when applied to today's build.

Would you mind taking another look?

Thanks,

Jared
Attachment #8440188 - Flags: review?(arthur.chen)
Comment on attachment 8440188 [details] [review]
Github PR 20517

Verified ok with the latest pvt build and the provided patch. Please squash the commits before merging, thanks!
Attachment #8440188 - Flags: review?(arthur.chen) → review+
Thanks Arthur!

Rebased and pushed, just waiting for Gaia to reopen[1] to merge this.

[1] https://groups.google.com/d/topic/mozilla.dev.gaia/taBuowY7MbE/discussion
Travis is green, except for a known marionette bug:

https://travis-ci.org/mozilla-b2g/gaia/builds/28361779

Merging to master.
doh, didn't actually click the green button earlier.

Master: https://github.com/mozilla-b2g/gaia/commit/aecb3041c71c5627aea30628ee82fc74e8af0d47
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S5 (4july)
Attached video verify.3gp
This issue has been successfully verified on Flame 2.1, 2.0
See attachment:verify.3gp
Reproducing rate: 0/5

flame2.1 new build:
Gaia-Rev        5655269098c7e82254e56933f1af05b4abe2a2f3
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/86608c9389b5
Build-ID        20141204001201
Version         34.0

Flame 2.0 versions:
Gaia-Rev        8d1e868864c8a8f1e037685f0656d1da70d08c06
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/ff1100ba2ab8
Build-ID        20141204000228

Version         32.0
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.