Current device appearing in synced devices list

RESOLVED FIXED in Firefox 50

Status

()

Firefox for Android
Android Sync
RESOLVED FIXED
2 years ago
7 months ago

People

(Reporter: Margaret, Assigned: Grisha)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 50
Points:
---

Firefox Tracking Flags

(fennec48+, firefox50 fixed)

Details

MozReview Requests

()

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

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
Created attachment 8740593 [details]
2016-04-12 20.03.26.png

I'm seeing "Nightly on Nexus 5X", last synced March 12, on my Nightly on Nexus 5X, which sync settings say has synced 0 minutes ago.

So... something is busted.

Grisha, would you know where to look to debug this?
Flags: needinfo?(gkruglov)
Started looking at this and ran into other bugs (crashes), which have been files away. Will continue my investigation today.
Flags: needinfo?(gkruglov)
Assignee: nobody → gkruglov
As far as I can tell, we don't actually store current client in the clients table or in the clients database.

Looking at the queries that power this data, they essentially do `SELECT * from tabs JOINT clients on tabs.client_guid = clients.guid`.

What you're seeing is not your current device! But, there's a twist - it's your current device from the past. If you've cleared app data, or disconnected your device from Sync, and then re-connected, a new client GUID will get generated for your device. Then, when syncing, we'll encounter GUID for your device from a few minutes ago - but it'll be different, and we have no other choice but to treat that client as a separate client. The name will likely be the same, unless you have manually changed it yourself at some point.

To make it even more confusing, list of tabs for this zombie client will likely be the same as your current list of tabs as well - because, it is your current list of tabs, but a few moments ago.

So, what you're seeing is both correct and incorrect. It is "working as intended" - I don't think there's a bug here - but it's also pretty confusing for the user.

