1264057 - Current device appearing in synced devices list

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Android Sync, defect)

Description

[:Margaret Leibovic]

Attachment: 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?

Comment 1

[:Grisha Kruglov]

Started looking at this and ran into other bugs (crashes), which have been files away. Will continue my investigation today.

Comment 2

[:Grisha Kruglov]

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.

Comment 3

[Richard Newman [:rnewman]]

(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.

Comment 4

[:Margaret Leibovic]

(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.

Comment 5

[Chenxia Liu [:liuche]]

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.

Comment 6

[Richard Newman [:rnewman]]

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.

Comment 7

[:Margaret Leibovic]

As part of bug 1262929, I think we should just hide the old clients if we can. We can deal with deleting them separately.

Comment 8

[:Margaret Leibovic]

Grisha, what is the status here? 48 is going to beta next week.

Comment 9

[:Grisha Kruglov]

Attachment: 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/

Comment 10

[:Grisha Kruglov]

Margaret, see the patch above - it simply hides old clients (haven't been synced in more than three weeks) from the combined history panel.

Comment 11

[:Grisha Kruglov]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1405f0e2e399

Comment 12

[:Grisha Kruglov]

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/

Comment 13

[:Margaret Leibovic]

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 :)

Comment 14

[:Grisha Kruglov]

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.

Comment 15

[Nick Alexander :nalexander [he/him]]

(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.

Comment 16

[:Grisha Kruglov]

I'm going to land this change, I think it's good to be consistent in the data we show to users.

Comment 17

[:Grisha Kruglov]

https://hg.mozilla.org/integration/fx-team/rev/11880fa53889c4b79a2b6289958ee3b8031eb03f
Bug 1264057 - Don't show stale clients on the History Panel r=margaret

https://hg.mozilla.org/integration/fx-team/rev/c6b4171833bd9806319fd0eb97e0591d241d1808
Bug 1264057 - Post: remove unnecessary import statements r=grisha

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/11880fa53889
https://hg.mozilla.org/mozilla-central/rev/c6b4171833bd