Send Tab: Device List shows devices disconnected from FxA but not yet expired by Sync

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Sync
P1
normal
RESOLVED FIXED
a year ago
9 months ago

People

(Reporter: adavis, Assigned: eoger)

Tracking

unspecified
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

Created attachment 8837684 [details]
Screen Shot 2017-02-15 at 9.12.47 AM.png

The device list in Send Tab is not accurate. I'm on Dev Edition (53) but have observed the same problem on Beta (52).

1) There is a device called "Send Tab" which is actually Firefox release for iOS. So the name here is no good.

2) Firefox Beta is in my list (see screenshot) but that browser no longer exists. We can see that through my device and app manager view that it is no longer connected to my account. In fact, it's not even installed anywhere anymore.
(Reporter)

Comment 1

a year ago
Created attachment 8837686 [details]
device-app-manager.png

We see that Firefox Beta for iOS is not listed in my devices.
I know we have some concern around *duplicate* device records appearing in the FxA device list, but IIUC we can be pretty confident that if a device does *not* appear in that list, then it's no longer connected to the account.  :markh, perhaps we can figure out a way to use the FxA device list to filter the sync clients list, as a first step towards unifying them completely?
> There is a device called "Send Tab" which is actually Firefox release for iOS.

I remember hearing about this before, it may be an existing bug, or even fixed in the pending release.  Grisha, does this ring any bells?
Flags: needinfo?(gkruglov)
(In reply to Ryan Kelly [:rfkelly] from comment #3)
> I remember hearing about this before, it may be an existing bug, or even
> fixed in the pending release.  Grisha, does this ring any bells?

There was a mailing list thread about this a couple of weeks ago. Quoting rnewman:

> When we upload a client record, we do so by asking the app bundle what
> the app's name is — "Firefox" or "Nightly" — and what the device name
> is — "Ryan's iPhone" — and then pushing those into a template string.
> That's the client name: "Firefox on Ryan's iPhone". We save that in
> prefs for next time.
> 
> What I think happened in your case is that the first piece of code
> to sync on your device was the Send Tab extension. Normally a user
> sets up Sync in the main Firefox app, and it syncs, so you'd see
> "Firefox" there. It seems that in an app extension that the extension
> has its own name, and its own bundle, so the app name we got was
> "Send Tab".
I split the "iOS device is called 'Send Tab' into bug 1339976, and we'll keep this for the "stale devices" part.

Alex confirmed in IRC that the now disconnected device was disconnected less than 21 days ago, so is yet to "expire" from Sync itself.

As Ryan mentions, a reasonable solution might be for a client engine sync to query all known FxA devices, and any which aren't known to FxA should be treated as "stale". This would be an additional FxA request per Sync, but hopefully that's OK. I doubt we want the latency that querying the device list as the menu opens would imply (even if that's actually possible, which it might not be given how menus are implemented)
Component: Firefox Sync: Cross-client → Sync
Flags: needinfo?(gkruglov)
Product: Cloud Services → Firefox
Summary: Send Tab: Device List Not Accurate → Send Tab: Device List shows devices disconnected from FxA but not yet expired by Sync
Target Milestone: mozilla53 → ---
> This would be an additional FxA request per Sync, but hopefully that's OK

In theory, you get a push notification for any action that would cause the contents of the FxA device list to change, so it may be possible to avoid a per-sync request.
(In reply to Ryan Kelly [:rfkelly] from comment #6)
> > This would be an additional FxA request per Sync, but hopefully that's OK
> 
> In theory, you get a push notification for any action that would cause the
> contents of the FxA device list to change, so it may be possible to avoid a
> per-sync request.

Ah yes, good point. So maybe we just query the list on the first sync for a browser session, then have the push message update that list. If push fails, the worst case is that the list has stale entries until the next restart.

This might be a bit of a tangent, but IIRC we did have some discussion in the past about an explicit request to update the current device registration so that the "last used" field is more likely to be accurate. I wonder if it is worth trying to kill 2 birds with one stone here (ie, a single request that both confirms the current device is in use and also gets a list of all other devices as the response?)
> a single request that both confirms the current device is in use and also gets
> a list of all other devices as the response?

I'm slightly scared by the increased DB traffic this would generate, but happy to have the conversation if it's going to deliver value to the overall sync experience :-)

This would undo a lot of layers of caching that are currently in place to reduce the per-sync network traffic, e.g. the signed certificate to avoid hitting FxA, and the signed token to avoid hitting tokenserver on every sync.
(In reply to Ryan Kelly [:rfkelly] from comment #8)
> I'm slightly scared by the increased DB traffic this would generate, but
> happy to have the conversation if it's going to deliver value to the overall
> sync experience :-)

The main value I see is allowing that "last used" field to be accurate and therefore shown, but that sounds non-trivial, so I think we should just stick with "plan a" (ie, ignore that tangent, and keep this functionality in the clients engine rather than trying to enhance FxA in this way at this time).
(Reporter)

Comment 10

a year ago
Thanks guys for the quick turn around here. The proposed solution around updating when a push notification is received makes sense.
(Assignee)

Updated

a year ago
Assignee: nobody → eoger
Status: NEW → ASSIGNED
Priority: -- → P1
(Assignee)

Comment 11

a year ago
Just a observation: we only send fxaccounts:device_disconnected to the device we want to disconnect, not to everyone. That means we would not update the list on other clients since we rely on push.
We can either change fxaccounts:device_disconnected to be sent to everyone (we send the ID to disconnect anyway), or we could send along a new general fxaccounts:device_list_updated notification to everyone, every time something changes in the list.
Comment hidden (mozreview-request)
(Assignee)

Comment 13

a year ago
Comment on attachment 8842174 [details]
Bug 1339861 - Mark Sync clients as stale if not in the FxA device list.

This is not ready yet per comment 11, but we're almost there :)
Attachment #8842174 - Flags: review?(markh) → feedback?(markh)
(Assignee)

Comment 14

a year ago
Comment on attachment 8842174 [details]
Bug 1339861 - Mark Sync clients as stale if not in the FxA device list.

From meeting with markh: instead of storing a list of non stale devices, let's store a list of known stale devices.
Why? If a device connects and we don't receive the push notification telling us to refresh our FxA list, we will mark the new device as stale on next sync :(
Attachment #8842174 - Flags: feedback?(markh)
Comment hidden (mozreview-request)
(Assignee)

Comment 16

a year ago
Done, I dropped the push thing too.

Comment 17

a year ago
mozreview-review
Comment on attachment 8842174 [details]
Bug 1339861 - Mark Sync clients as stale if not in the FxA device list.

https://reviewboard.mozilla.org/r/116084/#review119504

Thanks for the tests :) This is looking good but I've a few nits below...

::: services/fxaccounts/FxAccounts.jsm:685
(Diff revision 2)
> +      .then(data => {
> +        if (data && data.sessionToken) {
> +          return this.fxAccountsClient.getDeviceList(data.sessionToken);
> +        }
> +
> +        // Without a signed-in user, there can be no device list.

I'd be inclined to throw here - I don't really care, but it smells a little like a bug to try and fetch a device list when there's no signed in user. (You could probably argue the same above re the device ID, but that's a local ID) - eg, I believe we'd throw trying to fetch the profile.  Calling `_getVerifiedAccountOrReject()` would  do the right thing (I think that's where a profile fetch would fail grabbing tokens for the request)

Also, using async functions would be fine (and probably preferred moving forward in most cases) - we already use Task.jsm in a few places and that has the same stack mis-fortune as async functions) - I don't care if you re-write this or not - it's more a general comment ;)

