1248599 - [e10s] The user is not automatically signed into the Firefox account after resetting the password

Status: VERIFIED FIXED

(Cloud Services :: Server: Firefox Accounts, defect)

Description

[Vasilica Tamas, QA [:vtamas]]

[Affected versions]:
Firefox 45 Beta 3 with e10s enabled
Firefox 46.0a2 (2016-02-16)
Firefox 47.0a1 (2016-02-16)

[Affected platforms]:
Ubuntu 12.04 64-bit
Mac 10.10.5
Windows 10 32-bit


[Steps to reproduce]:
1.Launch Firefox with clean profile.
2.Navigate to about:preferences#sync
3.Click “Sign In” button and insert a valid email address.
4.Select “Forgot password?” link.
5.Click “Begin reset” button.
6.Check the email and click on “Reset password now” link.
7.Insert the new password twice and click “Next” button.


[Expected Results]:
The user is automatically logged into the Firefox account and no issues are encountered.


[Actual Results]:
[1] The user is not automatically signed into the Firefox account.
[2] The Firefox account appears as unverified.
[3] Clicking on “Verify Email” displays an “Unable to Send Verification” error: http://i.imgur.com/ZupwHts.png
[4] The following error is thrown in Browser Console:
Error: Can't add a login with a null username. nsLoginManager.js:276:13


[Additional notes]:
- This issue only reproduces with e10s enabled.
- I am marking Firefox 44 as unaffected since e10s cannot be enabled for Firefox 44.

Comment 1

[Chris Karlof [:ckarlof]]

Ryan, I feel this should be in the FxA backlog.

Comment 2

[Ryan Kelly [:rfkelly]]

> This issue only reproduces with e10s enabled.

Aye, and high priority because e10s-related

Comment 3

[Ryan Kelly [:rfkelly]]

I was able to reproduce this on Nightly.  I suspect the browser is somehow using the wrong sessionToken, as my console window is full of messages like:

1455655694051	FirefoxAccounts	ERROR	error GETing /recovery_email/status: {"code":401,"errno":110,"error":"Unauthorized","message":"Invalid authentication token in request signature","info":"https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md#response-format"}
1455655748303	FirefoxAccounts	ERROR	error POSTing /account/device: {"code":401,"errno":110,"error":"Unauthorized","message":"Invalid authentication token in request signature","info":"https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md#response-format"}
1455655748303	FirefoxAccounts	ERROR	error POSTing /account/device: {"code":401,"errno":110,"error":"Unauthorized","message":"Invalid authentication token in request signature","info":"https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md#response-format"}

I'll see if I can capture a full debug log from when this happens.

Comment 4

[Ryan Kelly [:rfkelly]]

(Also if I try to resend the verification email from the web, I get the "Attempt limit exceeded" error message)

Comment 5

[Ryan Kelly [:rfkelly]]

Attachment: console_output.txt

I didn't get an about:sync-log log, but attaching the full console output which has some nice tracebacks and looks very much like we failed to store the new sessionToken correctly.

Comment 6

[Ryan Kelly [:rfkelly]]

The flow involved here requires the reset-password tab to tunnel some state back to the other tab:

 https://github.com/mozilla/fxa-content-server/blob/master/app/scripts/views/complete_reset_password.js#L100

I wonder if the extra separation provided by e10s is causing this to fail.

Comment 7

[Ryan Kelly [:rfkelly]]

For completeness, I can confirm that disabling e10s in nightly makes the login flow work as expected.

Comment 8

[Vlad Filippov (:vladikoff)]

Attachment: non-e10s.png

non-e10s login message after password reset

Comment 9

[Vlad Filippov (:vladikoff)]

Attachment: e10s-login-reset.png

Comment 10

[Vlad Filippov (:vladikoff)]

Mike, Ryan, I attached the message we get from the LOGIN event after password reset , see 2 images above. 

Ryan, it is strange to see some of the information go through and some of it is `undefined`.

Comment 11

[Vlad Filippov (:vladikoff)]

Attachment: unsafe-cpow.png

And when I load the reset link I get a bunch of CPOW warnings.

Comment 12

[Vlad Filippov (:vladikoff)]

Mike, could this be the issue:

