Closed Bug 1403276 Opened 3 years ago Closed 3 years ago

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

Categories

(Firefox :: Sync, defect, P2)

defect

Tracking

()

RESOLVED FIXED
Firefox 58
Tracking Status
firefox58 --- fixed

People

(Reporter: mcomella, Assigned: tcsc)

Details

Attachments

(3 files)

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.
I'm running macOS Sierra on a Nightly only a few days old.
Note that when I send tabs to mia, I send it to each of the devices but I only ever receive the tab once.
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`.
Flags: needinfo?(michael.l.comella)
(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.
Flags: needinfo?(michael.l.comella)
Do you see duplicates in Weave.Service.clientsEngine.remoteClients? Do these duplicates all have an fxaDeviceId?
(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.
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.
It seems I did a refresh on Sept. 9th: there's an "Old Firefox Data" folder on the desktop created on that date.
(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 ...
So this is two sync client records with the same FxA device ID?  Deduping based on lastModified sounds reasonable to me.
Priority: -- → P2
Assignee: nobody → tchiovoloni
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.
Attachment #8917069 - Flags: review?(eoger) → review+
Pushed by tchiovoloni@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/95f96e06d51f
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
You need to log in before you can comment on or make changes to this bug.