If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

De-dupe stale mobile clients

RESOLVED FIXED in Firefox 48

Status

()

Firefox
Sync
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: kitcambridge, Assigned: kitcambridge)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 48
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: [sync-data-integrity])

MozReview Requests

()

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

Attachments

(1 attachment)

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.
Assignee: nobody → kcambridge
Status: NEW → ASSIGNED
Flags: firefox-backlog+
See Also: → bug 1250866
See Also: → bug 1250531
Created attachment 8741871 [details]
MozReview Request: Bug 1264498 - Hide duplicate remote Sync clients that haven't synced in a week. r?markh

Review commit: https://reviewboard.mozilla.org/r/46809/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46809/
Attachment #8741871 - Flags: review?(markh)

Updated

2 years ago
Blocks: 1257961
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.
Attachment #8741871 - Flags: review?(markh)
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/
Attachment #8741871 - Flags: review?(markh)
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.
Attachment #8741871 - Flags: review?(markh) → review+
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.

Comment 7

a year ago
https://hg.mozilla.org/integration/fx-team/rev/fe77c894cb9a

Comment 8

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

Updated

a year ago
Whiteboard: [sync-data-integrity]
Depends on: 1287687
See Also: → bug 1238871
You need to log in before you can comment on or make changes to this bug.