Closed Bug 1395485 Opened 7 years ago Closed 6 years ago

Having accounts.firefox.com in history with an old UID will disconnect Sync as a thumbnail is generated or the URL is selected from the awesomebar

Categories

(Firefox :: Sync, defect, P3)

defect

Tracking

()

RESOLVED WONTFIX
Tracking Status
firefox57 --- wontfix

People

(Reporter: markh, Unassigned)

Details

(Whiteboard: [fxa])

from the WTF department...

STR:
* Have accounts.firefox.com as a "top site" such that it appears on about:newtab etc.
* Refresh the profile.
* After a few seconds the new profile is kicked out of Sync.

What happens is that the page thumbnailer loads accounts.firefox.com to create a thumbnail. The webchannel mechanism kicks in and we see a fxaccounts:fxa_status message followed by a fxaccounts:logout message, and we kick the user out of sync.

For context, the "refresh profile" command blindly copies signedInUser.json to the new profile.

In my test-case, the we respond to the fxa_status message with:
{"signedInUser":
  {"email":"xxxx@gmail.com",
  "sessionToken":"big_hex_value",
  "uid", "big_hex_value",
  "verified":true}
  },
  "capabilities":{"engines":["addresses"]}
}

and we immediate get:
{"command":"fxaccounts:logout","data":{"uid":"big_hex_value"},"messageId":null}

(where the UID matches in both cases, hence we process the logout.)

Shane, any clues why we kick the user out in that case?
Flags: needinfo?(stomlinson)
Oh... I have almost no idea what's happening here. I'll have a look first thing in the morning.
Flags: needinfo?(stomlinson)
Assignee: nobody → stomlinson
Whiteboard: [fxa]
This doesn't rely on "profile reset" at all - any time the thumbnail is updated will kick us out of Sync.