::: services/sync/modules/engines/clients.js:296
(Diff revision 2)
>      const allCommands = this._readCommands();
>      delete allCommands[clientId];
>      this._saveCommands(allCommands);
>    },
>  
> -  _syncStartup: function _syncStartup() {
> +  _refreshKnownStaleClients() {

A brief comment saying why this exists would be good (eg, "we assume clients without a current FxA device ID have been disconnected and so are stale" or similar.

::: services/sync/modules/engines/clients.js:297
(Diff revision 2)
>      delete allCommands[clientId];
>      this._saveCommands(allCommands);
>    },
>  
> -  _syncStartup: function _syncStartup() {
> +  _refreshKnownStaleClients() {
> +    this._log.info('Refreshing the known stale clients list');

this should probably be .debug

::: services/sync/modules/engines/clients.js:298
(Diff revision 2)
>      this._saveCommands(allCommands);
>    },
>  
> -  _syncStartup: function _syncStartup() {
> +  _refreshKnownStaleClients() {
> +    this._log.info('Refreshing the known stale clients list');
> +    let localClients = Object.values(this._store._remoteClients).map(client => client.fxaDeviceId);

I *think* iOS doesn't include this field? Android does on beta, so I think that's probably OK, but if iOS doesn't, we probably want to assume records without an fxaDeviceId are OK?

::: services/sync/modules/engines/clients.js:302
(Diff revision 2)
> +    this._log.info('Refreshing the known stale clients list');
> +    let localClients = Object.values(this._store._remoteClients).map(client => client.fxaDeviceId);
> +    let fxaClients;
> +    try {
> +      fxaClients = Async.promiseSpinningly(this.fxAccounts.getDeviceList()).map(device => device.id);
> +    } catch (onlyInTests) {

This might also fail on network error (or service outage), so we should log.error() here, and assume that all devices are good - IIUC, every device will currently be considered stale in that case.

::: services/sync/modules/engines/clients.js:305
(Diff revision 2)
> +    try {
> +      fxaClients = Async.promiseSpinningly(this.fxAccounts.getDeviceList()).map(device => device.id);
> +    } catch (onlyInTests) {
> +      fxaClients = [];
> +    }
> +    this._knownStaleClients = Utils.arraySub(localClients, fxaClients);

I think this should hold the sync IDs of the stale clients rather than the fxa ID (or the name of the variable should be changed).

::: services/sync/modules/engines/clients.js:349
(Diff revision 2)
>        let names = new Set([this.localName]);
>        for (let [id, serverLastModified] of Object.entries(this._incomingClients)) {
>          let record = this._store._remoteClients[id];
>          // stash the server last-modified time on the record.
>          record.serverLastModified = serverLastModified;
> +        if (this._knownStaleClients.includes(record.fxaDeviceId)) {

if the var held the local client ID records you could skip handling the fact fxaDeviceId doesn't exist for iOS? If you stick with FxA device IDs, you probably need to assume a missing ID is OK like mentioned above?
Attachment #8842174 - Flags: review?(markh)
Comment hidden (mozreview-request)
(Assignee)

Comment 19

a year ago
Comments addressed, I also kept the list of fxaDeviceId because I like the simplicity of just doing an arraySub.

Comment 20

a year ago
mozreview-review
Comment on attachment 8842174 [details]
Bug 1339861 - Mark Sync clients as stale if not in the FxA device list.

https://reviewboard.mozilla.org/r/116084/#review120350

Looks great, thanks!

::: services/sync/modules/engines/clients.js:356
(Diff revision 3)
>        for (let [id, serverLastModified] of Object.entries(this._incomingClients)) {
>          let record = this._store._remoteClients[id];
>          // stash the server last-modified time on the record.
>          record.serverLastModified = serverLastModified;
> +        if (record.fxaDeviceId && this._knownStaleFxADeviceIds.includes(record.fxaDeviceId)) {
> +          this._log.info(`Hiding stale client ${id} in known stale clients list`);

nit: this reads like you are hiding the client in the stale list - something like `Hiding stale client ${id} - in known stale clients list` would be better.
Attachment #8842174 - Flags: review?(markh) → review+
Comment hidden (mozreview-request)

Comment 22

a year ago
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b4cca954cbb3
Mark Sync clients as stale if not in the FxA device list. r=markh

Comment 23

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b4cca954cbb3
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Updated

9 months ago
Depends on: 1367525

Updated

9 months ago
Depends on: 1367533
You need to log in before you can comment on or make changes to this bug.