the state that was setup by about:accounts' localstorage is not accessible by the newly opened reset (e10s) page?

Comment 13

[Ryan Kelly [:rfkelly]]

> Ryan, it is strange to see some of the information go through and some of it is `undefined`.

IIUC, the fields that have values in the e10s version ("keyFetchToken" and "unwrapBKey") are acquired via a different mechanism to the ones that are undefined (e.g. "sessionToken"):

  https://github.com/mozilla/fxa-content-server/blob/master/app/scripts/views/confirm_reset_password.js#L151

Definitely makes it look like a localstorage issue.

Comment 14

[Ryan Kelly [:rfkelly]]

(Also, content-server should be erroring out rather than sending "fxaccount:login" messages with empty fields; maybe another opportunity to use data schemas on our messages)

Comment 15

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

IIUC, the problem is that localStorage set by accounts.firefox.com when loaded into a tab isn't available to the page when it lives in the iframe in about:accounts.  I believe this will be due to the iframe living in the parent process and the tab in the child. Bug 1191791 indicates localStorage doesn't work between content processes, so I expect it to also not work between the parent and child.

aboutAccounts.xhtml declares the iframe as |<iframe mozframetype="content" id="remote" />| so it looks like it *tries* to load it in the child. However, if I instrument aboutAccounts.js to dump |this.iframe.frameLoader.tabParent| it prints null. I think this confirms that iframe is in the parent. In addition, that code touches iframe.contentWindow in a couple of places, which I'd expect to be a CPOW - which it appears to not be - but that also means that if we *did* manage to get the iframe in the parent we'd need to rejig this and add content scripts.

I guess a reasonable first step would be to work out how to get that iframe in the parent, see if this problem goes away and also what other problems that introduces.

Comment 16

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

oops - "if we *did* manage to get the iframe in the parent" and "work out how to get that iframe in the parent" should say *child* instead of parent.

Comment 17

[Mike Conley (:mconley) (:⚙️)]

(In reply to Mark Hammond [:markh] from comment #15)
> I guess a reasonable first step would be to work out how to get that iframe
> in the parent, see if this problem goes away and also what other problems
> that introduces.

Unfortunately, making iframes remote is not a thing that is supported in Desktop by default. Only B2G is able to do this, using mozbrowser iframes.

I believe setting dom.mozBrowserFramesEnabled to true, putting a mozbrowser attribute on the iframe, and setting remote="true", will do the deed, but I suspect this is highly untested on Desktop, and feels risky.

Other options include:

1) Having the user flow for the "reset in the same browser session" case work the same as "reset in a different browser session", where the user needs to re-login after the reset has taken place. This is probably easiest, but is a degradation in terms of user experience, imo.

2) Make about:accounts run in the content process. This, I'm not sure how easy it will be.

3) Find an alternative mechanism besides localStorage for a tab in the same session to alert the about:accounts iframe that the password has been reset.

I'll be looking at 2 and 3 today.

Comment 18

[Ryan Kelly [:rfkelly]]

> Find an alternative mechanism besides localStorage for a tab in the same session to alert the
> about:accounts iframe that the password has been reset.

