1403276 - One device shows up twice in "Send to device" menu

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P2)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Screenshot: send to device

I see "mia" twice in my send to device menu but it only appears once in my manage sync account screen: see the attached screenshots.

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Screenshot: devices in account preferences

I'm running macOS Sierra on a Nightly only a few days old.

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Note that when I send tabs to mia, I send it to each of the devices but I only ever receive the tab once.

Comment 3

[Lina Butler (ex-Mozilla)]

Thanks for the report! Would you mind inspecting `Weave.Service.clientsEngine.remoteClients` and `Weave.Service.clientsEngine._knownStaleFxADeviceIds` in your browser console, please?

In particular, I'm curious about the `fxaDeviceId` and `serverLastModified` for your two mias, and if either one shows up in `_knownStaleFxADeviceIds`.

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Kit Cambridge (he/him) [:kitcambridge] (UTC-7) from comment #3)
> In particular, I'm curious about the `fxaDeviceId` and `serverLastModified`
> for your two mias, and if either one shows up in `_knownStaleFxADeviceIds`.

_knownStaleFxADeviceIds is empty (note: before taking those screenshots, I did prune out all the devices I haven't been using from my account) so no, the fxaDeviceId's don't appear there.

Comment 5

[Edouard Oger [:eoger]]

Do you see duplicates in Weave.Service.clientsEngine.remoteClients? Do these duplicates all have an fxaDeviceId?

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Edouard Oger [:eoger] from comment #5)
> Do you see duplicates in Weave.Service.clientsEngine.remoteClients? Do these
> duplicates all have an fxaDeviceId?

I see two entries with the name, "mia":
- They have the same `fxaDeviceId`
- They have different `id`s, `version`s (57.0a1 and 58.0a1), `serverLastModified`, and `commands` (one has 8 items, the other has 1 item)

I run Nightly on that machine.

Comment 7

[Edouard Oger [:eoger]]

So mcomella told me on IRC there's a chance he refreshed his profile at some point in the past, but it shouldn't matter since signedInUser.json is kept during refresh anyway.
I think for this bug we could remove fxadeviceid duplicates, based on lastModified.

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

It seems I did a refresh on Sept. 9th: there's an "Old Firefox Data" folder on the desktop created on that date.

Comment 9

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

(In reply to Edouard Oger [:eoger] from comment #7)
> So mcomella told me on IRC there's a chance he refreshed his profile at some
> point in the past, but it shouldn't matter since signedInUser.json is kept
> during refresh anyway.

That's interesting - I'm guessing that somehow we got a new session ID.

(In reply to Michael Comella (:mcomella) from comment #8)
> It seems I did a refresh on Sept. 9th: there's an "Old Firefox Data" folder
> on the desktop created on that date.

That's before bug 1395332 landed, so what *might* have happened is that after the refresh we didn't quite do the right thing, so Michael manually reconnected to sync or something, which ended up reusing the device ID but got a new session token. I suspect that since but 1395332 we'd reuse both tokens (so probably wouldn't see this)

> I think for this bug we could remove fxadeviceid duplicates, based on lastModified.

cc rfkelly for his thoughts on this ...

Comment 10

[Ryan Kelly [:rfkelly]]

So this is two sync client records with the same FxA device ID?  Deduping based on lastModified sounds reasonable to me.

Comment 11

[Thom Chiovoloni [:tcsc] (ex-moco)]

Attachment: Bug 1403276 - Dedupe sync devices with the same fxaDeviceId by picking the one with the newer last modified date

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

Comment 12

[Edouard Oger [:eoger]]

Comment on attachment 8917069 [details]
Bug 1403276 - Dedupe sync devices with the same fxaDeviceId by picking the one with the newer last modified date

https://reviewboard.mozilla.org/r/188076/#review193270

Looks great thank you.

::: commit-message-f8a33:1
(Diff revision 1)
> +Bug 1403276 - Dedupe sync devices with the same fxaDeviceId by picking the one with the newer last modified date r?eoger

That's a long commit message :)

::: services/sync/modules/engines/clients.js:320
(Diff revision 1)
> +    // we've seen before, we can mark it stale immediately.
> +    let clientList = Object.values(this._store._remoteClients).sort((a, b) =>
> +      b.serverLastModified - a.serverLastModified);
> +    let seenDeviceIds = new Set([localFxADeviceId]);
> +    for (let client of clientList) {
> +      // iOS clients don't have an `fxaDeviceId`

I don't think this comment is accurate now, however some clients might fail the FxA registration process so this condition still makes sense.

Comment 13

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8917069 [details]
Bug 1403276 - Dedupe sync devices with the same fxaDeviceId by picking the one with the newer last modified date

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/95f96e06d51f
Dedupe sync devices with the same fxaDeviceId by picking the one with the newer last modified date r=eoger

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/95f96e06d51f