1145896 - ClientsDatabaseAccessor contains stale data

Status: RESOLVED FIXED

(Android Background Services Graveyard :: Firefox Accounts, defect)

Description

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

I'm not sure of exact STR, but I had sync enabled with multiple synced devices, removed the Firefox Account from the device, and ClientsDatabaseAccessor.clientsCount() returned 16 (this is in the context of bug 1122302, which uses ClientsDatabaseAccessor).

Comment 1

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

This caused bug 1145897 (but I'm going to use a workaround there).

Comment 2

[Nick Alexander :nalexander [he/him]]

NI to me to complete mentor information.

Comment 3

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #2)
> NI to me to complete mentor information.

What this ticket needs is the background services databases cleaned when Firefox Accounts are removed.  That will mean adding, around [1], code that cleans the databases created at [2] and [3].

AndroidBrowserHistoryDataExtender looks like it doesn't track profile or account at all; we should wipe the table entirely when any account is removed.  (And trust that there's only ever one account at any time.)

Clients database tracks profile, but not really Account.  We should wipe the tables entirely when any account is removed here, as well.

[1] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/fxa/receivers/FxAccountDeletedService.java#74

[2] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/sync/repositories/android/AndroidBrowserHistoryDataExtender.java

[3] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/sync/repositories/android/ClientsDatabase.java

Comment 4

[Nick Alexander :nalexander [he/him]]

To test this, connect to a Firefox Account, make sure other devices connect to the account, and sync.  You should see additional devices in the Synced Tabs display and in the Send Tab list (which uses the ClientsTable, IIRC).  Remove the Account, and then connect to another one (with no devices connected).  Before fixing this ticket, you should see the old devices (from the other account); after fixing this ticket, the old devices should be purged when the account is removed.

Comment 5

[Vivek Balakrishnan[:vivek]]

Attachment: 1145896.patch

Stale data from client and historyDataExtender database flushed in AccountDeletedService.

Comment 6

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/506323d5ecc3

Comment 7

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8589754 [details] [diff] [review]
1145896.patch

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

Looks good.  I landed with some try/catch blocks.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/506323d5ecc3

Comment 9

[Nick Alexander :nalexander [he/him]]

mcomella: here's how I tested this.  I created an FxA, synced against a few remote clients, and verified that using the Share Plane > Send to Other Device from within Fennec had a list of external clients.  Then I deleted the FxA.  I then tried the Share Plane again, but of course there is no account so there is no list of clients.  (This is expected, right?)  So I created a new FxA with no other remote clients, and verified that using the Share Plane > Send to Other Device was present (and empty).  (Fuzzy on that.)  I then added another device to the new FxA, synced, and verified that only the new device was present in the Other Device list.

Could you test your scenario and get QA involved where needed?

mcomella: on an unrelated note, all use of the Sync-specific clients database can and should transition to the TabsProvider, which is an honest ContentProvider and has not the same issues.  That's a little technical debt to dig out from; it's not urgent.

Comment 10

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

(In reply to Nick Alexander :nalexander from comment #9)
> I then tried the Share Plane again, but of course there is no
> account so there is no list of clients.  (This is expected, right?)

When you say there is no list of clients, do you mean you get an empty dialog, or do you get a "No connected devices" toast? If the latter, I filed bug 1159005 because this is not expected.

> created a new FxA with no other remote clients, and verified that using the
> Share Plane > Send to Other Device was present (and empty).  (Fuzzy on
> that.)

It should not be present with no other devices (bug 1159005 again?).

> I then added another device to the new FxA, synced, and verified
> that only the new device was present in the Other Device list.

My scenario would be the same except I would in the end add a different sync account with devices to ensure the list changes - previously, I would get the same list as the initial account.

> Could you test your scenario and get QA involved where needed?

I'll write out clearer steps and add the appropriate flags.

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

Should this take the place of bug 1132730?

Comment 11

[Nick Alexander :nalexander [he/him]]

(In reply to Michael Comella (:mcomella) from comment #10)
> (In reply to Nick Alexander :nalexander from comment #9)
> > I then tried the Share Plane again, but of course there is no
> > account so there is no list of clients.  (This is expected, right?)
> 
> When you say there is no list of clients, do you mean you get an empty
> dialog, or do you get a "No connected devices" toast? If the latter, I filed
> bug 1159005 because this is not expected.

Empty dialog.

> > created a new FxA with no other remote clients, and verified that using the
> > Share Plane > Send to Other Device was present (and empty).  (Fuzzy on
> > that.)
> 
> It should not be present with no other devices (bug 1159005 again?).

Not clear.
 
> > I then added another device to the new FxA, synced, and verified
> > that only the new device was present in the Other Device list.
> 
> My scenario would be the same except I would in the end add a different sync
> account with devices to ensure the list changes - previously, I would get
> the same list as the initial account.
> 
> > Could you test your scenario and get QA involved where needed?
> 
> I'll write out clearer steps and add the appropriate flags.
> 
> > mcomella: on an unrelated note, all use of the Sync-specific clients
> > database can and should transition to the TabsProvider
> 
> Should this take the place of bug 1132730?

No, just making sure you're aware.  The more we build on top of Sync's internal databases, the harder the technical debt is to pay off in the future.

Comment 12

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

STR:
* Prepare two sync accounts - each one with a different list of synced devices (so you can tell the difference between them)
* Connect the browser to one account - verify the "Send to other devices" menu appears with the devices you'd expect. You may need to wait for (or force) the device to sync before this menu item appears. 
* Remove the account. Verify the "Send to other devices" menu item does not appear or if it does (bug 1159005), it shows a toast rather than showing the previous sync account's device list.
*  Add the second account. Verify "Send to other devices" appears and has a different device list from the first account.

For me, the quick share menu state keeps the "Send to other devices" icon even after I remove the account, however, when resolving the activity, it skips it (thus choosing a different activity). The full share menu did not have this problem. I'm not sure how to repro indepedent of bug 1159005 so I did not file.

Comment 13

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

(In reply to Nick Alexander :nalexander from comment #11)
> Empty dialog.

Filed bug 1159019.

> > > created a new FxA with no other remote clients, and verified that using the
> > > Share Plane > Send to Other Device was present (and empty).  (Fuzzy on
> > > that.)
> > 
> > It should not be present with no other devices (bug 1159005 again?).
> 
> Not clear.

The "Send to other devices" menu item should not be present if you have no other devices connected to your sync account.

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

Filed bug 1159020.