STR:
* Ensure that accounts.firefox.com is a top site that is shown on about:new-tab
* Exit Firefox and delete the profile directory with thumbnails (and note that on Windows it is in the "AppData\Local" tree, not AppData\Roaming")
* Start the browser and wait a few seconds for thumbnails to be rebuilt.
* Note you are kicked out of Sync.
Summary: Having accounts.firefox.com as a "top site" will cause "reset profile" to disconnect sync → Having accounts.firefox.com as a "top site" will disconnect Sync as a thumnail is generated.
Summary: Having accounts.firefox.com as a "top site" will disconnect Sync as a thumnail is generated. → Having accounts.firefox.com as a "top site" will disconnect Sync as a thumbnail is generated.
markh: on OSX, so far I have been unable to reproduce. I'm not sure where the thumbnails are stored on OSX, I don't see anything obvious in my profile directory.

I have been able to force a thumbnail regeneration without a problem though.

The STRs I used to force thumbnail regeneration:

1. With a new profile. Sign into FxA.
2. Open about:newtab. If accounts.firefox.com is in the top sites, remove it saying "yes" to remove all items from history.
3. Open about:preferences#sync, click Manage Account.
4. Wait for the settings page to load. Close the tab.
5. Exit and restart the browser to the same profile. accounts.firefox.com should be in the top sites. It's thumbnail starts out as an A. It then changes to a blank page. I'm still signed into Sync though.

Could you have been signing in as two different users, the user stored in the thumbnail a different user to the currently signed in user, or with an expired sessionToken?
Flags: needinfo?(markh)
(In reply to Shane Tomlinson [:stomlinson] from comment #3)
> markh: on OSX, so far I have been unable to reproduce. I'm not sure where
> the thumbnails are stored on OSX, I don't see anything obvious in my profile
> directory.

As we discussed on IRC, I can repro on Mac using the exact same STR as in comment 2 - although on Mac it's the profile directory under /Caches/ rather than under /Application Support/ where the "thumbnails" folder lives.

> 1. With a new profile. Sign into FxA.
> 2. Open about:newtab. If accounts.firefox.com is in the top sites, remove it
> saying "yes" to remove all items from history.

I think this is where you went wrong. Once the thumbnail appears, quit the browser and delete all files from the profile's "thumbnails" directory.

> 3. Open about:preferences#sync, click Manage Account.

I believe in this case Firefox will take a thumbnail while accounts.firefox.com is open. This bug is only reproducible when the *background* thumbnailer opens the page in a hidden browser in a "private" context and takes the snapshot.

> Could you have been signing in as two different users, the user stored in
> the thumbnail a different user to the currently signed in user, or with an
> expired sessionToken?

Nope, definitely not. I can repro this 100% of the time immediately after a fresh login.

Note also that the thumbnails are considered stale after 2 days, which might cause this to be hit far more often than we expect, and is probably the root cause of :rfeeley and :adavis seeing anecdotal reports of users being silently kicked out of sync. I think the one saving grace we have is that most users probably will not visit accounts.firefox.com enough to have it appear as a "top site".

Adding the needinfo back to ensure you can reproduce it with the detail above.
Flags: needinfo?(markh) → needinfo?(stomlinson)
:markh - Try as I might, I haven't been able to reproduce even with the thumbnail generated by about:newtab. I tried against both prod and local dev. Are you able to take a video capture, perhaps using licecap [1] or Quicktime on a Mac? Sometimes seeing the exact STRs in video provides some clue that's hard to capture/easy to overlook in text.

[1] - http://www.cockos.com/licecap/
Flags: needinfo?(stomlinson) → needinfo?(markh)
After lots of trying, and Mark's tips in IRC, I have been able to reproduce! But, my STRs require two users.

First, apply this patch to mozilla-central to get more debug info: [1]

STRs


1. Sign in to Sync as user A.
2. In about:preferences#sync, click "Manage account"
3. Once FxA is open, copy the URL, open it in a new tab.
4. Quit Firefox, re-open. In about:newtab, ensure the settings page along with the uid of user A is a "top site"
5. Open "about:preferences#sync" again, click "Disconnect"
6. Sign in as user B.
7. Quit.
8. Delete all the about:newtab thumbnails
9. Open Firefox again.
10. Watch for the signout in the console.

Now, when I did this, the thumbnail is generated by opening the /settings page with the uid for user A. The problem is user B is signed in. In [2], we see this discrepancy and sign out the user, forcing them to sign in as user B. We did this in [3]

I suspect we did [3] to rely on [4] to send the user to the /force_auth page. Since no user will be signed in and email is specified on the URL, the user will be sent to /force_auth. I also vaguely remember it being possible to sign in on browser A as user A, and then go to browser B, delete user A's account, and sign up with the same email address. When the user goes back to browser A and opens /settings, the uid associated with the email will have changed. We need to send the logout message to the browser so that it can update the uid[5]. There is also [6].

We could just redirect the user to /force_auth directly if they aren't currently signed in. No logout message will be sent to the browser though. 

:markh - in this scenario, what would happen if we send an fxaccounts:login message with a different uid w/o first sending an fxaccounts:logout?


[1] - https://irccloud.mozilla.com/file/pJKAitVC/more-debug-info.patch
[2] - https://github.com/mozilla/fxa-content-server/blob/27f33317e787b3f48085277cddb939221e98e6fa/app/scripts/views/settings.js#L64:L80
[3] - https://github.com/mozilla/fxa-content-server/blob/27f33317e787b3f48085277cddb939221e98e6fa/app/scripts/views/settings.js#L79
[4] - https://github.com/mozilla/fxa-content-server/blob/27f33317e787b3f48085277cddb939221e98e6fa/app/scripts/views/base.js#L308
[5] - https://github.com/mozilla/fxa-content-server/pull/3419
[6] - https://github.com/mozilla/fxa-content-server/issues/1686
(In reply to Shane Tomlinson [:stomlinson] from comment #6)
> After lots of trying, and Mark's tips in IRC, I have been able to reproduce!
> But, my STRs require two users.

It turns out mine does too - my apologies for not picking up on that earlier. In my case the 2nd user had the same email address but a different UID.

So I think we can safely treat this as a bit of an edge-case and unlikely to be hit by most users. This also probably explains why it tended to be seen in Ryan's sync-fests, where user-swapping is the order of the day.

However, I guess it's also worth calling out that another vector for this will be the awesomebar - a user starts typing "accounts.", notices the completion and selects it - but if it is for a different user they'll end up in the same boat.

> I suspect we did [3] to rely on [4] to send the user to the /force_auth
> page. Since no user will be signed in and email is specified on the URL, the
> user will be sent to /force_auth.

That does seem like a large hammer though - would it be possible to delay the forced logout until the user completes the force_auth?

IOW, in your scenario, where 2 different email addresses are involved, it seems quite likely the user would notice the address is wrong and just close the tab or otherwise attempt to abort the flow.

> I also vaguely remember it being possible
> to sign in on browser A as user A, and then go to browser B, delete user A's
> account, and sign up with the same email address. When the user goes back to
> browser A and opens /settings, the uid associated with the email will have
> changed.

That's roughly what I did - deleted and recreated the user - but the old UID persisted in history.

> We need to send the logout message to the browser so that it can
> update the uid[5]. There is also [6].

Although the browser may already have the correct UID, as it did in my case. It's just the history entry which had the stale version.

> We could just redirect the user to /force_auth directly if they aren't
> currently signed in. No logout message will be sent to the browser though. 
> 
> :markh - in this scenario, what would happen if we send an fxaccounts:login
> message with a different uid w/o first sending an fxaccounts:logout?

We want the logout so it can reset sync and the current implementation appears likely to get upset if we see a login message when already logged in - although I guess we could change chat.

But wouldn't it be possible to still send the logout, but only when we actually have the new credentials ready? That gives the user the ability to abort the flow when they see the wrong thing happening and would seem to also solve this case when the user doesn't see anything at all (ie, the thumbnail case)

Either way, this is no where near as bad as I suspected. Thanks for helping to track this down.
Flags: needinfo?(markh)
Summary: Having accounts.firefox.com as a "top site" will disconnect Sync as a thumbnail is generated. → Having accounts.firefox.com in history with an old UID will disconnect Sync as a thumbnail is generated or the URL is selected from the awesomebar
So I kept wondering why I could continue to reproduce this when testing profile resets - I took care to kill my history etc. Long story short, my sync account is actually syncing the accounts.firefox.com visit with the bad uid :/ That doesn't change anything we've already said above, but I just needed to rant :)
(In reply to Mark Hammond [:markh] from comment #8)
> Long story short, my
> sync account is actually syncing the accounts.firefox.com visit with the bad
> uid :/

This made me chuckle. Sync working as Sync should then?
Priority: -- → P3
Hi Shane! Are you still planning to work on this bug, or should we put it back into our backlog?
Flags: needinfo?(stomlinson)
(In reply to Kit Cambridge (he/him) [:kitcambridge] (UTC-7) from comment #10)
> Hi Shane! Are you still planning to work on this bug, or should we put it
> back into our backlog?

Backlog it! I have about 0 extra time, and getting into this state is unlikely in the wild (how long until I have to eat these words?).
Flags: needinfo?(stomlinson)
Sounds good, thanks!
Assignee: stomlinson → nobody
from mtg: unlikely in the wild, closing for now.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.