Closed Bug 1351805 Opened 7 years ago Closed 7 years ago

Pull FxA Devices on sync

Categories

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

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

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: eoger, Assigned: eoger)

References

Details

Attachments

(3 files)

Bug 1351805 part 1 - Create a org.mozilla.gecko.fxa.devices package.
59 bytes, text/x-review-board-request
Grisha
: review+
Details
Bug 1351805 part 2 - Add remote devices to the Browser Provider.
59 bytes, text/x-review-board-request
Grisha
: review+
Details
Bug 1351805 part 3 - Refresh the remote devices list on Married/Engaged states.
59 bytes, text/x-review-board-request
Grisha
: review+
Details 
In order to support bug 1351104, we need an up-to-date list of the FxA devices in our account.
Let's implement this by adding an abstract FxA Devices sync stage.
Blocks: 1351104
Comment on attachment 8852622 [details]
Bug 1351805 part 1 - Create a org.mozilla.gecko.fxa.devices package.

This is a fairly naive implementation, we replace the old device list by a new one each Sync. The fxa_devices table is stored in browser.db.
Attachment #8852622 - Flags: feedback?(nalexander)
Attachment #8852622 - Flags: feedback?(gkruglov)
Comment on attachment 8852622 [details]
Bug 1351805 part 1 - Create a org.mozilla.gecko.fxa.devices package.

https://reviewboard.mozilla.org/r/124812/#review127364

Break these patches into two parts:

1) DB and Provider gubbins;
2) then FxA state machine stages.

However, I don't think your approach makes sense long term.  Right now, FxA only serves the Sync master, so it is perhaps reasonable to tie the FxA device list to the Sync apparatus.  However, future services will _not_ run through Sync; they'll be driven by FxA.  It would therefore be possible to have a _healthy_ account that doesn't really advance to `Married` -- perhaps it's an account not intended for Sync, so it'll _never_ have the FxA key material -- but would manage OAuth tokens needed to connect to a service, and could update and maintain the FxA device list.

