1159020 - Move share overlay's access to sync to TabsProvider

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

via bug 1145896 comment 9:

> > mcomella: on an unrelated note, all use of the Sync-specific clients
> > database can and should transition to the TabsProvider

So we should stop touching the ClientDatabaseAccessor directly and move to TabsProvider to avoid technical debt.

Comment 1

[Nick Alexander :nalexander [he/him]]

The sooner the better.  Some context:

We have two databases that track Sync clients: an App-wide "clients.db" and a per-profile "tabs.db".  The former is Sync-specific and pre-dates the latter.  The latter is what Fennec should use.  This ticket tracks removing direct access to Sync's database and only using the latter.

Looking at [1], and noting that mobile/android/base/sync and mobile/android/tests/background are Sync-specific, I see uses of ClientsDatabaseAccessor which we should get rid of in:

mobile/android/base/overlays/service/sharemethods/SendTab.java
mobile/android/base/widget/ActivityChooserModel.java

Presumably both of those want lists of Sync clients (not tabs), and that can be extracted from the tabs DB by querying and grouping appropriately.  The RemoteTabs home panel code at [2] provides a model.

[1] https://dxr.mozilla.org/mozilla-central/search?q=ClientsDatabaseAccessor&redirect=true
[2] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/home/RemoteTabsBaseFragment.java#229

Comment 2

[Nick Alexander :nalexander [he/him]]

Ahmed, this is a better-defined but valuable ticket you might be interested in.  vivek, this is code you understand well and it could be a good ticket for us to co-mentor.

Comment 3

[Ahmed Khalil]

Hi Nick, 

I've investigated the code in SendTab.java, and I'd like to ask how can I retrieve a ClientRecord from 
BrowserDB.getTabsAccessor() ? Is there any code that you can refer to it that helps me in that task ?

Comment 4

[Nick Alexander :nalexander [he/him]]

