Closed Bug 1351104 Opened 8 years ago Closed 7 years ago

[meta] Use the FxA device list to filter stale clients

Categories

(Firefox for Android Graveyard :: Android Sync, enhancement, P1)

Product:
Firefox for Android Graveyard
Component:
Android Sync
Type:
enhancement

Tracking

(firefox57 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: eoger, Assigned: eoger)

References

Details

Attachments

(4 files, 4 obsolete files)

Bug 1351104 part 1 - Store the fxaDeviceId in the Clients database.
59 bytes, text/x-review-board-request
Grisha
: review+
Details
Bug 1351104 part 2 - Filter the clients list in Send Tab with the FxA device list.
59 bytes, text/x-review-board-request
Grisha
: review+
Details
Bug 1351104 part 3 - Refresh the FxA device list on client disconnected push notification.
59 bytes, text/x-review-board-request
Grisha
: review+
Details
Bug 1351104 part 4 - Sync the clients collection after an update.
59 bytes, text/x-review-board-request
Grisha
: review+
Details
Basically we need to do what we did on desktop in bug 1339861. Once per launch, fetch the FxA list, compare it to our clients list, and build a map of known stale clients. The only problem is, since Sync is background service on Android I'm not sure how we would determine what's a "launch" (we could fetch it regularly on clients collection sync every 2-3 days).
It might be better to store a list of FxA devices. If you have a browser.db table for current FxA devices, and you keep it up-to-date via push and periodic pulls (once per launch or what-have-you), you can easily modify queries which build up client lists for the History panel and for Send Tab UI to perform the necessary filtering out of stale clients. Those queries should be fast enough, and much more straightforward than what we currently have. You proposed figuring out and persisting the list of stale clients. If you're going to persist that list to SQL, you might as well just do the above. You'll have the same query performance, and you'll eat the costs of having to add a table and write a migration, but the end result won't be as generally useful. If you're going to persist it as a SharedPref or a json file, you would have gone most of the way there, but won't gain any benefits from having the filtering and querying simply done in SQL. And I bet it'll be slower and clumsier all around. So my rough proposal: - add fxadevices table - modify clients table to store fxadeviceid, unless it already has one - figure out migrations for the above... - update history & send tab and whatever other queries in BrowserProvider to do the "stale client" filtering in SQL - keep fxadevices table up-to-date via push events and periodic manual pulls.
Going home, just posting what I said on our IRC conversation: [19:11:00] <eoger> what makes me worry is the last item [19:11:07] <eoger> what if our FxA list is not up to date? [19:11:14] <eoger> we're gonna hide devices that are not supposed to [19:13:05] <eoger> that's what I did first in my desktop patch, then markh convinced me to do the opposite, store only the stales one, so if push fails we're not doing a really bad thing
The simple (if a bit "hammer" like) solution to this is to fetch the FxA devices list on every sync, in addition to maintaining it via push. From a client's point of view that seems non-ideal but acceptable if it's providing us with enough benefit. Your radio is already on, you're doing bunch of requests already, etc. - throwing another into the mix doesn't seem terrible. It does add additional complexity around "what if request fails, etc" - but you'll have to deal with that regardless. From a server's p.o.v., it's a read-only request, right? My guess is that it should be fine to drive those up permanently. Perhaps Ryan can clarify. The above is assuming we can't rely purely on push to drive this list. But we _do_ pull after receiving corresponding push messages. If the eventual goal is to move towards the FxA devices list driving a lot of our UI - right now it's the History panel and the Send Tab ui - it doesn't seem like we can get around the fact that we need to maintain that list locally. And if I say this quickly, once you're maintaining the FxA devices list locally you're moving along the path towards phasing out the clients collection.
I am thrilled to see this migration from the Sync clients collection to the FxA devices list happening. Woot! Long term, we're _definitely not_ going to be storing {Sync clients} - {known stale client GUIDs from FxA}, so I'd like to see whatever API we expose in Fennec be {known FxA clients} centric -- even if the initial implementation of the store represents them differently. This argues for expanding or augmenting https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md#get-v1accountdevices with a more Sync- or Kinto-like API allowing to quickly 204 unchanged device collections, make conditional queries, etc. To Grisha's point: > The above is assuming we can't rely purely on push to drive this list. But we _do_ pull after receiving corresponding push messages. I'd prefer not to rely solely on Push, if we can help it. There might be an opportunity to meet user expectations here and solve some of our hard scheduling problems, though: allow the user to pull-to-refresh the device list wherever we display it! I think one of the key lessons from Sync is that explicit user control _augmented_ with some very simple background magic outperforms even the darkest, most wondrous background magic.
(In reply to Nick Alexander :nalexander from comment #4) > This argues for expanding or augmenting > https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md#get- > v1accountdevices with a more Sync- or Kinto-like API allowing to quickly 204 > unchanged device collections, make conditional queries, etc. It does! No sense in wasting bandwidth/"radio-is-on" time on devices. We rely on last-modified timestamps for skipping Sync fetches. We could do something similar here. If we treat a local copy of the FxA device-list as a cache on the client, using something like an ETag approach also makes sense. Either way if we can somehow get back a 304 when nothing changed, that's progress. > I'd prefer not to rely solely on Push, if we can help it. There might be an > opportunity to meet user expectations here and solve some of our hard > scheduling problems, though: allow the user to pull-to-refresh the device > list wherever we display it! I think one of the key lessons from Sync is > that explicit user control _augmented_ with some very simple background > magic outperforms even the darkest, most wondrous background magic. That is an excellent point. Our UX already supports this in some places (displaying synced history), but not all. Specifically, we expose a device list in: - History -> synced devices -- "there are other clients" state has "pull-to-refresh" -- "there are no other clients" - needs adjustment, atm there's no pull-to-refresh and it displays "Welcome to Sync" message - Send Tab overlay UI - needs adjustment to allow refreshing the list - Send Tab pop-up UI - needs adjustment to allow refreshing the list - FxA-served device management page, which is up-to-date at the time of loading and has a "Refresh" button. I'm going to file a collection of bugs for these changes.
> I'm going to file a collection of bugs for these changes. Let's get some UX feedback (here and on Desktop) before we file too many tickets :)
Depends on: 1351481
(In reply to Nick Alexander :nalexander from comment #6) > Let's get some UX feedback (here and on Desktop) before we file too many > tickets :) Yup, that will be the goal of those tickets :-)
See Also: → 1351484
(In reply to :Grisha Kruglov from comment #3) > The simple (if a bit "hammer" like) solution to this is to fetch the FxA > devices list on every sync, in addition to maintaining it via push. FWIW, desktop chose to take the simple approach because until the FxA device list becomes canonical, the list of stale devices seems quite unlikely to change during a browser session - and even when it does, the user impact is relatively low. I agree it's not ideal, so we certainly should reconsider the desktop implementation. > If the eventual goal is to move towards the FxA devices list driving a lot > of our UI - right now it's the History panel and the Send Tab ui - it > doesn't seem like we can get around the fact that we need to maintain that > list locally. > > And if I say this quickly, once you're maintaining the FxA devices list > locally you're moving along the path towards phasing out the clients > collection. I'm not convinced phasing out the clients collection is worthwhile - eg, we'd need to reinvent commands etc. I wonder if there's a middle ground - something like the FxA device ID holding the canonical metadata (ie, ID, name, etc) for the device, with the existing clients collection being stripped down to information which doesn't really make sense on the FxA record (eg, commands)?
Depends on: 1351805
Summary: Use the FxA device list to filter stale clients → [meta] Use the FxA device list to filter stale clients
Priority: -- → P3
Assignee: nobody → eoger
Status: NEW → ASSIGNED
Priority: P3 → P1
Comment on attachment 8875439 [details] Bug 1351104 part 2 - Filter the clients list in Send Tab with the FxA device list. https://reviewboard.mozilla.org/r/146880/#review152554 r-, primarily because I think if we can make this a bit faster/better without too much effort, we should. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/devices/FxAccountDevice.java:82 (Diff revision 1) > body.put(JSON_KEY_PUSH_AUTHKEY, this.pushAuthKey); > } > return body; > } > > + public static FxAccountDevice fromRemoteDeviceCursor(Cursor cursor) { Probably not necessary. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/devices/FxAccountDevice.java:100 (Diff revision 1) > private String type; > private String pushCallback; > private String pushPublicKey; > private String pushAuthKey; > > - public void id(String id) { > + public Builder id(String id) { These changes aren't necessary, right? I don't think you're actually using the builder in these patches, or changing how it's used. ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/ClientsDatabaseAccessor.java:86 (Diff revision 1) > cur.close(); > } > } > > + // Filters our list of clients with the device list we have from FxA. > + public Collection<ClientRecord> fetchNonStaleClients(Context context) throws NullCursorException { Ugh, this is so annoying. If this all was in a single database, we could have just reduced this code to a simple sql query. ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/ClientsDatabaseAccessor.java:87 (Diff revision 1) > } > } > > + // Filters our list of clients with the device list we have from FxA. > + public Collection<ClientRecord> fetchNonStaleClients(Context context) throws NullCursorException { > + final Collection<ClientRecord> allClients = this.fetchAllClients().values(); I think a better (faster) approach would be to implement a ClientsDatabase accessor method which takes in a list of FxA Device IDs, and returns a list of clients whose ID is present in the given list. E.g. SELECT * FROM clients WHERE guid IN [provided list]; This method then becomes just two SQL queries, instead of nested loops.
Attachment #8875439 - Flags: review?(gkruglov) → review-
Comment on attachment 8875438 [details] Bug 1351104 part 1 - Store the fxaDeviceId in the Clients database. https://reviewboard.mozilla.org/r/146878/#review152572 ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/ClientsDatabase.java:120 (Diff revision 1) > db.execSQL("ALTER TABLE " + TBL_CLIENTS + " ADD COLUMN " + COL_APP_PACKAGE + " TEXT"); > db.execSQL("ALTER TABLE " + TBL_CLIENTS + " ADD COLUMN " + COL_DEVICE + " TEXT"); > } > + > + if (newVersion >= 4) { > + db.execSQL("ALTER TABLE " + TBL_CLIENTS + " ADD COLUMN " + COL_FXA_DEVICE_ID + " TEXT"); You're going to be querying by the fxa device id in the next patch. Perhaps consider adding an index on this field?
Attachment #8875438 - Flags: review?(gkruglov) → review+
Comment on attachment 8875440 [details] Bug 1351104 part 3 - Refresh the FxA device list on client disconnected push notification. https://reviewboard.mozilla.org/r/146882/#review152580 ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountPushHandler.java:101 (Diff revision 1) > Log.e(LOG_TAG, "The account does not exist anymore"); > return; > } > final AndroidFxAccount fxAccount = new AndroidFxAccount(context, account); > if (!fxAccount.getDeviceId().equals(data.getString("id"))) { > + Log.i(LOG_TAG, "Another device in the account got disconnected, refreshing FxA device list."); Add a comment describing _why_ you're doing this. Something like "We filter our clients list by the list of FxA devices, and so it's necessary to fetch an updated devices list so that our clients list is up-to-date in the UI".
Attachment #8875440 - Flags: review?(gkruglov) → review+
Comment on attachment 8875438 [details] Bug 1351104 part 1 - Store the fxaDeviceId in the Clients database. https://reviewboard.mozilla.org/r/146878/#review152582 After an application update, and before a sync completes, our clients database won't have any fxa device Ids for any of the rows, and so our filtered lists will be empty. Is that a correct reading of these changes? If so, I think this might be worth considering explicitely. Since we didn't look far ahead when adding a RemoteDevices table, I don't think we can populate clients db rows with fxaids using just the information we have locally. Which leaves us with two options: - trigger a sync of the clients collection right after an app update, in hopes that it will succeed and populate our rows. - don't perform filtering _until_ a sync succeeds. This would likely involve adding a SharedPreference which you read every time filtering is required, and toggle at the end of a sync... Which seems very hacky, but should achieve the desired outcome and we can remove it entirely after one update cycle. What do you think?
Depends on: 1372655
Comment on attachment 8875439 [details] Bug 1351104 part 2 - Filter the clients list in Send Tab with the FxA device list. https://reviewboard.mozilla.org/r/146880/#review160978 This looks good, sans a few comments. Could you please add tests? r- so that I look at the response. ::: mobile/android/base/java/org/mozilla/gecko/overlays/service/sharemethods/SendTab.java:248 (Diff revision 2) > - if (remoteTabsCursor.getCount() == 0) { > - return Collections.emptyList(); > + final Collection<ClientRecord> clientRecords = clientsDatabaseAccessor.fetchNonStaleClients(context); > + final Collection<RemoteClient> remoteClients = new ArrayList<>(clientRecords.size()); > + for (ClientRecord cr : clientRecords) { > + remoteClients.add(new RemoteClient(cr.guid, cr.name, cr.lastModified, cr.type)); > } > - return tabsAccessor.getClientsWithoutTabsNoStaleSortedFromCursor(remoteTabsCursor); > + return remoteClients; Do we still need getClientsWithoutTabsNoStale.... stuff? ::: mobile/android/base/java/org/mozilla/gecko/widget/ActivityChooserModel.java:1320 (Diff revision 2) > - if (remoteClientsCursor == null) { > - return false; > - } > - > try { > - return remoteClientsCursor.getCount() > 0; > + final Collection<ClientRecord> clientRecords = clientsDatabaseAccessor.fetchNonStaleClients(mContext.getApplicationContext()); This seems rather wasteful. Could you implement a lower overhead "get count" method instead? Alternatively, will every consumer of hasOtherSyncClients use the data set after checking? Can we refactor the consumers to instead just fetch a lit of non-stale clients? ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/devices/FxAccountDevice.java:7 (Diff revision 2) > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > package org.mozilla.gecko.fxa.devices; > > +import android.database.Cursor; Unused imports? ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/devices/FxAccountDevice.java:90 (Diff revision 2) > private String type; > private String pushCallback; > private String pushPublicKey; > private String pushAuthKey; > > - public void id(String id) { > + public Builder id(String id) { You don't seem to use these changes here, but maybe later in the patch series? If the changes are unused, consider dropping them. ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/ClientsDatabaseAccessor.java:88 (Diff revision 2) > cur.close(); > } > } > > + // Filters our list of clients with the device list we have from FxA. > + public Collection<ClientRecord> fetchNonStaleClients(Context context) throws NullCursorException { This looks fine, but could you please write unit tests for this to define your desired behaviour?
Attachment #8875439 - Flags: review?(gkruglov) → review-
Comment on attachment 8877297 [details] Bug 1351104 part 4 - Sync the clients collection after an update. https://reviewboard.mozilla.org/r/148626/#review160980 ::: mobile/android/base/java/org/mozilla/gecko/PackageReplacedReceiver.java:33 (Diff revision 1) > } > > // Load new Gecko libs to extract them to cache. > context.startService(GeckoService.getIntentToLoadLibs(context)); > + > + // Remove this when we reach version 60ish. Can you file a bug for this? ::: mobile/android/base/java/org/mozilla/gecko/PackageReplacedReceiver.java:35 (Diff revision 1) > // Load new Gecko libs to extract them to cache. > context.startService(GeckoService.getIntentToLoadLibs(context)); > + > + // Remove this when we reach version 60ish. > + // Do a clients collection sync after update in order to populate FxA device ids. > + // See https://bugzilla.mozilla.org/show_bug.cgi?id=1351104#c15 Thanks for linking the comment, I forgot the context for this by now! ::: mobile/android/base/java/org/mozilla/gecko/PackageReplacedReceiver.java:36 (Diff revision 1) > context.startService(GeckoService.getIntentToLoadLibs(context)); > + > + // Remove this when we reach version 60ish. > + // Do a clients collection sync after update in order to populate FxA device ids. > + // See https://bugzilla.mozilla.org/show_bug.cgi?id=1351104#c15 > + doSync(context); This seems like what we want to do, but I'm unclear on what else might be going on when this intent fires. I'm going to assume that it's OK to trigger a sync here, and I'm somewhat unsure how to test this in a realistic fashion. You _could_ trigger this intent manually to test, but that won't be terribly helpful since the context (as in, what else is happening) won't be correct.
Attachment #8877297 - Flags: review?(gkruglov) → review+
Does https://reviewboard.mozilla.org/r/148626/diff/#index_header raise any flags for you?
Flags: needinfo?(s.kaspari)
Comments addressed. I also changed fetchNonStaleClients following your comment in comment 12, since it is much easier to test.
Comment on attachment 8877297 [details] Bug 1351104 part 4 - Sync the clients collection after an update. https://reviewboard.mozilla.org/r/148626/#review162372 ::: mobile/android/base/java/org/mozilla/gecko/PackageReplacedReceiver.java:33 (Diff revision 2) > } > > // Load new Gecko libs to extract them to cache. > context.startService(GeckoService.getIntentToLoadLibs(context)); > + > + // Remove this when we reach version 60ish. We have FxAccountUpgradeReceiver, which is probably a better fit for this?
(In reply to :Grisha Kruglov from comment #24) > Does https://reviewboard.mozilla.org/r/148626/diff/#index_header raise any > flags for you? No, not really. But I'm not at all familiar with that part of the code. Is there something specific you wanted me to look at?
Flags: needinfo?(s.kaspari)
Blocks: 1386016
Comment on attachment 8875439 [details] Bug 1351104 part 2 - Filter the clients list in Send Tab with the FxA device list. https://reviewboard.mozilla.org/r/146880/#review168536 New revision looks all right, so I'm going to trust myself from 21 days ago and r+ this. Apologies for the delay! ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/ClientsDatabase.java:255 (Diff revision 4) > return queryHelper.safeQuery(db, ".fetchAllClients", TBL_CLIENTS, TBL_CLIENTS_COLUMNS, null, null); > } > > + public Cursor fetchClientsWithFxADeviceIds(String[] fxaDeviceIds) throws NullCursorException { > + String inClause = DBUtils.computeSQLInClause(fxaDeviceIds.length, COL_FXA_DEVICE_ID); > + String query = inClause + " OR " + COL_FXA_DEVICE_ID + " IS NULL"; I hope we don't need to worry about SQLITE_MAX_VARIABLE limit here :) What are the chances someone will have more than 999 fxa device ids? ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/ClientsDatabaseAccessor.java:122 (Diff revision 4) > + } > + } > + > + public String[] getRemoteDevicesIds(Context context) { > + final ContentResolver cr = context.getContentResolver(); > + final Cursor c = cr.query(BrowserContract.RemoteDevices.CONTENT_URI, null, null, null, "NAME ASC"); Surely you want to define a [GUID] projection here? "Passing null will return all columns, which is inefficient." ::: mobile/android/tests/background/junit3/src/org/mozilla/gecko/background/db/TestClientsDatabaseAccessor.java:130 (Diff revision 4) > thrown = true; > } > assertTrue(thrown); > } > + > + public void testFetchNonStaleClients() throws NullCursorException { These could be off-device junit4 tests...
Attachment #8875439 - Flags: review?(gkruglov) → review+
Comment on attachment 8875439 [details] Bug 1351104 part 2 - Filter the clients list in Send Tab with the FxA device list. https://reviewboard.mozilla.org/r/146880/#review168888 ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/ClientsDatabaseAccessor.java:130 (Diff revisions 4 - 5) > + final Cursor c = cr.query(BrowserContract.RemoteDevices.CONTENT_URI, guidProjection, null, null, "NAME ASC"); > final String[] remoteDevicesIds = new String[c.getCount()]; > try { > int i = 0; > while (c.moveToNext()) { > - remoteDevicesIds[i] = RepoUtils.getStringFromCursor(c, BrowserContract.RemoteDevices.GUID); > + remoteDevicesIds[i] = c.getString(0); For consistency with the rest of the codebase, and to avoid weird bugs in the wild, prefer cursor.getColumnIndexOrThrow
Comment on attachment 8892548 [details] Bug 1351104 part 1 - Store the fxaDeviceId in the Clients database. https://reviewboard.mozilla.org/r/163544/#review168896
Attachment #8892548 - Flags: review?(gkruglov) → review+
Comment on attachment 8892549 [details] Bug 1351104 part 2 - Filter the clients list in Send Tab with the FxA device list. https://reviewboard.mozilla.org/r/163546/#review168898
Attachment #8892549 - Flags: review?(gkruglov) → review+
Comment on attachment 8892550 [details] Bug 1351104 part 3 - Refresh the FxA device list on client disconnected push notification. https://reviewboard.mozilla.org/r/163548/#review168900
Attachment #8892550 - Flags: review?(gkruglov) → review+
Comment on attachment 8892551 [details] Bug 1351104 part 4 - Sync the clients collection after an update. https://reviewboard.mozilla.org/r/163550/#review168902
Attachment #8892551 - Flags: review?(gkruglov) → review+
Attachment #8875438 - Attachment is obsolete: true
Attachment #8875439 - Attachment is obsolete: true
Attachment #8875440 - Attachment is obsolete: true
Attachment #8877297 - Attachment is obsolete: true
Pushed by eoger@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/24cdf9875b5b part 1 - Store the fxaDeviceId in the Clients database. r=Grisha https://hg.mozilla.org/integration/autoland/rev/ce4d65ff8165 part 2 - Filter the clients list in Send Tab with the FxA device list. r=Grisha https://hg.mozilla.org/integration/autoland/rev/c80b721e4dd7 part 3 - Refresh the FxA device list on client disconnected push notification. r=Grisha https://hg.mozilla.org/integration/autoland/rev/1514c01c63cb part 4 - Sync the clients collection after an update. r=Grisha
Depends on: 1387053
Depends on: 1387113
Product: Android Background Services → Firefox for Android
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: