Following up on bug 1250531 for mobile. From IRC: > 10:24 <kitcambridge> nalexander: ack. when a user disconnects sync on Fennec, does it delete the client from the clients collection? > 10:24 <•nalexander> kitcambridge: I think we do not, or at least we don't try hard. > 10:24 <•nalexander> kitcambridge: the reason is that we don't want to try to get a token-server token, etc, after the user has disconnected. > 10:25 <•nalexander> kitcambridge: yeah, I don't think we implemented that! https://dxr.mozilla.org/mozilla-central/source/mobile/android/> services/src/main/java/org/mozilla/gecko/fxa/receivers/FxAccountDeletedService.java > 10:27 <•nalexander> kitcambridge: it's hard, for token caching/fetching reasons. So stale and duplicate mobile clients are likely to show up in the Synced Tabs list. FWIW, Fennec hides devices with the same name that haven't synced in a week (bug 1180287). I don't think we need to worry about the 3-week TTL, because Desktop now fetches the full clients collection for every sync. But we could implement the same de-duping logic. What do you think, Mark?

SGTM. I was about to suggest that assuming Fennec does a server logout (which should invalidate the session token) we could use FxA to determine the device doesn't exist - but sadly Fennec doesn't register the device yet (that's bug 1254640) so the duplicate detection you mention probably is worthwhile even if it ends up partially subsumed by future FxA device work.

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

Comment on attachment 8741871 [details] MozReview Request: Bug 1264498 - Hide duplicate remote Sync clients that haven't synced in a week. r?markh https://reviewboard.mozilla.org/r/46809/#review43983 ::: services/sync/modules/engines/clients.js:202 (Diff revision 1) > + let record = this._store._remoteClients[id]; > + if (!names.has(record.name)) { > + names.add(record.name); > + continue; > + } > + let modified = this._store._remoteClientTimestamps[id]; I'm not super-excited by this._remoteClientTimestamps, especially seeing we recently added this._incomingClients. Do you think we could change _incomingClients() to a map (or regular object that acts as a map) storing the ID as the key and the modified (or possibly the entire record) as the value? It has the correct locality (ie, it's really only used by processIncoming()) and means we don't need to correctly track those timestamps in places where we don't, in practice, care about them. ::: services/sync/modules/engines/clients.js:205 (Diff revision 1) > + continue; > + } > + let modified = this._store._remoteClientTimestamps[id]; > + let remoteAge = AsyncResource.serverTime - modified; > + if (remoteAge > STALE_CLIENT_REMOTE_AGE) { > + this._removeRemoteClient(id); I first thought we should consider taking the "os" and "type" fields into account - although it seems unlikely to happen in practice, if either of those 2 fields don't match they don't match the criteria for this bug. But OTOH, maybe they actually *should* - eg, if I rename my new OSX laptop to "Marks Laptop" without disconnecting my old Windows laptop with the same name, it does make sense that name should be de-duped too. It seems the risk of 2 discrete devices having identical names would be smaller than the possibility that this patch might help others in the scenario above. So yeah, I guess we should do it this way :) Can you please add a log.info() call here to indicate you de-duped, so we can determine this via the logs if people report a problem.

Comment on attachment 8741871 [details] MozReview Request: Bug 1264498 - Hide duplicate remote Sync clients that haven't synced in a week. r?markh Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46809/diff/1-2/

https://reviewboard.mozilla.org/r/46809/#review43983 > I'm not super-excited by this._remoteClientTimestamps, especially seeing we recently added this._incomingClients. Do you think we could change _incomingClients() to a map (or regular object that acts as a map) storing the ID as the key and the modified (or possibly the entire record) as the value? It has the correct locality (ie, it's really only used by processIncoming()) and means we don't need to correctly track those timestamps in places where we don't, in practice, care about them. Yes, good point! This cleans up the stale comparison logic, too, and isolates it to `_processIncoming`. Updated exactly as you suggested. > I first thought we should consider taking the "os" and "type" fields into account - although it seems unlikely to happen in practice, if either of those 2 fields don't match they don't match the criteria for this bug. > > But OTOH, maybe they actually *should* - eg, if I rename my new OSX laptop to "Marks Laptop" without disconnecting my old Windows laptop with the same name, it does make sense that name should be de-duped too. It seems the risk of 2 discrete devices having identical names would be smaller than the possibility that this patch might help others in the scenario above. > > So yeah, I guess we should do it this way :) Can you please add a log.info() call here to indicate you de-duped, so we can determine this via the logs if people report a problem. I definitely agree about user-specified names. There's a chance we might flag a default name as a false positive, but hostnames need to be unique within a network, and the device needs to have been idle for over a week. Also, since we don't remove the record from the server, the erroneous dupe will be restored if it ever syncs again.

Comment on attachment 8741871 [details] MozReview Request: Bug 1264498 - Hide duplicate remote Sync clients that haven't synced in a week. r?markh https://reviewboard.mozilla.org/r/46809/#review44347 Awesome, thanks.