It already sends some information via BroadcastChannel that seems to work (the values that are not "undefined" in Vlad's e10s screenshot) so I wonder if we can move more towards that.

Comment 19

[Mike Conley (:mconley) (:⚙️)]

(In reply to Ryan Kelly [:rfkelly] from comment #18)
> > Find an alternative mechanism besides localStorage for a tab in the same session to alert the
> > about:accounts iframe that the password has been reset.
> 
> It already sends some information via BroadcastChannel that seems to work
> (the values that are not "undefined" in Vlad's e10s screenshot) so I wonder
> if we can move more towards that.

Yes, BroadcastChannel seems to work properly from process to process. Again, I'm not privy to how things occur under the hood for accounts here, but if it could be modified to use BroadcastChannel instead, that should address the issue.

vladikoff - I remember you saying that BroadcastChannel might not be the road we want to go down, but I don't remember why that was. Can you re-explain it?

Comment 20

[Ryan Kelly [:rfkelly]]

The comments at [1] indicate that we're relying on localstorage/broadcast/etc here because about:accounts only listens for a login message from the originating tab.  It's not obvious to me that this constrain still holds given that we can login from arbitrary tabs, first-run etc these days.  I wonder if the verification tab could send the message itself and save us the trouble.  Could be a big refactoring though... :-(

[1] https://github.com/mozilla/fxa-content-server/blob/master/app/scripts/views/confirm_reset_password.js#L97

Comment 21

[Shane Tomlinson [:stomlinson]]

(In reply to Ryan Kelly [:rfkelly] from comment #20)
> The comments at [1] indicate that we're relying on
> localstorage/broadcast/etc here because about:accounts only listens for a
> login message from the originating tab.  It's not obvious to me that this
> constrain still holds given that we can login from arbitrary tabs, first-run
> etc these days.  I wonder if the verification tab could send the message
> itself and save us the trouble.  Could be a big refactoring though... :-(
> 
> [1]
> https://github.com/mozilla/fxa-content-server/blob/master/app/scripts/views/
> confirm_reset_password.js#L97


Indeed, this constraint no longer appears to be the case. With the referenced patch [1] applied, the reset password flow works fine and dandy in today's Nightly set up to sync with a local dev stack.



[1] - https://gist.github.com/shane-tomlinson/725aa75d93f95ca0e6af

Comment 22

[Mike Conley (:mconley) (:⚙️)]

I've been looking at "2) Make about:accounts run in the content process." from comment 17.

I estimate this to be quite difficult. aboutaccounts.js does a number of privileged things and de-tangling / separating the privileged things out from the unprivileged would take time (and would be an ill-advised uplift, imo).

Am I correct that the problem is that we expect some kind of session token to exist in localStorage after the in-browser tab for password reset has opened and filled out properly?

Can we not send an additional notification (or just augment the existing notification) to set that same session token in the about:accounts iframe once the form has been submitted properly? It seems to me that this would work for both the e10s and non-e10s case until we can get about:accounts running remotely (which should be an eventuality).

Or am I misunderstanding how all of this works? Is my suggestion not viable / straight-forward?

Comment 23

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 46+ since we aim to ship e10s in 46.

Comment 24

[Mike Conley (:mconley) (:⚙️)]

Talked to vladikoff and stomlinson in #fxa, and I believe they're going to try to fix this on the FxA side of things in the short term.

Comment 25

[Mike Conley (:mconley) (:⚙️)]

stomlinson linked to a commit in IRC here: https://github.com/mozilla/fxa-content-server/pull/3525

Comment 26

[Vlad Filippov (:vladikoff)]

Fixed in https://github.com/mozilla/fxa-content-server/pull/3525

The fix will be deployed to production with fxa-content-server 0.56.2 or 0.57.0

Comment 27

[Mike Conley (:mconley) (:⚙️)]

Awesome vladikoff, thanks! Do you know when the deployment to production is next scheduled for?

Setting qawanted for verification once we know the deployment has gone out.

Comment 28

[Shane Tomlinson [:stomlinson]]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #27)
> Awesome vladikoff, thanks! Do you know when the deployment to production is
> next scheduled for?
> 
> Setting qawanted for verification once we know the deployment has gone out.


jrgm is looking at the diff and determining the risk level. I'll let you know once we find out more. ni myself and John so we remember to get back to you.

Comment 29

[John Morrison [:jrgm]]

I'm deploying this in the next few hours. I had to pick up an nginx config fix along with this change.

Comment 30

[John Morrison [:jrgm]]

The fix for this went to production 2016-02-23T17:36:00Z

Comment 31

[Mike Conley (:mconley) (:⚙️)]

Excellent - I just tested locally, and the STR from comment 0 no longer show the bug. Thanks everybody, I think we're done here!

Comment 32

[Liz Henry (:lizzard) (relman/hg->git project)]

Fixed on the server side, fabulous!

Comment 33

[Vasilica Tamas, QA [:vtamas]]

I also confirm that this bug is Verified fixed on Firefox 47.0a1 (2016-03-01/02), Firefox 46.0a2 (2016-03-01/02) and Firefox 45RC (20160301003640) with e10s enabled under Windows 10 64-bit, Ubuntu 12.04 32-bit and Mac OS X 10.11.