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

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

[Alex Davis [:adavis]]

Attachment: 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.

Comment 1

[Alex Davis [:adavis]]

Attachment: device-app-manager.png

We see that Firefox Beta for iOS is not listed in my devices.

Comment 2

[Ryan Kelly [:rfkelly]]

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?

Comment 3

[Ryan Kelly [:rfkelly]]

> 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?

Comment 4

[Lina Butler [:lina]]

(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".

Comment 5

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

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)

Comment 6

[Ryan Kelly [:rfkelly]]

> 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.

Comment 7

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

(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?)

Comment 8

[Ryan Kelly [:rfkelly]]

> 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.

Comment 9

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

(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).

Comment 10

[Alex Davis [:adavis]]

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

Comment 11

[Edouard Oger [:eoger]]

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 12

[Edouard Oger [:eoger]]

Attachment: Bug 1339861 - Mark Sync clients as stale if not in the FxA device list.

Review commit: https://reviewboard.mozilla.org/r/116084/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/116084/

Comment 13

[Edouard Oger [:eoger]]

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 :)

Comment 14

[Edouard Oger [:eoger]]

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 :(

Comment 15

[Edouard Oger [:eoger]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116084/diff/1-2/

Comment 16

[Edouard Oger [:eoger]]

Done, I dropped the push thing too.

Comment 17

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

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?

Comment 18

[Edouard Oger [:eoger]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116084/diff/2-3/

Comment 19

[Edouard Oger [:eoger]]

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

Comment 20

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

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.

Comment 21

[Edouard Oger [:eoger]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116084/diff/3-4/

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b4cca954cbb3
Mark Sync clients as stale if not in the FxA device list. r=markh

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b4cca954cbb3