I'm not sure how Sync treats these zombie clients, I'll research more into it. As far as showing this stuff in the UI - other than some super hacky/unreliable matching, I'm not sure there's anything to be done given how underlying data is stored/synced.
(In reply to :Grisha Kruglov from comment #2)
> If you've cleared app data, or
> disconnected your device from Sync, and then re-connected, a new client GUID
> will get generated for your device.

Bug 963446 tracks extending the Sync 1.1 client-record-deleter for Sync 1.5.

We also (at some point) will start using the FxA device service for this, rather than Sync's ad hoc client manager.


> I'm not sure how Sync treats these zombie clients, I'll research more into
> it. As far as showing this stuff in the UI - other than some super
> hacky/unreliable matching, I'm not sure there's anything to be done given
> how underlying data is stored/synced.

Fennec will expire them out of the UI after a week or so. The server should discard the old record after three weeks.
(Reporter)

Comment 4

2 years ago
(In reply to Richard Newman [:rnewman] from comment #3)

> > I'm not sure how Sync treats these zombie clients, I'll research more into
> > it. As far as showing this stuff in the UI - other than some super
> > hacky/unreliable matching, I'm not sure there's anything to be done given
> > how underlying data is stored/synced.
> 
> Fennec will expire them out of the UI after a week or so. The server should
> discard the old record after three weeks.

The zombie client I'm seeing in my UI says it was last sycned March 12, which is over a month ago.

I'm willing to accept that I'm probably an outlier of a user, but this is definitely confusing. Users don't care about the data model, they're just confused by this UI.
If we are indeed expiring clients, there's probably some bug there because I'm also seeing tons of old devices on some test devices where the sync date is early or mid-March.

As a side note: one other place where we keep track of device guids is in RemoteTabsExpandableListState. This saves device UI state (collapsed, hidden, selected) in a shared pref, although it's only used to check a device's state (once it's been fetched from the SQL call), and isn't directly used to populate anything.
There are two parts to expiration: the server throwing away the record after a TTL, and each client _obeying that TTL_ and throwing away its local copy. Note that when the server throws away a record, _clients are not told_: it's not a deletion per se.

We deliberately don't locally track and obey TTLs for some records (e.g., history). We might choose to do so for others (e.g., clients).

I'm pretty sure we don't do this at all yet: I've had "File bug to make clients respect TTLs" on my to-do list for ages.

We can either ignore all records older than three weeks (which I thought Vivek implemented, but perhaps you threw away that code?), or we can actively delete them when we sync.
(Reporter)

Comment 7

2 years ago
As part of bug 1262929, I think we should just hide the old clients if we can. We can deal with deleting them separately.
Blocks: 1262929
tracking-fennec: ? → 48+
(Reporter)

Comment 8

2 years ago
Grisha, what is the status here? 48 is going to beta next week.
Flags: needinfo?(gkruglov)
Created attachment 8759397 [details]
Bug 1264057 - Don't show stale clients on the History Panel

Currently we might have old (aka stale) clients appear on the History panel, which could be confusing.
This patch modifies underlying query to select only those clients whose LAST_MODIFIED timestamp
is newer than three weeks ago.

Review commit: https://reviewboard.mozilla.org/r/57404/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57404/
Attachment #8759397 - Flags: review?(margaret.leibovic)
Margaret, see the patch above - it simply hides old clients (haven't been synced in more than three weeks) from the combined history panel.
Flags: needinfo?(gkruglov)
Comment on attachment 8759397 [details]
Bug 1264057 - Don't show stale clients on the History Panel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57404/diff/1-2/
(Reporter)

Comment 13

2 years ago
Comment on attachment 8759397 [details]
Bug 1264057 - Don't show stale clients on the History Panel

https://reviewboard.mozilla.org/r/57404/#review54396

This seems fine to me, but how did you decide to choose 3 weeks? Can you document your reasoning in a comment?

However, I'm starting to wonder if it's even worth landing this. It's an edge case of a UI bug, but this patch turns it into two edge case bugs - one that can happen on either side (e.g. you may have a valid client that just hasn't been synced in a while, but you'd still want to see its tabs).

::: mobile/android/base/java/org/mozilla/gecko/db/LocalTabsAccessor.java:30
(Diff revision 2)
>  import android.text.format.DateUtils;
>  import android.util.Log;
>  
>  public class LocalTabsAccessor implements TabsAccessor {
>      private static final String LOGTAG = "GeckoTabsAccessor";
> +    private static final long THREE_WEEKS_IN_MILLISECONDS = 1000 * 60 * 60 * 24 * 7 * 3;

I seem to remember Mike (or someone else?) recently shared some utility method for making these ms calculations more readable... but I can't remember what it was. If nobody knows what I'm talking about, this is fine though :)
Attachment #8759397 - Flags: review?(margaret.leibovic) → review+
https://reviewboard.mozilla.org/r/57404/#review54396

Three weeks was chosen arbitrarily; seems like a "good enough" threshold for defining what stale is.

Although, I agree that we're turning this into two edge cases. With the UI updates to the history panel, old clients are much less visible now (at the bottom of a long list) - so it doesn't seem as bad showing some older data in case it might be useful to the user. We could just ignore this edge case and perhaps deal with underlying expiration issues, or perhaps bump up the three week threshold to a longer time frame?

> I seem to remember Mike (or someone else?) recently shared some utility method for making these ms calculations more readable... but I can't remember what it was. If nobody knows what I'm talking about, this is fine though :)

I do remember what that is, I'll push an update.
(In reply to :Grisha Kruglov from comment #14)
> https://reviewboard.mozilla.org/r/57404/#review54396
> 
> Three weeks was chosen arbitrarily; seems like a "good enough" threshold for
> defining what stale is.

A little clarification.  The value 3 weeks is arbitrary; it was chosen in the mists of time.  (rnewman is the keeper of such records.)  All clients upload records that are set to expire from the Sync server after 3 weeks; that is, a new client will never see client records older than 3 weeks.  Clients should upload a record at least once each week.  What's happening here is that *downloaded* client records aren't being expired after 3 weeks.  We work around this in the UI in various places.
Status: NEW → ASSIGNED
I'm going to land this change, I think it's good to be consistent in the data we show to users.

Comment 18

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/11880fa53889
https://hg.mozilla.org/mozilla-central/rev/c6b4171833bd
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50

Updated

7 months ago
Product: Android Background Services → Firefox for Android
You need to log in before you can comment on or make changes to this bug.