(In reply to Ahmed Khalil from comment #3)
> Hi Nick, 
> 
> I've investigated the code in SendTab.java, and I'd like to ask how can I
> retrieve a ClientRecord from 
> BrowserDB.getTabsAccessor() ? Is there any code that you can refer to it
> that helps me in that task ?

Hey!  Your model should be the Synced Tabs home panel, which is somewhat confusingly named RemoteTabs*.java in the source code.  You get the clients in two steps:

* getRemoteTabsCursor -- see [1];
* getClientsFromCursor -- see [2].

[1] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/home/RemoteTabsBaseFragment.java#214
[2] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/home/RemoteTabsBaseFragment.java#229

This ticket will require some refactoring -- but that's what makes it valuable :)  By the time you're done, you should have no references to ClientRecord coming from org.mozilla.gecko.sync.

Comment 5

[Ahmed Khalil]

Attachment: send_tabs.patch

I've replaced the usage of ClientsDatabaseAccessor with TabsAccessor in SendTab.java, also replaced the usage of ClientRecords with RemoteClients and doing the necessary refactoring.

Comment 6

[Vivek Balakrishnan[:vivek]]

@Nick: can you please review this patch. Unfortunately I can review this patch only on Thursday.

Comment 7

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8606565 [details] [diff] [review]
send_tabs.patch

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

This looks like a great start!

I have one comment about ordering the clients, but I expect that to not be a big deal.  Over the weekend, I had a concern, related to Bug 1025128.  Right now, the tabs table is odd: it joins tabs and clients together.  I'm concerned that we currently won't "see" (in this code) clients that are connected and known to Sync but have no tabs open.  (This can happen for a bunch of legitimate reasons.)

Could you test that having a client with no tabs shows up?  If not, we'll need to work on the Remote Client accessor to fetch the clients directly (without the tabs), or otherwise work-around.  Thanks!

(This is r+ but I'd like to see it again, and there's more to come for this ticket.)

::: mobile/android/base/overlays/service/sharemethods/SendTab.java
@@ +190,5 @@
>      /**
>       * Load the list of Sync clients that are not this device using the given TabSender.
>       */
>      private void updateClientList(TabSender tabSender) {
> +        Collection<RemoteClient> otherClients = getOtherClients(tabSender);

Can we ensure that we maintain the client order?  We try to sort clients so that the most recently used appear first.  I'd like to keep that.

We might need to sort ourselves, or get a List<> back, or...

Comment 8

[Ahmed Khalil]

(In reply to Nick Alexander :nalexander from comment #7)
> Comment on attachment 8606565 [details] [diff] [review]
> send_tabs.patch
> 
> Review of attachment 8606565 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks like a great start!
> 
> I have one comment about ordering the clients, but I expect that to not be a
> big deal.  Over the weekend, I had a concern, related to Bug 1025128.  Right
> now, the tabs table is odd: it joins tabs and clients together.  I'm
> concerned that we currently won't "see" (in this code) clients that are
> connected and known to Sync but have no tabs open.  (This can happen for a
> bunch of legitimate reasons.)
> 
> Could you test that having a client with no tabs shows up?  If not, we'll
> need to work on the Remote Client accessor to fetch the clients directly
> (without the tabs), or otherwise work-around.  Thanks!
> 

Just tested it and you are right, the clients that is known to sync but have no tabs open are not shown
in the clients list when sharing a tab.

> (This is r+ but I'd like to see it again, and there's more to come for this
> ticket.)
> 
> ::: mobile/android/base/overlays/service/sharemethods/SendTab.java
> @@ +190,5 @@
> >      /**
> >       * Load the list of Sync clients that are not this device using the given TabSender.
> >       */
> >      private void updateClientList(TabSender tabSender) {
> > +        Collection<RemoteClient> otherClients = getOtherClients(tabSender);
> 
> Can we ensure that we maintain the client order?  We try to sort clients so
> that the most recently used appear first.  I'd like to keep that.
> 
> We might need to sort ourselves, or get a List<> back, or...
The RemoteClient has "lastModified" field, does this field change according to the usage so that I could sort according to it ?

Comment 9

[Ahmed Khalil]

Also I'd like to ask if I wanted to implement getClientsCount in TabsAccessor, Is it okay to just query the content provider with the clients uri, and retrieve all the clients available ?

Something like

> context.getContentResolver().query(clientsUriWithProfile, null, null, null, null);

Or should I add a projection or a where clause ?

Comment 10

[Nick Alexander :nalexander [he/him]]

(In reply to Ahmed Khalil from comment #9)
> Also I'd like to ask if I wanted to implement getClientsCount in
> TabsAccessor, Is it okay to just query the content provider with the clients
> uri, and retrieve all the clients available ?
> 
> Something like
> 
> > context.getContentResolver().query(clientsUriWithProfile, null, null, null, null);
> 
> Or should I add a projection or a where clause ?

This is a good approach.  You'll want WHERE client_guid IS NOT NULL to filter the local client.

With regard to the last modified field, we might want to do something a little more clever, like we do on iOS: see https://github.com/mozilla/firefox-ios/blob/a7d95f10898a54720e4b5f57775992eab86b6865/Storage/RemoteTabs.swift#L17.  That is, we take the latest tab used timestamp (or the client record if there are no tabs).  You could probably take the max over the tabs usage stamps using SQL, but it might not be easy using the ContentProvider interface.  Investigate?

Comment 11

[Ahmed Khalil]

Attachment: send_tabs.patch

In this patch, I've removed the ClientsDatabaseAccessor from the ActivityChooserModel, and used TabsAccessor instead, also added getClientsCount to the TabAccessor interface.

I've implemented in-memory sorting of a list of RemoteClients, it would be better though to implement that using SQL statements but that approach would need to change the TabsProvider I guess.

I think that what still missing is making clients with no tabs to be returned when querying the TabsProvider. 

Nick, Could you please point me to that ?

Finally, rnewman has offered his help on the SQL solution of returning RemoteClients sorted according to the latest usage. So I would be glad if he could advice me on that matter.

Comment 12

[Richard Newman [:rnewman]]

Comment on attachment 8609079 [details] [diff] [review]
send_tabs.patch

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

If you want to get a list of clients, including those without tabs, sorted by most recent tab:

  SELECT clients.guid AS guid, clients.name AS name, clients.last_modified AS last_modified, clients.device_type AS device_type, MAX(tabs.last_used)
  FROM clients LEFT JOIN tabs ON
  clients.guid = tabs.client_guid
  WHERE clients.guid IS NOT NULL
  GROUP BY tabs.client_guid
  ORDER BY COALESCE(MAX(tabs.last_used), clients.last_modified) DESC;

Avoid hard-coding table names etc. when you implement this.

This would be a reasonable thing to stick behind a predefined query URI for one of the ContentProviders.

::: mobile/android/base/db/LocalTabsAccessor.java
@@ +71,5 @@
> +
> +        try {
> +            return cursor != null ? cursor.getCount() : 0;
> +        } finally {
> +            if (cursor != null) {

final Cursor cursor = ...;
if (cursor == null) {
    return 0;
}

try {
    return cursor.getCount();
} finally {
    cursor.close();
}

Comment 13

[Ahmed Khalil]

Attachment: send_tabs.patch

Implemented Richard's query in Android's way. 
Tested with clients with no tabs and with clients that has changed tabs.

Comment 14

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8609617 [details] [diff] [review]
send_tabs.patch

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

This is looking great.  Let's break it up into pieces: BrowserContract and TabsProvider changes first.  I'm going to investigate writing a few tests for this new functionality.  I specifically want to know what happens when we have local tabs (so NULL client GUID) and remote clients with and without tabs (like you tested).

I would like to see getClientsCount go away and use the other helpers.

Can ParcelableClientRecord and friends go away after this patch?  As a follow-up patch?

This is r+ after some small-ish changes and I test locally.  Nice patch!

::: mobile/android/base/db/LocalTabsAccessor.java
@@ +72,5 @@
> +        clientsRecencyUriWithProfile = DBUtils.appendProfileWithDefault(mProfile, BrowserContract.Clients.CONTENT_RECENCY_URI);
> +    }
> +
> +    @Override
> +    public int getClientsCount(Context context) {

I'm not sure this should be in this class.  Maybe?

::: mobile/android/base/db/TabsAccessor.java
@@ +16,5 @@
>      public interface OnQueryTabsCompleteListener {
>          public void onQueryTabsComplete(List<RemoteClient> clients);
>      }
>  
> +    public int getClientsCount(Context context);

This isn't following the cursor -> consumer pattern.  It's a problem because it's not clear if this includes the local client.  I'd remove this or clarify it.

::: mobile/android/base/db/TabsProvider.java
@@ +85,5 @@
> +        map.put(Clients.GUID, projectColumn(TABLE_CLIENTS, Clients.GUID) + " AS guid");
> +        map.put(Clients.NAME, projectColumn(TABLE_CLIENTS, Clients.NAME) + " AS name");
> +        map.put(Clients.LAST_MODIFIED, projectColumn(TABLE_CLIENTS, Clients.LAST_MODIFIED) + " AS last_modified");
> +        map.put(Clients.DEVICE_TYPE, projectColumn(TABLE_CLIENTS, Clients.DEVICE_TYPE) + " AS device_type");
> +        map.put(Tabs.LAST_USED, "MAX(" + projectColumn(TABLE_TABS, Tabs.LAST_USED) + ")");

What happens here if there's no matching tab?  Is it NULL?

@@ +254,5 @@
>              case TABS_ID:
>                  trace("Delete on TABS_ID: " + uri);
>                  selection = DBUtils.concatenateWhere(selection, selectColumn(TABLE_TABS, Tabs._ID));
>                  selectionArgs = DBUtils.appendSelectionArgs(selectionArgs,
> +                        new String[]{Long.toString(ContentUris.parseId(uri))});

nit: don't change this line.

@@ +384,5 @@
>                  if (TextUtils.isEmpty(sortOrder)) {
>                      sortOrder = DEFAULT_CLIENTS_SORT_ORDER;
>                  } else {
>                      debug("Using sort order " + sortOrder + ".");
>                  }

nit: leave this too.  In general, extraneous changes make life harder.

@@ +395,5 @@
> +                    sortOrder = DEFAULT_CLIENTS_RECENCY_SORT_ORDER;
> +                } else {
> +                    debug("Using sort order " + sortOrder + ".");
> +                }
> +                trace("Query is on CLIENTS_RECENCY: " + uri);

Nifty!

::: mobile/android/base/overlays/service/sharemethods/SendTab.java
@@ +195,3 @@
>  
> +        // Put the list of RemoteClients into the uiStateIntent and broadcast it.
> +        RemoteClient[] records = new RemoteClient[otherClients.size()];

I'm still concerned about ordering the GUIDs here.  Explain why this does the right thing, keeping most recently used at the top?

::: mobile/android/base/widget/ActivityChooserModel.java
@@ +1312,5 @@
>              return false;
>          }
>  
> +        final BrowserDB browserDB = GeckoProfile.get(mContext).getDB();
> +        final int clientsCount = browserDB.getTabsAccessor().getClientsCount(mContext);

Ah, I see why you did this how you did.  I'd be pretty happy with using getRemoteClientsWithoutTabs().size().  It's as clear, and saves some special case code.

Comment 15

[Ahmed Khalil]

(In reply to Nick Alexander :nalexander from comment #14)
> Comment on attachment 8609617 [details] [diff] [review]
> send_tabs.patch
> 
> Review of attachment 8609617 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is looking great.  Let's break it up into pieces: BrowserContract and
> TabsProvider changes first.  I'm going to investigate writing a few tests
> for this new functionality.  I specifically want to know what happens when
> we have local tabs (so NULL client GUID) and remote clients with and without
> tabs (like you tested).
> 
> I would like to see getClientsCount go away and use the other helpers.
> 
> Can ParcelableClientRecord and friends go away after this patch?  As a
> follow-up patch?

Yeah, It is very possible to do that, it is not used anymore.

> 
> This is r+ after some small-ish changes and I test locally.  Nice patch!
> 
> ::: mobile/android/base/db/LocalTabsAccessor.java
> @@ +72,5 @@
> > +        clientsRecencyUriWithProfile = DBUtils.appendProfileWithDefault(mProfile, BrowserContract.Clients.CONTENT_RECENCY_URI);
> > +    }
> > +
> > +    @Override
> > +    public int getClientsCount(Context context) {
> 
> I'm not sure this should be in this class.  Maybe?

I've removed that in the following patch and used getRemoteClientsByRecencyCursor().getCount() instead.

> 
> ::: mobile/android/base/db/TabsAccessor.java
> @@ +16,5 @@
> >      public interface OnQueryTabsCompleteListener {
> >          public void onQueryTabsComplete(List<RemoteClient> clients);
> >      }
> >  
> > +    public int getClientsCount(Context context);
> 
> This isn't following the cursor -> consumer pattern.  It's a problem because
> it's not clear if this includes the local client.  I'd remove this or
> clarify it.
> 
> ::: mobile/android/base/db/TabsProvider.java
> @@ +85,5 @@
> > +        map.put(Clients.GUID, projectColumn(TABLE_CLIENTS, Clients.GUID) + " AS guid");
> > +        map.put(Clients.NAME, projectColumn(TABLE_CLIENTS, Clients.NAME) + " AS name");
> > +        map.put(Clients.LAST_MODIFIED, projectColumn(TABLE_CLIENTS, Clients.LAST_MODIFIED) + " AS last_modified");
> > +        map.put(Clients.DEVICE_TYPE, projectColumn(TABLE_CLIENTS, Clients.DEVICE_TYPE) + " AS device_type");
> > +        map.put(Tabs.LAST_USED, "MAX(" + projectColumn(TABLE_TABS, Tabs.LAST_USED) + ")");
> 
> What happens here if there's no matching tab?  Is it NULL?
I think it will return an empty cursor but not sure though, as I haven't tested that case.
> 
> @@ +254,5 @@
> >              case TABS_ID:
> >                  trace("Delete on TABS_ID: " + uri);
> >                  selection = DBUtils.concatenateWhere(selection, selectColumn(TABLE_TABS, Tabs._ID));
> >                  selectionArgs = DBUtils.appendSelectionArgs(selectionArgs,
> > +                        new String[]{Long.toString(ContentUris.parseId(uri))});
> 
> nit: don't change this line.
Reverted.

> ::: mobile/android/base/overlays/service/sharemethods/SendTab.java
> @@ +195,3 @@
> >  
> > +        // Put the list of RemoteClients into the uiStateIntent and broadcast it.
> > +        RemoteClient[] records = new RemoteClient[otherClients.size()];
> 
> I'm still concerned about ordering the GUIDs here.  Explain why this does
> the right thing, keeping most recently used at the top?

I don't really understand this comment, If you are wondering, this list is returned from a cursor that contains RemoteClients sorted by the recently used, being on top of the list.

Comment 16

[Ahmed Khalil]

Attachment: send_tabs.patch

Comment 17

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1159020 - Move share overlay's access to sync to TabsProvider, r=nalexander

Bug 1159020 - Move share overlay's access to sync to TabsProvider, r=nalexander

Comment 18

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1159020 - Review comments and a browser JUnit 3 test.

Bug 1159020 - Review comments and a browser JUnit 3 test.

I wrote a JUnit 3 test (not yet run in automation) to test this
functionality, and found that the query as written wasn't quite right:
* the ordering didn't handle the case when there are no tabs;
* the grouping was wrong, so you only ever got one row.

I have not tested this on a device with real clients.

Comment 19

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #18)
> Created attachment 8612553 [details]
> MozReview Request: Bug 1159020 - Review comments and a browser JUnit 3 test.
> 
> Bug 1159020 - Review comments and a browser JUnit 3 test.
> 
> I wrote a JUnit 3 test (not yet run in automation) to test this
> functionality, and found that the query as written wasn't quite right:
> * the ordering didn't handle the case when there are no tabs;
> * the grouping was wrong, so you only ever got one row.
> 
> I have not tested this on a device with real clients.

Ahmed: can you take a look at these changes?  Running the JUnit 3 test can be a little tricky, but as long as you're in IntelliJ (or Android Studio) it should "just work".  Open the test file, right click the class name, choose Run, and choose the "Android guy" with the "JUnit red and green arrows".  (Just the red and green arrows will run it as a regular JUnit test, which will fail.)  I'm planning a post and video about this, and will land a patch or two to smooth this shortly.

I'd appreciate testing on a real device with some clients with and without tabs, if you could do it.

Comment 20

[Ahmed Khalil]

(In reply to Nick Alexander :nalexander from comment #18)
> Created attachment 8612553 [details]
> MozReview Request: Bug 1159020 - Review comments and a browser JUnit 3 test.
> 
> Bug 1159020 - Review comments and a browser JUnit 3 test.
> 
> I wrote a JUnit 3 test (not yet run in automation) to test this
> functionality, and found that the query as written wasn't quite right:
> * the ordering didn't handle the case when there are no tabs;
> * the grouping was wrong, so you only ever got one row.
> 
> I have not tested this on a device with real clients.

I've tested it on real devices, and I believe things are working as expected.

I've tried syncing with devices that has tabs opened recently and it was ordered correctly, also tried closing tabs on a device and invoke a sync, it was still listed on the devices list.

Comment 21

[Ahmed Khalil]

Comment on attachment 8612553 [details]
MozReview Request: Bug 1159020 - Review comments and a browser JUnit 3 test.

https://reviewboard.mozilla.org/r/9595/#review8393

Ship It!

Comment 22

[Nick Alexander :nalexander [he/him]]

Tested as follows:

* new install, new profile on Android device.
* create and verify new test Firefox Account.   Connect Android device to account.  Verify that no clients display in Synced Tabs, and that "Add to Fennec" from another App does not offer any "other devices".
* connect a fresh Nightly profile.  Ensure the fresh Nightly profile has no tabs.  (This is testing an edge case specifically.)  Force a sync on Desktop.  Force a sync on Android.  Verify that no clients display in Synced Tabs.  (Since there are not tabs.)  Verify that "Add to Fennec" from another App /does/ offer the new device.  (Since there is a device (without tabs).)
* connect a second Nigthly profile.  Ensure the fresh Nightly profile has at least one tab.  Force a sync on Desktop and on Android.  Verify that clients appear in Synced Tabs.  Verify that "Add to Fennec" from another App shows both devices.

Comment 23

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/52763281c7b1

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/52763281c7b1

Comment 25

[Dave Miller [:justdave]]

An apparent side-effect of this patch: on profiles that have been around for a while across multiple devices, you now get in the share tab a list of every device you've ever had on your account, and every name any of them has ever had.  In the past the older ones would drop off a while after you stopped syncing from them.  I have 3 active devices on my account. I now get 19 choices in my share menu, several of which are older devices I no longer have, and some are the original "default" names for devices I changed the name of shortly after setting up to make them more distinct.

Comment 26

[Nick Alexander :nalexander [he/him]]

(In reply to Dave Miller [:justdave] ([e-mail]) from comment #25)
> An apparent side-effect of this patch: on profiles that have been around for
> a while across multiple devices, you now get in the share tab a list of
> every device you've ever had on your account, and every name any of them has
> ever had.  In the past the older ones would drop off a while after you
> stopped syncing from them.  I have 3 active devices on my account. I now get
> 19 choices in my share menu, several of which are older devices I no longer
> have, and some are the original "default" names for devices I changed the
> name of shortly after setting up to make them more distinct.

justdave: thanks for this report.  We have a custom query that we clearly regressed, or somehow changed in your case.

Ahmed: we've regressed something.  We may be able to witness this locally; can you look into it?

justdave: Would you be willing to pull your browser.db file using https://addons.mozilla.org/en-US/android/addon/copy-profile and share parts of it privately?  I think we'd need both your remote tabs and your clients, since they both factor into this calculation.  (But we wouldn't need your history or bookmarks or passwords.)  I could produce an SQLite cleaning script that dropped URLs and titles from the tabs, too, if they are sensitive -- we really only need the timestamps of the various records.

Comment 27

[Ahmed Khalil]

Hi Nick,

I've investigated the issue, and I found that it happens as the clients table contains multiple rows with same name but with different guids and different timestamps.

Here is my clients table dump 

> ||1435781943266|
> fiT2G8n7iWCz|Fennec ramdac on Xperia ZR|1434495517200|mobile
> vSkt2RJ5cQQe|Firefox on Xperia ZR|1435730765380|mobile
> T6AikF6oIrAK|Fennec ramdac on Xperia ZR|1434415214060|mobile
> gA2sQQOsVlre|Fennec ramdac on Xperia ZR|1434414514100|mobile
> xR67OqDXXL2W|Fennec ramdac on Xperia ZR|1434414120310|mobile
> 2NdJFU6AGxJ8|Fennec ramdac on Xperia ZR|1434413529240|mobile
> KLSNrIVvZOMc|Fennec ramdac on Xperia ZR|1434412633990|mobile
> 8lhwRaftNzpS|Fennec ramdac on Xperia ZR|1434409747880|mobile
> e_8GJpy1DDcZ|Fennec ramdac on Xperia ZR|1433967976330|mobile

And here are the tabs table dump with dummy data

> 2|fiT2G8n7iWCz|Welcome to Facebook|https://m.facebook.com/?_rdr&refsrc=https%3A%2F%2Fwww.facebook.com%2F|["https:\/\/m.facebook.com\/?_rdr&refsrc=https%3A%2F%2Fwww.facebook.com%2F"]||1434495402000|0
> 4|T6AikF6oIrAK|Welcome to Facebook|https://m.facebook.com/?_rdr&refsrc=https%3A%2F%2Fwww.facebook.com%2F|["https:\/\/m.facebook.com\/?_rdr&refsrc=https%3A%2F%2Fwww.facebook.com%2F"]||1434415128000|0
> 5|gA2sQQOsVlre|Welcome to Facebook|https://m.facebook.com/?_rdr&refsrc=https%3A%2F%2Fwww.facebook.com%2F|["https:\/\/m.facebook.com\/?_rdr&refsrc=https%3A%2F%2Fwww.facebook.com%2F"]||1434414375000|0
> 6|xR67OqDXXL2W|Welcome to Facebook|https://m.facebook.com/?_rdr&refsrc=https%3A%2F%2Fwww.facebook.com%2F|["https:\/\/m.facebook.com\/?_rdr&refsrc=https%3A%2F%2Fwww.facebook.com%2F"]||1434414002000|0
> 7|KLSNrIVvZOMc|Welcome to Facebook|https://m.facebook.com/?_rdr&refsrc=https%3A%2F%2Fwww.facebook.com%2F|["https:\/\/m.facebook.com\/?_rdr&refsrc=https%3A%2F%2Fwww.facebook.com%2F"]||1434412615000|0
> 8|8lhwRaftNzpS|Welcome to Facebook|https://m.facebook.com/?_rdr&refsrc=https%3A%2F%2Fwww.facebook.com%2F|["https:\/\/m.facebook.com\/?_rdr&refsrc=https%3A%2F%2Fwww.facebook.com%2F"]||1434409721000|0
> 9|8lhwRaftNzpS|tech crunch - Google Search|https://www.google.com/search?q=tech+crunch&ie=utf-8&oe=utf-8|["https:\/\/www.google.com\/search?q=tech+crunch&ie=utf-8&oe=utf-8"]||1434409721000|1
> 10|e_8GJpy1DDcZ|Welcome to Facebook|https://m.facebook.com/?_rdr&refsrc=https%3A%2F%2Fwww.facebook.com%2F|["https:\/\/m.facebook.com\/?_rdr&refsrc=https%3A%2F%2Fwww.facebook.com%2F"]||1433967918000|0
> 28|vSkt2RJ5cQQe|Facebook|https://m.facebook.com/home.php?refsrc=https%3A%2F%2Fwww.facebook.com%2F&refid=9&_rdr|["https:\/\/m.facebook.com\/home.php?refsrc=https%3A%2F%2Fwww.facebook.com%2F&refid=9&_rdr"]|https://m.facebook.com/icon.svg|1435730696000|0

So the question is why is there are multiple different guids but with the same name are inserted in the clients table ?

Comment 28

[Nick Alexander :nalexander [he/him]]

> So the question is why is there are multiple different guids but with the
> same name are inserted in the clients table ?

This is what happens when you connect the same device to a Firefox Account (really, Sync account) multiple times.  There's no knowledge (really) of the device; it's new, chooses a random identifier, and you get the table you see.

The old entries should get purged from the table eventually.  The server deletes the records after a TTL expires (usually, 3 weeks).  It's my belief that the Sync code deletes all the local records each sync and replaces them with the server records, so we should expire old records eventually as well.  This all happens in https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/sync/stage/SyncClientsEngineStage.java (which writes the older Sync-specific clients database (not table!)) and especially in https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/sync/repositories/android/FennecTabsRepository.java#219.

Looking at that code again, I think my understanding is wrong!  We're not deleting old clients (and their tab records) from the database.  Ahmed, can you dig into this and figure out if we do remove old clients, and if we don't, where that code could go?

Thanks!

Comment 29

[Richard Newman [:rnewman]]

It's supposed to wipe:

execute > downloadClientRecords > (sets shouldWipe = true) > handleRequestSuccess > wipeAndStore.

That it doesn't is surprising.

Comment 30

[Nick Alexander :nalexander [he/him]]

(In reply to Richard Newman [:rnewman] from comment #29)
> It's supposed to wipe:
> 
> execute > downloadClientRecords > (sets shouldWipe = true) >
> handleRequestSuccess > wipeAndStore.
> 
> That it doesn't is surprising.

This is happening in the clients engine, which touches the Sync-specific clients database.  We're standardizing Fennec on the clients & tabs tables that Fennec owns, which are maintained by the tabs engine, and need to be updated accordingly.

Long term, the clients database disappears.

Comment 31

[Richard Newman [:rnewman]]

Then yeah, that's why this is b0rked :)

Comment 32

[Ahmed Khalil]

I've created another Bug 1180287 that address this issue, and I've already submitted a patch there.

Comment 33

[Dave Miller [:justdave]]

(In reply to Nick Alexander :nalexander from comment #26)
> justdave: Would you be willing to pull your browser.db file using
> https://addons.mozilla.org/en-US/android/addon/copy-profile and share parts
> of it privately?

Sorry, I didn't see this until just now, but it looks like you already figured it out already anyway.