The FxA profile integration is somewhat like this: your profile should be connected and working even before you verify your account.  (It might be that Fennec or the fxa-content-server won't let you manage it before you verify, though.)

I think it makes more sense to bake the device management into the FxALoginStateMachine, in some way.  Perhaps it's a final step on the way to finishing the state machine that tries to keep things fresh from any state that provides a `sessionToken`?  This will require non-trivial changes to the state machine, but that's okay...

In addition, what's the story for updating the current device's information?  Do you expect that to be via the fxa-content-server, so you can assume that you don't need to propogate changes _from_ the local datastore (made when this device has no connectivity, say) to the remote source of truth?  If that's true, your syncing/replication needs remain as simple as they are in this patch.  If that's not true (it's not true for the Sync client information; you can modify the device name offline) then you need to plan for tracking those types of changes and pushing them to the remote datastore.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserContract.java:463
(Diff revision 1)
>          public static final String LAST_MODIFIED = "last_modified";
>  
>          public static final String DEVICE_TYPE = "device_type";
>      }
>  
> +    public static final class FxAccountsDevices implements CommonColumns {

I have a slight preference for not baking "fxa" or "fxaccounts" into the title.  I'd prefer something like "devices" or "remote_devices", since this could be managed in many different ways in the future.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:244
(Diff revision 1)
>      }
>  
> +    private void createFxADevicesTable(SQLiteDatabase db) {
> +        debug("Creating " + TABLE_FXA_DEVICES + " table");
> +        db.execSQL("CREATE TABLE " + TABLE_FXA_DEVICES + "(" +
> +                BrowserContract.FxAccountsDevices._ID + " CHAR(32) PRIMARY KEY," +

The `_id` column is special in Android; it's used for `ListView` and other things.  It's best to keep `_id` as `INTEGER PRIMARY KEY AUTOINCREMENT` and to use something else (usually `guid TEXT NOT NULL UNIQUE`) as your external identifier.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:245
(Diff revision 1)
>  
> +    private void createFxADevicesTable(SQLiteDatabase db) {
> +        debug("Creating " + TABLE_FXA_DEVICES + " table");
> +        db.execSQL("CREATE TABLE " + TABLE_FXA_DEVICES + "(" +
> +                BrowserContract.FxAccountsDevices._ID + " CHAR(32) PRIMARY KEY," +
> +                BrowserContract.FxAccountsDevices.NAME + " TEXT," +

Can you assert `NOT NULL` for any of these?  If not, explain why not in comments here.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:247
(Diff revision 1)
> +        debug("Creating " + TABLE_FXA_DEVICES + " table");
> +        db.execSQL("CREATE TABLE " + TABLE_FXA_DEVICES + "(" +
> +                BrowserContract.FxAccountsDevices._ID + " CHAR(32) PRIMARY KEY," +
> +                BrowserContract.FxAccountsDevices.NAME + " TEXT," +
> +                BrowserContract.FxAccountsDevices.TYPE + " TEXT," +
> +                BrowserContract.FxAccountsDevices.IS_CURRENT_DEVICE + " INTEGER" +

Bitter experience has shown that you really, really, really want to track `created` and `modified` timestamps in all tables.  This looks ahead to incrementally syncing the set of devices.

If at all possible, argue the service into tracking _incrementing generations_ in some way, and keep the latest _generation_ here, so you have a more robust way to determine if you're really up-to-date.  That is, rather than `/devices?since=TIMESTAMP`, prefer `/devices?since=GENERATION_COUNTER`, and have the service own the generation counter.  This is all standard practice for data replication and there are many folks who can help with some of these design choices.  (Maybe Kinto even gets this better than Sync does!)

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:156
(Diff revision 1)
>      static final int ACTIVITY_STREAM_BLOCKLIST = 1400;
>  
>      static final int PAGE_METADATA = 1500;
>  
> +    static final int FXA_DEVICES = 1600;
> +    static final int FXA_DEVICE_ID = 1601;

nit: `DEVICES_ID`, just to have a consistent prefix.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:675
(Diff revision 1)
>                  break;
>  
> +            case FXA_DEVICE_ID:
> +                debug("Delete on FXA_DEVICE_ID: " + uri);
> +
> +                selection = DBUtils.concatenateWhere(selection, TABLE_FXA_DEVICES + "._id = ?");

I think you have some copy-pasta here.  Your `_ID` was `TEXT`, right?  So this will work... but there's a subtle mismatch between Android's numeric IDs and your `TEXT` IDs.

You want to support this for Android reasons, but you want `_ID` to be numeric.

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/FetchFxAccountsDevicesStage.java:48
(Diff revision 1)
> +        }
> +
> +        @Override
> +        public void handleSuccess(FxAccountDevice[] result) {
> +            if (result == null || result.length == 0) {
> +                // Something went probably wrong, because there should be at least 1 device (this one)

nit: full sentence comments, throughout!

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/FetchFxAccountsDevicesStage.java:48
(Diff revision 1)
> +        }
> +
> +        @Override
> +        public void handleSuccess(FxAccountDevice[] result) {
> +            if (result == null || result.length == 0) {
> +                // Something went probably wrong, because there should be at least 1 device (this one)

Why?  Couldn't we witness a `GET` before we `POST`, or after a failed `POST`?  Be defensive and accept as much as you can.

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/FetchFxAccountsDevicesStage.java:63
(Diff revision 1)
> +            // First purge our list of FxA devices
> +            cr.delete(fxaDevicesUriWithProfile, null, null);
> +
> +            // And insert the new list
> +            ContentValues[] toInsert = new ContentValues[result.length];
> +            for (int i = 0; i < result.length; i++) {

Consider looking ahead to the incremental future already: try to at least recognize when you're in the `UPDATE` case and not in the `INSERT` case.

Are you aware that bulk-replacing will cause the `ContentResolver` to notify any listeners that the underlying data has changed, even when it might not have?  A `ListView` that was displaying the `devices` table might churn unnecessarily.

Now, you don't _have_ to do this now -- but there's not so much work, and it will be instructive.  See my comments about time-stamping everything about each device row above.
Attachment #8852622 - Flags: review-
I don't think the FxA state machine is a good host for this functionality. It currently has a benefit of being primarily concerned with managing account state, with some tolerable side-effects (like push registration). That's nice, and I'd rather it stay as simple as possible.

A device list is a fundamentally different sort of a thing, with different requirements around liveliness. Given the state of things now, it's tightly coupled to the Sync Clients list, and so _for now_ it makes sense to update them at the same time.

I think a reasonable middle-ground approach here is to abstract out the device-list management stuff into a separate component, which we can grow over time as need arises. If we'll keep the sync stage approach, it then becomes a simple wrapper around a call into that component. It's easy to imagine other invocations of this component - we want to refresh a device list while looking at the "Send Tab" ui, for example.

As for POSTing local changes to the list - while it does seem like a reasonable thing that we might want to do in the future, let's not overthink this for now. I don't think the above approach precludes us from any one direction as we get a clearer picture as to how this stuff will evolve.
(In reply to :Grisha Kruglov from comment #4)
> I don't think the FxA state machine is a good host for this functionality.
> It currently has a benefit of being primarily concerned with managing
> account state, with some tolerable side-effects (like push registration).
> That's nice, and I'd rather it stay as simple as possible.

I disagree -- I see the details of the FxA device constellation as being critical to the Firefox Account -- but I trust your judgement.  If you think it's unwise to try to push for this now, and instead maintain the Sync-centric approach, OK.

> A device list is a fundamentally different sort of a thing, with different
> requirements around liveliness. Given the state of things now, it's tightly
> coupled to the Sync Clients list, and so _for now_ it makes sense to update
> them at the same time.

I disagree, as I say above.  The devices/ API was built into the fxa-auth-server for backend reasons -- distributing state across backend services is hard -- but those reasons are basically present in the front-end as well.

Phrased another way: could an FxA-authenticated *non-Sync* service derive value from the device list?  Would it _require_ the device list?  I think the answers are "yes" and "almost certainly yes".

> I think a reasonable middle-ground approach here is to abstract out the
> device-list management stuff into a separate component, which we can grow
> over time as need arises. If we'll keep the sync stage approach, it then
> becomes a simple wrapper around a call into that component. It's easy to
> imagine other invocations of this component - we want to refresh a device
> list while looking at the "Send Tab" ui, for example.
> 
> As for POSTing local changes to the list - while it does seem like a
> reasonable thing that we might want to do in the future, let's not overthink
> this for now. I don't think the above approach precludes us from any one
> direction as we get a clearer picture as to how this stuff will evolve.

This is a fair position, and you are the de-facto owner of this code.  Perhaps you could review my feedback and guide eoger about what you think needs to happen now, what you think can be postponed, and what you think is wrong?
Flags: needinfo?(gkruglov)
Priority: -- → P1
Apologies for the delay in responding.

Nick, thank you for phrasing the question in terms of value derived from an FxA account; that's an excellent way of looking at the problem with a horizon that's beyond our immediate needs.

You're quite correct in stating that sync is not a good long term vehicle for this.

I'll be happy with an approach which performs device fetch as a by-product of handling the final state machine state, similarly to how we perform device registration, profile fetch, and sync itself, for that matter.

eoger, do you foresee problems fetching device list asynchronously from the client list, given our immediate uses for it? If so, consider ordering these operations - that is, invoke sync _after_ fetching the device list. Given that device list doesn't change most of the time, I'd expect us to see a 304 and move on quickly - so this shouldn't be a big blocker. Perhaps enforcing a strict order of operations _is_ the simple thing to do here.

The above should generally have the same end result as making device list fetch be part of the FxA state machine, but without the added complexity. It should also give us an easy path forward in case we'd like to re-think this approach in the future and move things around.
Flags: needinfo?(gkruglov)
Comment on attachment 8852622 [details]
Bug 1351805 part 1 - Create a org.mozilla.gecko.fxa.devices package.

Clearing feedback flags for now. Nick's review about coupled with the comments above should be enough to get a second iteration of this.

BTW, Edouard, thanks for jumping on this!
Attachment #8852622 - Flags: feedback?(nalexander)
Attachment #8852622 - Flags: feedback?(gkruglov)
Patch amended with both of your comments.
I decided to keep this simple on purpose, which means for now no update/timestamp mechanisms.
Until we know what will be the caching implementation on the server side, let's not make any assumptions and play it dumb (drop everything and bulk insert).
Once we know more, let's open a follow-up bug.
Comment on attachment 8852622 [details]
Bug 1351805 part 1 - Create a org.mozilla.gecko.fxa.devices package.

https://reviewboard.mozilla.org/r/124812/#review129962

This is getting close.  Please split the DB/ContentProvider pieces out from the Sync/FxAClient pieces, for ease of iteration, and then off to Grisha for his take.

I'm marking this r+ but I'd like to take another look before landing.  Great work!

::: mobile/android/base/android-services.mozbuild:836
(Diff revision 2)
>      'fxa/authenticator/FxAccountLoginDelegate.java',
>      'fxa/authenticator/FxAccountLoginException.java',
>      'fxa/authenticator/FxADefaultLoginStateMachineDelegate.java',
>      'fxa/FirefoxAccounts.java',
>      'fxa/FxAccountConstants.java',
>      'fxa/FxAccountDevice.java',

Next time we do something large-ish here, let's push a new package: `fxa/devices/*`.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserContract.java:472
(Diff revision 2)
> +        public static final String GUID = "guid"; // FxA device ID
> +        public static final String NAME = "name";
> +        public static final String TYPE = "type";
> +        public static final String IS_CURRENT_DEVICE = "is_current_device";
> +
> +        public static final Uri CONTENT_URI = Uri.withAppendedPath(AUTHORITY_URI, "remote_devices");

I know you're trying to simplify, but I _really_ believe that not tracking created/updated timestamps will come back to bite you as soon as you want to actually use this list.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/sync/FxAccountSyncAdapter.java:594
(Diff revision 2)
>                Logger.info(LOG_TAG, "Not syncing (token server).");
>                syncDelegate.postponeSync(tokenBackoffHandler.delayMilliseconds());
>                return;
>              }
>  
> +            FxAccountDeviceListUpdater deviceListUpdater = new FxAccountDeviceListUpdater(context);

Hmm, I thought I argued for doing this regardless of whether we got to `Married`, but I see that was for a different ticket.  (Or I am hallucinating.)

Per our discussions of `getSessionToken`, I think you might want to refresh the remote device list in more states; for example, do it in `handleFinal` or somewhere more general than `handleMarried`.  See https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/FxADefaultLoginStateMachineDelegate.java#71.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/sync/FxAccountSyncAdapter.java:596
(Diff revision 2)
>                return;
>              }
>  
> +            FxAccountDeviceListUpdater deviceListUpdater = new FxAccountDeviceListUpdater(context);
> +            deviceListUpdater.update();
> +            // This will block until done for simplicity reasons: we need an updated list to

nit: Put the comment above the code, not after it.  "simplicity reasons" is a little awkward, consider: "Since the clients stage requires a fresh list of remote devices, we update the device list synchronously."
Attachment #8852622 - Flags: review?(nalexander) → review+
Comment on attachment 8852622 [details]
Bug 1351805 part 1 - Create a org.mozilla.gecko.fxa.devices package.

https://reviewboard.mozilla.org/r/124812/#review129992

This is looking good! General feedback:
- tests, please. Consider BrowserProvider crud tests optional, but do write them for the updater
- some light refactoring to make writing tests easier
- consider running the updater in `handleFinal`, and let it decide if it may proceed (has a token or not..)

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserContract.java:472
(Diff revision 2)
> +        public static final String GUID = "guid"; // FxA device ID
> +        public static final String NAME = "name";
> +        public static final String TYPE = "type";
> +        public static final String IS_CURRENT_DEVICE = "is_current_device";
> +
> +        public static final Uri CONTENT_URI = Uri.withAppendedPath(AUTHORITY_URI, "remote_devices");

I agree with Nick, let's add the timestamps here. A lot of things might go wrong, and so it might be helpful to validate the assumption that just because you have the list and attempt to fetch it frequently, it's actually recent.
And there might be UI uses for timestamps as well which we haven't considered.

Once we consider making changes to these lists, I'm hopeful that instead of timestamps we'll be using generation numbers or something similar when talking to the API in the future. That migration, if necessary, won't be difficult to write when the time comes.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:250
(Diff revision 2)
> +                RemoteDevices.GUID + " TEXT UNIQUE NOT NULL," +
> +                RemoteDevices.NAME + " TEXT NOT NULL," +
> +                RemoteDevices.TYPE + " TEXT NOT NULL," +
> +                RemoteDevices.IS_CURRENT_DEVICE + " INTEGER" +
> +                ");");
> +    }

Consider usage patterns for this data. What indecies might be helpful?

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:2139
(Diff revision 2)
>                  case 36:
>                      upgradeDatabaseFrom35to36(db);
>                      break;
> +
> +                case 37:
> +                    upgradeDatabaseFrom36to37(db);

You'll need to add a new test database file as well.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:670
(Diff revision 2)
>              case PAGE_METADATA:
>                  trace("Delete on PAGE_METADATA: " + uri);
>                  deleted = deletePageMetadata(uri, selection, selectionArgs);
>                  break;
>  
> +            case REMOTE_DEVICES_ID:

Please consider writing at least basic CRUD unit tests for this.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:673
(Diff revision 2)
>                  break;
>  
> +            case REMOTE_DEVICES_ID:
> +                debug("Delete on REMOTE_DEVICES_ID: " + uri);
> +
> +                selection = DBUtils.concatenateWhere(selection, TABLE_REMOTE_DEVICES + "._id = ?");

`RemoteDevices._ID`?

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceListUpdater.java:30
(Diff revision 2)
> +
> +    private Context context;
> +    private String profileName;
> +
> +    public FxAccountDeviceListUpdater(Context context) {
> +        this.context = context.getApplicationContext();

It's better to have this class be constructed with `AndroidFxAccount` and the `ContentResolver`. Don't pass in context, it's a bit rich and you don't need it here.

Once you do that, with some light reshuffling you'll be able to easily unit test this!

As you should, please write tests for the delegate handlers (well, the success one for now), and for the update method. I suggest mocking and verifying calls to the FxA client object (see how we use mockito in other tests).

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceListUpdater.java:42
(Diff revision 2)
> +                .appendQueryParameter(BrowserContract.PARAM_PROFILE, profileName)
> +                .build();
> +        ContentResolver cr = context.getContentResolver();
> +
> +        // First purge our list of FxA devices
> +        cr.delete(remoteDevicesUriWithProfile, null, null);

See if you can somehow wrap the delete and inserts into one transaction.

But I'm not sure you'll be able to do that without using the `call` interface and writing a custom handler on the other side.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceListUpdater.java:42
(Diff revision 2)
> +                .appendQueryParameter(BrowserContract.PARAM_PROFILE, profileName)
> +                .build();
> +        ContentResolver cr = context.getContentResolver();
> +
> +        // First purge our list of FxA devices
> +        cr.delete(remoteDevicesUriWithProfile, null, null);

Double check that empty result always means "no fxa on the server for this account". Otherwise we'll wipe our local device list and won't repopulate it with anything.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceListUpdater.java:45
(Diff revision 2)
> +
> +        // First purge our list of FxA devices
> +        cr.delete(remoteDevicesUriWithProfile, null, null);
> +
> +        // And insert the new list
> +        ContentValues[] toInsert = new ContentValues[result.length];

final

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceListUpdater.java:68
(Diff revision 2)
> +    @Override
> +    public void handleFailure(FxAccountClientException.FxAccountClientRemoteException e) {
> +        onError(e);
> +    }
> +
> +    private void onError(final Exception e) {

Some of these errors are more interesting than others.

401, for example... At least we log them. We _could_ be emitting events to the state machine. Not something to worry about for now.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceListUpdater.java:69
(Diff revision 2)
> +    public void handleFailure(FxAccountClientException.FxAccountClientRemoteException e) {
> +        onError(e);
> +    }
> +
> +    private void onError(final Exception e) {
> +        Log.e(LOG_TAG, "Error while getting the FxA device list: ", e);

nit: "[...] list.", drop the collon

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/sync/FxAccountSyncAdapter.java:594
(Diff revision 2)
>                Logger.info(LOG_TAG, "Not syncing (token server).");
>                syncDelegate.postponeSync(tokenBackoffHandler.delayMilliseconds());
>                return;
>              }
>  
> +            FxAccountDeviceListUpdater deviceListUpdater = new FxAccountDeviceListUpdater(context);

I'd say fetch the list whenever you can get a sessionToken. `handleFinal` seems like a fine place; perhaps your updater class makes the "should i fetch" decision, as well.
Attachment #8852622 - Flags: review?(gkruglov) → review-
Comment on attachment 8852622 [details]
Bug 1351805 part 1 - Create a org.mozilla.gecko.fxa.devices package.

Woops, need to split in 2 commits
Attachment #8852622 - Flags: review?(gkruglov)
Comment on attachment 8855849 [details]
Bug 1351805 part 2 - Add remote devices to the Browser Provider.

https://reviewboard.mozilla.org/r/127732/#review131044

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserContract.java:471
(Diff revision 1)
> +
> +        public static final String GUID = "guid"; // FxA device ID
> +        public static final String NAME = "name";
> +        public static final String TYPE = "type";
> +        public static final String IS_CURRENT_DEVICE = "is_current_device";
> +        public static final String DATE_CREATED = "date_created";

I'd just re-use DateSyncColumns. Or at least drop the date_ prefix.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/receivers/FxAccountDeletedService.java:167
(Diff revision 1)
>      } else {
>        Logger.error(LOG_TAG, "Cached OAuth server URI is null or cached OAuth tokens are null; ignoring.");
>      }
>    }
>  
> +  private void clearRemoteDevicesList(Intent intent, Context context) {

nit: This probably belongs to part 3, or a separate commit. But don't worry about it.
Attachment #8855849 - Flags: review?(gkruglov) → review+
Comment on attachment 8855850 [details]
Bug 1351805 part 3 - Refresh the remote devices list on Married/Engaged states.

https://reviewboard.mozilla.org/r/127734/#review131058

Almost there. I think it's worth the effort to delete/inser clients with assurance that we actually do it properly.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/devices/FxAccountDeviceListUpdater.java:39
(Diff revision 1)
> +    public FxAccountDeviceListUpdater(final AndroidFxAccount fxAccount, final ContentResolver cr) {
> +        this.fxAccount = fxAccount;
> +        this.contentResolver = cr;
> +    }
> +
> +    protected ContentProviderOperation makeDeleteAllOperation(Uri uri) {

@VisibleForTesting

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/devices/FxAccountDeviceListUpdater.java:43
(Diff revision 1)
> +
> +    protected ContentProviderOperation makeDeleteAllOperation(Uri uri) {
> +        return ContentProviderOperation.newDelete(uri).withSelection(null, null).build();
> +    }
> +
> +    protected ContentProviderOperation makeInsertOperation(Uri uri, ContentValues values) {

@VisibleForTesting

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/devices/FxAccountDeviceListUpdater.java:75
(Diff revision 1)
> +            deviceValues.put(RemoteDevices.DATE_MODIFIED, now);
> +            ContentProviderOperation insert = makeInsertOperation(remoteDevicesUriWithProfile, deviceValues);
> +            batchOperations.add(insert);
> +        }
> +        try {
> +            contentResolver.applyBatch(BrowserContract.AUTHORITY, batchOperations);

This might not do what you think it does: https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java#2403 - that is, our implementation is a 'best effort' and does not actually provide transactionality.

If this throws, you might be in an inconsistent state.

That's why I mentioned using 'call' in another comment for gaining that level of control. See sync history insert operations for an example of this: https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java#2180

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/devices/FxAccountDeviceListUpdater.java:114
(Diff revision 1)
> +        Log.e(LOG_TAG, "Beginning FxA device list update.");
> +        final byte[] sessionToken;
> +        try {
> +            sessionToken = fxAccount.getState().getSessionToken();
> +        } catch (State.NotASessionTokenState e) {
> +            Log.e(LOG_TAG, "Could not get a session token during Sync (?)", e);

Consider throwing here instead, now that you're explicitely calling this from a token state.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/sync/FxAccountSyncAdapter.java:425
(Diff revision 1)
>        FxAccountDeviceRegistrator.renewRegistration(context);
>      }
>    }
>  
> +  private void onSessionTokenStateReached(Context context, AndroidFxAccount fxAccount) {
> +    maybeRegisterDevice(context, fxAccount); // Does not block

nit: full sentence comments.

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/fxa/devices/TestFxAccountDeviceListUpdater.java:1
(Diff revision 1)
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Tests are public domain, see licence headers in other test files.

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/fxa/devices/TestFxAccountDeviceListUpdater.java:51
(Diff revision 1)
> +
> +    @Mock
> +    State state;
> +
> +    @Mock
> +    ContentResolver contentResolver;

I'm somewhat concerned that we're never actually testing that clients do end up in the database.

Perhaps consider doing something similar to https://dxr.mozilla.org/mozilla-central/source/mobile/android/tests/background/junit4/src/org/mozilla/gecko/sync/repositories/android/VisitsHelperTest.java?q=path%3AVisitsHelperTest&redirect_type=single#59 - it shouldn't be difficult, and you'll end up exercising the BrowserProvider code as well.
Attachment #8855850 - Flags: review?(gkruglov) → review-
Comment on attachment 8852622 [details]
Bug 1351805 part 1 - Create a org.mozilla.gecko.fxa.devices package.

https://reviewboard.mozilla.org/r/124812/#review131098

Assuming this is just moving files around.
Attachment #8852622 - Flags: review?(gkruglov) → review+
Sorry about the lag, thanks for the review Grisha.
I amended my patch with your comments.
Comment on attachment 8855850 [details]
Bug 1351805 part 3 - Refresh the remote devices list on Married/Engaged states.

https://reviewboard.mozilla.org/r/127734/#review135916

r+ with nits and whatnot, but let's expand the model a tiny bit.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/devices/FxAccountDeviceListUpdater.java:54
(Diff revision 2)
> +            ContentValues deviceValues = new ContentValues();
> +            deviceValues.put(RemoteDevices.GUID, fxADevice.id);
> +            deviceValues.put(RemoteDevices.TYPE, fxADevice.type);
> +            deviceValues.put(RemoteDevices.NAME, fxADevice.name);
> +            deviceValues.put(RemoteDevices.IS_CURRENT_DEVICE, fxADevice.isCurrentDevice);
> +            deviceValues.put(RemoteDevices.DATE_CREATED, now);

Since we're straight up replacing values, and this is currently our only update point for this data, this will ensure the CREATED field is essentially useless at the time being.

I'm ok with this for now.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/devices/FxAccountDeviceListUpdater.java:54
(Diff revision 2)
> +            ContentValues deviceValues = new ContentValues();
> +            deviceValues.put(RemoteDevices.GUID, fxADevice.id);
> +            deviceValues.put(RemoteDevices.TYPE, fxADevice.type);
> +            deviceValues.put(RemoteDevices.NAME, fxADevice.name);
> +            deviceValues.put(RemoteDevices.IS_CURRENT_DEVICE, fxADevice.isCurrentDevice);
> +            deviceValues.put(RemoteDevices.DATE_CREATED, now);

There is a potentially very useful "lastAccessTime" field that comes with a device record. We'll certainly need it once this list is being used in the UI, so perhaps let's add it to the data model for the first iteration.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/devices/FxAccountDeviceListUpdater.java:62
(Diff revision 2)
> +        }
> +        valuesBundle.putParcelableArray(BrowserContract.METHOD_PARAM_DATA, insertValues);
> +        try {
> +            contentResolver.call(uri, BrowserContract.METHOD_REPLACE_REMOTE_CLIENTS, uri.toString(),
> +                                 valuesBundle);
> +            Log.e(LOG_TAG, "FxA Device list update done.");

Not an error ;-)

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/devices/FxAccountDeviceListUpdater.java:96
(Diff revision 2)
> +                }
> +        );
> +    }
> +
> +    public void update() {
> +        Log.e(LOG_TAG, "Beginning FxA device list update.");

Not an error, I hope ;-)

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/devices/FxAccountDeviceListUpdater.java:103
(Diff revision 2)
> +        try {
> +            sessionToken = fxAccount.getState().getSessionToken();
> +        } catch (State.NotASessionTokenState e) {
> +            // This should never happen, because the caller (FxAccountSyncAdapter) verifies that
> +            // we are in a token state before calling this method.
> +            throw new IllegalStateException("Could not get a session token during Sync (?)");

Does `e` include any useful information? It might be helpful to include it in the exception.
Attachment #8855850 - Flags: review?(gkruglov) → review+
Comment on attachment 8855849 [details]
Bug 1351805 part 2 - Add remote devices to the Browser Provider.

https://reviewboard.mozilla.org/r/127732/#review134622

I'd be happier if included the whole `call` thing in the third commit, it would have made this easier to review. Doesn't matter now though.

::: commit-message-e3f3e:1
(Diff revision 2)
> +Bug 1351805 part 2 - Add a remote_devices table. r?Grisha

Maybe change the commit message to reflect the BrowserProvider changes as well.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserContract.java:471
(Diff revision 2)
> +        public static final String TABLE_NAME = "remote_devices";
> +
> +        public static final String GUID = "guid"; // FxA device ID
> +        public static final String NAME = "name";
> +        public static final String TYPE = "type";
> +        public static final String IS_CURRENT_DEVICE = "is_current_device";

Let's add "lastAccessed" as well.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:2008
(Diff revision 2)
>  
>          final SQLiteDatabase db = getWritableDatabase(uri);
>          return db.delete(TABLE_PAGE_METADATA, selection, selectionArgs);
>      }
>  
> +    private int deleteRemoveDevices(final Uri uri, final String selection, final String[] selectionArgs) {

typo: `deleteRemoteDevices`

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:2298
(Diff revision 2)
> +
> +        // Wrap everything in a transaction.
> +        beginBatch(db);
> +
> +        try {
> +            // First purge our list of remove devices.

s/remove/remote

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:2299
(Diff revision 2)
> +        // Wrap everything in a transaction.
> +        beginBatch(db);
> +
> +        try {
> +            // First purge our list of remove devices.
> +            deleteInTransaction(uri, null, null);

Let's at least log the return value here. We won't know if it's what it should be here without an additional `select`.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:2299
(Diff revision 2)
> +        // Wrap everything in a transaction.
> +        beginBatch(db);
> +
> +        try {
> +            // First purge our list of remove devices.
> +            deleteInTransaction(uri, null, null);

Let's use an explicit content uri here, not what the consumer passed in. Just to be sure we're actually going to do what this method intends to do.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:2303
(Diff revision 2)
> +            // First purge our list of remove devices.
> +            deleteInTransaction(uri, null, null);
> +
> +            // Then insert the new ones.
> +            for (int i = 0; i < values.length; i++) {
> +                insertInTransaction(uri, values[i]);

Better approach is to call `insertFxaDevice` directly (why bother with extra calls and switch statements?), wrap that call in try/catch as that calls insertOrThrow, and decide what you want to do in case of individual failures. At the very least, log the failures.

Same note about using an explicit content uri as above.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:2310
(Diff revision 2)
> +            markBatchSuccessful(db);
> +        } finally {
> +            endBatch(db);
> +        }
> +
> +        getContext().getContentResolver().notifyChange(uri, null, false);

You might eventually want to make "should sync to network" a URI property here that consumers of this api can toggle.
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a3dfb4b9dc66
part 1 - Create a org.mozilla.gecko.fxa.devices package. r=Grisha,nalexander
https://hg.mozilla.org/integration/autoland/rev/430b56176e04
part 2 - Add remote devices to the Browser Provider. r=Grisha
https://hg.mozilla.org/integration/autoland/rev/da34da5f310a
part 3 - Refresh the remote devices list on Married/Engaged states. r=Grisha
Thanks, turns out these tests only failed when running the whole test suite.
Flags: needinfo?(eoger)
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e9423fdf344a
part 1 - Create a org.mozilla.gecko.fxa.devices package. r=Grisha,nalexander
https://hg.mozilla.org/integration/autoland/rev/af459377995c
part 2 - Add remote devices to the Browser Provider. r=Grisha
https://hg.mozilla.org/integration/autoland/rev/e6418e1abf30
part 3 - Refresh the remote devices list on Married/Engaged states. r=Grisha
https://hg.mozilla.org/mozilla-central/rev/e9423fdf344a
https://hg.mozilla.org/mozilla-central/rev/af459377995c
https://hg.mozilla.org/mozilla-central/rev/e6418e1abf30
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Depends on: 1360047
Blocks: 1359279
Depends on: 1361534
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: