1180287 - Hide client records that are likely to be duplicates or stale in Fennec Synced Tabs panel

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Data Providers, defect)

Comment 1

[Ahmed Khalil]

Attachment: remote_clients_week_old.patch

This patch forces the TabsAccessor to return only clients/tabs whose last_modified field are less than 7 days old.

Nick: If you'd like I could add a JUnit test.

Comment 2

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

Comment on attachment 8629488 [details] [diff] [review]
remote_clients_week_old.patch

Review of attachment 8629488 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delayed review.  This is a great first cut.

However, it's not clear to me that this will address the issues in our UI.  We have:

* Sent Tabs pane;
* Send Tab action;
* the ActivityChooser model.

It's a little hard to trace all the code paths, but most seem to get a cursor directly and won't see this change.  We want to unify these through a single point, so these user facing changes are consistent.  Can you see what I'm missing chasing these things down?

We have some ContentProvider tests in TestRemoteTabs.  Can you investigate adding a test for this query?

::: mobile/android/base/db/LocalTabsAccessor.java
@@ +27,5 @@
>  
>  public class LocalTabsAccessor implements TabsAccessor {
>      private static final String LOGTAG = "GeckoTabsAccessor";
>  
> +    private static final long DAY_IN_MS = 1000 * 60 * 60 * 24;

nit: ONE_DAY_IN_MILLISECONDS and ONE_WEEK_IN_MILLISECONDS.

@@ +48,5 @@
>                                                                      BrowserContract.Clients.DEVICE_TYPE
>                                                              };
>  
> +    public static final String[] REMOTE_CLIENTS_SELECTION_ARGS = new String[] {
> +                                                                    String.valueOf(System.currentTimeMillis() - WEEK_AGO)

This will definitely not do the right thing, since it's static: the time will be fixed.  Push the currentTimeMillis() into consumers, and move the comparison math into the selection below.

@@ +175,5 @@
>      @Override
>      public Cursor getRemoteClientsByRecencyCursor(Context context) {
>          final Uri uri = clientsRecencyUriWithProfile;
>          return context.getContentResolver().query(uri, CLIENTS_PROJECTION_COLUMNS,
> +                REMOTE_CLIENTS_SELECTION, REMOTE_CLIENTS_SELECTION_ARGS, null);

Here's where the time calculation will go, in the args.

@@ +196,5 @@
>  
>          final Cursor cursor =  context.getContentResolver().query(uri,
>                                                              TABS_PROJECTION_COLUMNS,
>                                                              REMOTE_TABS_SELECTION,
> +                                                            REMOTE_CLIENTS_SELECTION_ARGS,

Nifty.

Comment 3

[Ahmed Khalil]

Attachment: remote_clients_week_old.patch

I've moved the interval calculation into the methods instead of doing it into a static final field.
I've added a test to TestRemoteTabs.java that tests adding clients older than one week and clients that is more recent than one week, I've also tested adding tabs to these clients and see if the tabs that are retrieved from the recent clients or not.

Comment 4

[Ahmed Khalil]

(In reply to Nick Alexander :nalexander (PTO July 17-27) from comment #2)
> Comment on attachment 8629488 [details] [diff] [review]
> remote_clients_week_old.patch
> 
> Review of attachment 8629488 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for the delayed review.  This is a great first cut.
> 
> However, it's not clear to me that this will address the issues in our UI. 
> We have:
> 
> * Sent Tabs pane;
> * Send Tab action;
> * the ActivityChooser model.

I've tested the patch against Sending a Tab, when a user presses the options menu that calls the ActivityChooser model, and also when he is viewing the "Synced Tabs" tab and all seems working. 

If there are other places that I forgotten to look for, please point out to me.

Comment 5

[Ahmed Khalil]

Comment on attachment 8633017 [details] [diff] [review]
remote_clients_week_old.patch

I'm flagging Richard instead, as Nick is going to be on a holiday and he won't be able to review the patch.

Comment 6

[Richard Newman [:rnewman]]

Comment on attachment 8633017 [details] [diff] [review]
remote_clients_week_old.patch

Review of attachment 8633017 [details] [diff] [review]:
-----------------------------------------------------------------

This isn't quite the fix that I had planned. This will aggressively exclude old client records; if we made this THREE_WEEKS_IN_MILLISECONDS, it's basically local TTL, which is fine, but doesn't directly address duping.

Our goal here is to exclude records that are obviously bad. Those fall into two categories:

1. Records that are really stale. They no longer exist on the server, because they were TTLed for being older than three weeks. We don't see a deletion because they were TTLed.

2. Records that are very likely the remnants of a device reconnection. There will be two records with the same name, and one of them is old -- perhaps more than a week old.


The reason for that second condition -- not just limiting everything to under a week, or fetching with a uniqueness constraint on device name -- is to avoid arbitrarily excluding some of a user's devices just because they have the same name; e.g., "Firefox on iPhone" or "Richard's Firefox on Mac OS X" are going to be common names!


To do these two things, we need a two-step query:

* Select records that are younger than 3 weeks.
* Exclude any records which have a younger sibling of the same name and are older than one week old.

I'm happy to mentor you through this in this bug, or we can switch this bug to be THREE_WEEKS and file a follow-up. Your call!

::: mobile/android/base/db/LocalTabsAccessor.java
@@ +27,5 @@
>  public class LocalTabsAccessor implements TabsAccessor {
>      private static final String LOGTAG = "GeckoTabsAccessor";
>  
> +    private static final long ONE_DAY_IN_MILLISECOND = 1000 * 60 * 60 * 24;
> +    private static final long ONE_WEEK_IN_MILLISECOND = 7 * ONE_DAY_IN_MILLISECOND;

`…IN_MILLISECONDS`

::: mobile/android/tests/browser/junit3/src/org/mozilla/tests/browser/junit3/TestRemoteTabs.java
@@ +23,5 @@
>  import java.util.List;
>  
>  public class TestRemoteTabs extends InstrumentationTestCase {
> +    private static final long ONE_DAY_IN_MILLISECOND = 1000 * 60 * 60 * 24;
> +    private static final long ONE_WEEK_IN_MILLISECOND = 7 * ONE_DAY_IN_MILLISECOND;

Same.

@@ +165,5 @@
> +            remote2.put(BrowserContract.Clients.GUID, "guid2");
> +            remote2.put(BrowserContract.Clients.NAME, "remote2");
> +            remote2.put(BrowserContract.Clients.LAST_MODIFIED, now - ONE_WEEK_IN_MILLISECOND + ONE_DAY_IN_MILLISECOND);
> +
> +            // Insert a Remote Client that are one week old.

s/are/is

Comment 7

[Ahmed Khalil]

Attachment: remote_clients_week_old.patch

Hi Richard,

I've been trying to use a subquery inside the where clause, but unfortunately I haven't figured out the right query. :( 

Here is what I tried 
> private static final String STALE_CLIENTS_SUB_SELECTION = 
> BrowserContract.Clients.NAME + " IN " +
>            "(SELECT Stale." + BrowserContract.Clients.NAME + " FROM " + 
> TabsProvider.TABLE_CLIENTS + " AS STALE WHERE " +
>            "Stale." + BrowserContract.Clients.LAST_MODIFIED + " > ? and " + 
> BrowserContract.Clients.NAME + " != " + "Stale." + 
> BrowserContract.Clients.NAME + ")";

I've also modified the test case (which currently of course fails) to include the cases that we are expecting.

Comment 8

[Richard Newman [:rnewman]]

Sorry for the delay getting to this; I'm swamped with iOS stuff. Will get to it.

Comment 9

[Richard Newman [:rnewman]]

Let's have some sample data. Taking 500 as "three weeks" and 1000 as "one week", just for the sake of messing around in a console:

sqlite> SELECT * from clients;
guid          name                  last_modified
------------  --------------------  ----------------
young1        Young                 1234
old1          Old                   800
old2          Old                   900
old3          Old                   999
young2        Young Again           1235


We're trying to select two different groups of rows: new ones, and then some subset of old ones.

We want greatest-n-per-group where n is specialized to 1. In sqlite > 3.7.11, that's easy, because the row returned for a group is guaranteed to match a MAX or MIN if one is present.

(Interestingly, this also seems to work fine without that MAX(last_modified) if there's an ORDER BY that's equivalent, but I wouldn't bet on it.)

  SELECT guid, name, last_modified, MAX(last_modified)
  FROM clients
  WHERE (last_modified < 1000 AND last_modified > 500)
  GROUP BY name
  ORDER BY last_modified;


The generic SQL solution, which we'll need for sqlite earlier than 3.7.11, is straightforward, but less efficient than relying on the query engine's guarantees:

  SELECT c.guid AS guid, c.name AS name, c.last_modified AS last_modified FROM clients AS c
  JOIN (
    SELECT guid, MAX(last_modified) AS last_modified
    FROM clients
    WHERE (last_modified < 1000 AND last_modified > 500)
    GROUP BY name
  ) AS c2
  ON c.guid = c2.guid;


To get the most recent from every old group, plus all recent records, we can use UNION ALL.

Android 16 and up support sqlite 3.7.11. So on those platforms, ignoring the last column:


  SELECT guid, name, last_modified, last_modified
  FROM clients
  WHERE last_modified > 1000

  UNION ALL

  SELECT guid, name, last_modified, MAX(last_modified)
  FROM clients
  WHERE (last_modified < 1000 AND last_modified > 500)
  GROUP BY name
  ORDER BY last_modified;


and on older Android versions (please test!):

  SELECT guid, name, last_modified
  FROM clients
  WHERE last_modified > 1000

  UNION ALL

  SELECT c.guid AS guid, c.name AS name, c.last_modified AS last_modified FROM clients AS c
  JOIN (
    SELECT guid, MAX(last_modified) AS last_modified
    FROM clients
    WHERE (last_modified < 1000 AND last_modified > 500)
    GROUP BY name
  ) AS c2
  ON c.guid = c2.guid;


Running this query on our test data gives us what we want:

guid          name                  last_modified     last_modified
------------  --------------------  ----------------  -------------
old3          Old                   999               999
young1        Young                 1234              1234
young2        Young Again           1235              1235


Obviously there are three values to pass in here in either query -- the timestamp of one week ago (twice) and the timestamp of three weeks ago, in place of 1000 and 500.

Let me know when you've got a new patch, and sorry for the delay!

Comment 10

[Ahmed Khalil]

Attachment: remote_clients_week_old.patch

Hi Richard,

I've finally implemented your query, and the test case now is passing. 

Let me know what do you think.

Comment 11

[Richard Newman [:rnewman]]

Comment on attachment 8644918 [details] [diff] [review]
remote_clients_week_old.patch

Review of attachment 8644918 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good; just nits. Thanks for your patience!

::: mobile/android/base/db/TabsProvider.java
@@ +33,5 @@
>      static final int CLIENTS = 602;
>      static final int CLIENTS_ID = 603;
>      static final int CLIENTS_RECENCY = 604;
>  
> +    // Exclude clients that are more than three weeks old and also the duplicates that is older than one week old.

replace "the duplicates that is" by "any duplicates that are".

@@ +34,5 @@
>      static final int CLIENTS_ID = 603;
>      static final int CLIENTS_RECENCY = 604;
>  
> +    // Exclude clients that are more than three weeks old and also the duplicates that is older than one week old.
> +    static final String EXCLUDE_STALE_CLIENTS_TABLE = "(SELECT " + Clients.GUID + ", " + Clients.NAME + ", " + Clients.LAST_MODIFIED + ", " + Clients.DEVICE_TYPE +

replace "_TABLE" by "_SUBQUERY" in the name of this field.

Extra points for cleaning up the formatting so this is more readable:

  static final String EXCLUDE_STALE_CLIENTS_SUBQUERY =
  "(SELECT " + Clients.GUID +
  ", " + Clients.NAME +
  ", " + Clients.LAST_MODIFIED +
  ", " + Clients.DEVICE_TYPE + 
  " FROM " + …

@@ +39,5 @@
> +                                            "  FROM " + TABLE_CLIENTS +
> +                                            "  WHERE " + Clients.LAST_MODIFIED + " > %1$s " +
> +                                            " GROUP BY " + Clients.NAME +
> +                                            " UNION ALL " +
> +                                            " SELECT " + projectColumn("c", Clients.GUID) + " AS " + Clients.GUID + ", " + projectColumn("c", Clients.NAME) + " AS " + Clients.NAME + ", " +

I have a preference for

  " SELECT c." + Clients.GUID

rather than

  " SELECT " + projectColumn("c", Cients.GUID)

@@ +314,5 @@
>                      debug("Using sort order " + sortOrder + ".");
>                  }
>  
> +                final long oneWeekDuration = System.currentTimeMillis() - ONE_WEEK_IN_MILLISECONDS;
> +                final long threeWeeksDuration = System.currentTimeMillis() - THREE_WEEKS_IN_MILLISECONDS;

These should be called `oneWeekAgo` and `threeWeeksAgo`.

@@ +322,2 @@
>                  qb.setProjectionMap(CLIENTS_RECENCY_PROJECTION_MAP);
> +                qb.setTables(excludeStaleClientsTable + " AS " + TABLE_CLIENTS + " LEFT OUTER JOIN " + TABLE_TABS +

Add a comment:

// Use a subquery to quietly exclude stale duplicate client records.

Comment 12

[Ahmed Khalil]

Attachment: remote_clients_week_old.patch

Applied your comments, If you have other comments I'll be glad to apply.

Comment 13

[Richard Newman [:rnewman]]

Comment on attachment 8645834 [details] [diff] [review]
remote_clients_week_old.patch

Review of attachment 8645834 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. If this works in practice, ship it!

::: mobile/android/base/db/TabsProvider.java
@@ +35,5 @@
>      static final int CLIENTS_RECENCY = 604;
>  
> +    // Exclude clients that are more than three weeks old and also any duplicates that are older than one week old.
> +    static final String EXCLUDE_STALE_CLIENTS_SUBQUERY =
> +                                            "(SELECT " + Clients.GUID +

Can we shift all of these to be aligned under 'static'?

::: mobile/android/tests/browser/junit3/src/org/mozilla/tests/browser/junit3/TestRemoteTabs.java
@@ +139,5 @@
> +
> +        try {
> +            // Start Clean
> +            cpc.delete(uri, null, null);
> +            Cursor allClients = cpc.query(uri, null, null, null, null);

final

Comment 14

[Ahmed Khalil]

Attachment: remote_clients_week_old.patch

Did Richards suggestions.

Comment 15

[Ahmed Khalil]

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

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/4e6b58e166b7

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4e6b58e166b7