Closed Bug 1248599 Opened 6 years ago Closed 6 years ago

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

Categories

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

defect
Not set
normal

Tracking

(e10sm9+, firefox44 unaffected, firefox45 verified, firefox46+ verified, firefox47+ verified)

VERIFIED FIXED
Tracking Status
e10s m9+ ---
firefox44 --- unaffected
firefox45 --- verified
firefox46 + verified
firefox47 + verified

People

(Reporter: vtamas, Assigned: stomlinson)

References

()

Details

Attachments

(4 files)

[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.
Component: Sync → Server: Firefox Accounts
Product: Firefox → Cloud Services
Version: Trunk → unspecified
Ryan, I feel this should be in the FxA backlog.
> This issue only reproduces with e10s enabled.

Aye, and high priority because e10s-related
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.
(Also if I try to resend the verification email from the web, I get the "Attempt limit exceeded" error message)
Attached file 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.
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.
For completeness, I can confirm that disabling e10s in nightly makes the login flow work as expected.
Assignee: nobody → mconley
tracking-e10s: --- → m9+
Attached image non-e10s.png
non-e10s login message after password reset
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`.
Attached image unsafe-cpow.png
And when I load the reset link I get a bunch of CPOW warnings.
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?
> 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.
(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)
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.
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.
(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.
> 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.
(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?
Flags: needinfo?(vlad)
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
(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
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?
Tracking for 46+ since we aim to ship e10s in 46.
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.
Assignee: mconley → vlad
Flags: needinfo?(vlad)
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
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
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.
Flags: needinfo?(vlad)
Keywords: qawanted
(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.
Assignee: vlad → stomlinson
Flags: needinfo?(vlad)
Flags: needinfo?(stomlinson)
Flags: needinfo?(jrgm)
Depends on: 1250236
I'm deploying this in the next few hours. I had to pick up an nginx config fix along with this change.
Flags: needinfo?(jrgm)
The fix for this went to production 2016-02-23T17:36:00Z
Excellent - I just tested locally, and the STR from comment 0 no longer show the bug. Thanks everybody, I think we're done here!
Status: RESOLVED → VERIFIED
Flags: needinfo?(stomlinson)
Flags: qe-verify+
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.
You need to log in before you can comment on or make changes to this bug.