Firefox Accounts has a new(ish) device registration API, allowing devices to explicitly register themselves when they connect to an account, and provide metadata including a displayname and push notification URL. Registered devices will appear in a "devices list" view in the FxA settings UI, and can receive push notifications of events associated with the account. This bug is to discuss (and hopefully implement!) getting Fennec devices to register with this API. The API calls are documented at [1], and the initial desktop integration was done in Bug 1227527. We're working on using it to speed up account verification in Bug 1235607, and plan to build out more push-driven functionality this year. Nick, what are your current thoughts on this API, and whether/how it can add value to the Fennec connection experience? [1] https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md#api-endpoints

I actually thought I filed this, but now I don't see it. This is blocked by Bug 1206207 (at least, the value is blocked) but should follow very fast. I'm working on Android Push as we speak, and have already structured this consumer into that work. I would like to understand what the schedule to POST /device is on Desktop. Do we do this every Sync? Only when the endpoint changes? On a schedule (say every week)? How careful do we need to be to *delete* devices? Technically, this is a lot more feasible to do -- somewhat robustly -- when Android accounts are removed. This is because we can DELETE with the known, stable FxA auth token; that's not possible with the possibly unknown, definitely not stable Sync token server token. I am so excited to burn Sync's clients with fire!

This isn't a great mentor ticket, but an advanced contributor could do it. I can break it down if I don't get to it.

> I would like to understand what the schedule to POST /device is on Desktop. Phil shared this in IRC earlier today: """ <pb> 1. on sync start, if there is no device id <pb> 2. on sync start, if the stale device info flag is set <pb> 3. when the device name is updated <pb> (stale device flag is set if an earlier attempt to register the device failed for some reason) """ To which we'll soon be adding: 4. when the push subscription URL changes > How careful do we need to be to *delete* devices? When you disconnect a device, destroy its device record, which will also destroy the associated sessionToken. Obviously things won't always work out that way, but part of the value of pulling these records up into FxA, is that we can provide more UI options for cleaning up stale device records. Also if you're interested, we could take another look at device id deduplication and recovery [1] as part of driving this forward on mobile. [1] https://github.com/mozilla/fxa-auth-server/issues/1077

In addition to comment 4, desktop currently does not unregister the device when sync is unlinked, which is an omission. I'm hoping we will fix this when we introduce push support (at that point we will want to unregister the push URL with Firefox too) > I am so excited to burn Sync's clients with fire! Indeed :) Note that to try and help us mitigate the pain of multiple device concepts, desktop now writes a |fxaDeviceId| to the client record, which is the FxA device GUID. The expectation is that this will (a) help later Sync clients to determine whether a client record came from an FxA-device-aware client (eg, to prefer the FxA device info over the Sync info), and (b) to allow Synced Tabs to indirect back to the FxA device (tab records have the Sync client ID rather than the FxA ID). So it probably makes sense for Fennec to do the same until we actually *do* kill it with fire.

Nick, I would be interested in contributing if you could perform a breakdown of the tasks.

(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #5) > Nick, I would be interested in contributing if you could perform a breakdown > of the tasks. It's not a complete breakdown, but I filed Bug 1254640 to get this started.

Francisco: I will be working on some different things for quite a while, so I'm going to remove myself as a mentor from this ticket. I suggest you look for a different ticket; this one isn't an easy task, and I was the best person to help you get it done. Lots of other work to be done for Fennec, though!

Sure, np. Thanks!

Review commit: https://reviewboard.mozilla.org/r/53660/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/53660/

Review commit: https://reviewboard.mozilla.org/r/53662/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/53662/

Review commit: https://reviewboard.mozilla.org/r/53664/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/53664/

Review commit: https://reviewboard.mozilla.org/r/53666/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/53666/

Review commit: https://reviewboard.mozilla.org/r/53668/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/53668/

Review commit: https://reviewboard.mozilla.org/r/53670/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/53670/

Review commit: https://reviewboard.mozilla.org/r/53672/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/53672/

Review commit: https://reviewboard.mozilla.org/r/53674/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/53674/

Review commit: https://reviewboard.mozilla.org/r/53676/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/53676/

Review commit: https://reviewboard.mozilla.org/r/53678/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/53678/

It's probably bad since it's my first Android patch but I nevertheless I would love to get feedback on this. Here's what's in this patch : - Registration on signup/login - Registration on name change - Registration on stale device This patch does not include any push-related changes, it mostly mimics what the desktop client is doing. I don't know exactly what kind of action handleTokenError should take if the account still exists but the sessionToken is invalid, I put a TODO there.

Grisha may also be able to offer feedback.

Comment on attachment 8754035 [details] Bug 1250782 - Allow MozResponse to return a Json array body. https://reviewboard.mozilla.org/r/53660/#review51046 ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/MozResponse.java:117 (Diff revision 1) > } finally { > content.close(); > } > } > > + public JSONArray jsonArrayBody() throws NonArrayJSONException, IOException { Since this is only being used to access JSONArray values (as opposed to being passed into an API that accepts the particular type), it'd probably be better to use the built-in JSONArray classes: https://developer.android.com/reference/org/json/JSONArray.html That being said, considering the other code in this file uses the json simple APIs, it's not a big enough deal to warrant rewriting code that already works. ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/MozResponse.java:118 (Diff revision 1) > + JSONParser parser = new JSONParser(); > + Object parsed; nit: it's good to declare your variables as `final` (immutable). The compiler will warn you if you try to reassign or never assign to the variable, better helping you to track your assumptions. Here and below. ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/MozResponse.java:123 (Diff revision 1) > + JSONParser parser = new JSONParser(); > + Object parsed; > + if (body != null) { > + // Do it from the cached String. > + try { > + parsed = parser.parse(body); Do you mean `return parser.parse(body);`? ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/MozResponse.java:143 (Diff revision 1) > + parsed = parser.parse(in); > + } catch(ParseException e) { > + throw new NonArrayJSONException(e); > + } > + } finally { > + content.close(); This should be `in.close()`, which closes the inner stream afaik.

Comment on attachment 8754036 [details] Bug 1250782 - ResourceDelegate#handleSuccess now gives access to a JsonArray body. https://reviewboard.mozilla.org/r/53662/#review51050 I'd like to try to get rid of the code duplication here, otherwise it looks good. ::: mobile/android/services/src/main/java/org/mozilla/gecko/background/fxa/FxAccountClient20.java:180 (Diff revision 1) > * Override <code>handleSuccess</code> to parse the body of the resource > * request and call the request callback. <code>handleSuccess</code> is > * invoked via the executor, so you don't need to delegate further. > */ > protected abstract class ResourceDelegate<T> extends BaseResourceDelegate { > - protected abstract void handleSuccess(final int status, HttpResponse response, final ExtendedJSONObject body); > + protected abstract void handleSuccess(final int status, SyncResponse response); Not part of your changes, but I'd think `handleSuccess` should `throws Exception` so we always handle exceptions in the same `delegate.handleError` line, rather than duplicating it in each of the handleSuccess methods. ::: mobile/android/services/src/main/java/org/mozilla/gecko/background/fxa/FxAccountClient20.java:422 (Diff revision 1) > @Override > - public void handleSuccess(int status, HttpResponse response, ExtendedJSONObject body) { > + public void handleSuccess(int status, SyncResponse response) { > try { > byte[] kA = new byte[FxAccountUtils.CRYPTO_KEY_LENGTH_BYTES]; > byte[] wrapkB = new byte[FxAccountUtils.CRYPTO_KEY_LENGTH_BYTES]; > + ExtendedJSONObject body = response.jsonObjectBody(); I dislike that we're duplicating this code in all of the inheritors. An alternative solution: have the `ResourceDelegate` constructor take in a parameter, probably a custom enum like `enum ResponseType { JSON_ARRAY, JSON_OBJECT }`, and perform each operation as a branch in `invokeSuccess`.

Comment on attachment 8754037 [details] Bug 1250782 - FxAccountClient: add accountStatus method and rename status method to recoveryEmailStatus. https://reviewboard.mozilla.org/r/53664/#review51058 This looks good besides the test bustage and the unanswered question. If you decide to fix the tests in a separate commit, let me know – after you answer my other question, I'd r+ this. ::: mobile/android/services/src/main/java/org/mozilla/gecko/background/fxa/FxAccountClient20.java:440 (Diff revision 1) > public static class StatusResponse { > - public final String email; > - public final boolean verified; > - public StatusResponse(String email, boolean verified) { > + public final boolean exists; > + public final boolean locked; > + public StatusResponse(boolean exists, boolean locked) { This class is used by the unit tests (`TestUserAgentHeaders`) and appears to have broken compilation of the tests. Several notes on the tests: * If you dev in the IDE, I believe it should warn you of test compilation failures. However, you have to make sure to set the build variant "Test Artifact" to "Unit Tests" (click "Build Variants" on the bottom left and change it in the dropdown) * You can run these tests in the ide by right-clicking on the suite and clicking run * You can run all the tests locally with `./mach gradle test`. I have local tests failures (bug 1271000) but it should at least warn you if the tests are not compiling (in the case the IDE does not) ::: mobile/android/services/src/main/java/org/mozilla/gecko/background/fxa/FxAccountClient20.java:458 (Diff revision 1) > - return; > - } > - > BaseResource resource; > try { > - resource = getBaseResource("recovery_email/status"); > + resource = getBaseResource("account/status"); Was this method unused before? Why are we okay removing it? On that note, perhaps we should make the name of this method, `accountStatus` rather than just `status`? For future reference, it's good to explain in the commit message if you make incidental changes like this (or to keep them as separate commits).

I'll continue reviewing this next week.

Comment on attachment 8754035 [details] Bug 1250782 - Allow MozResponse to return a Json array body. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53660/diff/1-2/

Comment on attachment 8754036 [details] Bug 1250782 - ResourceDelegate#handleSuccess now gives access to a JsonArray body. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53662/diff/1-2/

Comment on attachment 8754037 [details] Bug 1250782 - FxAccountClient: add accountStatus method and rename status method to recoveryEmailStatus. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53664/diff/1-2/

Comment on attachment 8754038 [details] Bug 1250782 - FxAccountClient: add registerOrUpdateDevice method. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53666/diff/1-2/

Comment on attachment 8754039 [details] Bug 1250782 - FxAccountClient: add deviceList method. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53668/diff/1-2/

Comment on attachment 8754040 [details] Bug 1250782 - Add FxAccountDeviceRegistrator. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53670/diff/1-2/

Comment on attachment 8754041 [details] Bug 1250782 - Register device on married state. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53672/diff/1-2/

Comment on attachment 8754042 [details] Bug 1250782 - Update device registration on device name change. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53674/diff/1-2/

Comment on attachment 8754043 [details] Bug 1250782 - Include fxaDeviceId in sync client records. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53676/diff/1-2/

Comment on attachment 8754044 [details] Bug 1250782 - Update device registration when getting fxaDeviceId if necessary. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53678/diff/1-2/

https://reviewboard.mozilla.org/r/53660/#review51046 > Since this is only being used to access JSONArray values (as opposed to being passed into an API that accepts the particular type), it'd probably be better to use the built-in JSONArray classes: > https://developer.android.com/reference/org/json/JSONArray.html > > That being said, considering the other code in this file uses the json simple APIs, it's not a big enough deal to warrant rewriting code that already works. It adds a bit more complexity to parse a Reader with the built-in class, I'd prefer to keep the json simple one.

https://reviewboard.mozilla.org/r/53662/#review51050 > Not part of your changes, but I'd think `handleSuccess` should `throws Exception` so we always handle exceptions in the same `delegate.handleError` line, rather than duplicating it in each of the handleSuccess methods. Fixed it

https://reviewboard.mozilla.org/r/53664/#review51058 > Was this method unused before? Why are we okay removing it? > > On that note, perhaps we should make the name of this method, `accountStatus` rather than just `status`? > > For future reference, it's good to explain in the commit message if you make incidental changes like this (or to keep them as separate commits). Kept the old method and renamed it to recoveryEmailStatus.

Thank you for this first round of reviews Michael, I updated the commits you reviewed already.

Comment on attachment 8754035 [details] Bug 1250782 - Allow MozResponse to return a Json array body. https://reviewboard.mozilla.org/r/53660/#review52020

Comment on attachment 8754036 [details] Bug 1250782 - ResourceDelegate#handleSuccess now gives access to a JsonArray body. https://reviewboard.mozilla.org/r/53662/#review52022 ::: mobile/android/services/src/main/java/org/mozilla/gecko/background/fxa/FxAccountClient20.java:190 (Diff revisions 1 - 2) > - protected abstract void handleSuccess(final int status, SyncResponse response); > > + protected void handleSuccess(final int status, HttpResponse response, final ExtendedJSONObject body) throws Exception { > + throw new UnsupportedOperationException(); > + } > + protected void handleSuccess(final int status, HttpResponse response, final JSONArray body) throws Exception { nit: nl above

Comment on attachment 8754037 [details] Bug 1250782 - FxAccountClient: add accountStatus method and rename status method to recoveryEmailStatus. https://reviewboard.mozilla.org/r/53664/#review52040 ::: mobile/android/services/src/main/java/org/mozilla/gecko/background/fxa/FxAccountClient20.java:465 (Diff revision 2) > + * @param uid > + * to query. > + * @param delegate > + * to invoke callbacks. nit: you can put the parameter descriptions on the same line ::: mobile/android/services/src/main/java/org/mozilla/gecko/background/fxa/FxAccountClient20.java:471 (Diff revision 2) > + * to query. > + * @param delegate > + * to invoke callbacks. > + */ > + public void accountStatus(String uid, final RequestDelegate<AccountStatusResponse> delegate) { > + BaseResource resource; nit: `final` here in particular will give you some safety by ensuring `resource` is intialized before it's used. ::: mobile/android/services/src/main/java/org/mozilla/gecko/background/fxa/FxAccountClient20.java:473 (Diff revision 2) > + * to invoke callbacks. > + */ > + public void accountStatus(String uid, final RequestDelegate<AccountStatusResponse> delegate) { > + BaseResource resource; > + try { > + final Map<String, String> params = new HashMap<>(); micro-nit: `new HashMap(1)` Since it doesn't look like the method it's passed to modifies the hash map at all ::: mobile/android/services/src/main/java/org/mozilla/gecko/background/fxa/FxAccountClient20.java:484 (Diff revision 2) > + Boolean exists = body.getBoolean(JSON_KEY_EXISTS); > + Boolean locked = body.getBoolean(JSON_KEY_LOCKED); micro-nit: `boolean` lower-case. If this code changes to use the other json object, `boolean` will be returned and we'll be spared from having to wrap the primitive boolean in the Object `Boolean` ::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/fxa/login/MockFxAccountClient.java:101 (Diff revision 2) > + requestDelegate.handleSuccess(new AccountStatusResponse(true, false)); > + return; > + } > + } > + requestDelegate.handleSuccess(new AccountStatusResponse(false, false)); nit: you can remove the duplication by setting a temporary variable in the loop, e.g.: boolean userFound = false; for (...) { if (...) { userFound = true; break; } } requestDelegate.handleSuccess(new ...(userFound, false));

Comment on attachment 8754038 [details] Bug 1250782 - FxAccountClient: add registerOrUpdateDevice method. https://reviewboard.mozilla.org/r/53666/#review52054 Code here looks good but you'll need to get the tests compiling before you can land this (commit on top of this series?) ::: mobile/android/services/src/main/java/org/mozilla/gecko/background/fxa/FxAccountClient.java:19 (Diff revision 2) > public interface FxAccountClient { > public void accountStatus(String uid, RequestDelegate<AccountStatusResponse> requestDelegate); > public void recoveryEmailStatus(byte[] sessionToken, RequestDelegate<RecoveryEmailStatusResponse> requestDelegate); > public void keys(byte[] keyFetchToken, RequestDelegate<TwoKeys> requestDelegate); > public void sign(byte[] sessionToken, ExtendedJSONObject publicKey, long certificateDurationInMilliseconds, RequestDelegate<String> requestDelegate); > + public void registerOrUpdateDevice(byte[] sessionToken, FxaDevice device, RequestDelegate<FxaDevice> requestDelegate); Add this to MockFxAccountClient. ::: mobile/android/services/src/main/java/org/mozilla/gecko/background/fxa/FxAccountClient20.java:787 (Diff revision 2) > + * @param sessionToken > + * to query. > + * @param delegate > + * to invoke callbacks. nit: param desc on same line ::: mobile/android/services/src/main/java/org/mozilla/gecko/background/fxa/FxAccountClient20.java:819 (Diff revision 2) > + return; > + } catch (Exception e) { > + delegate.handleError(e); > + return; nit: no need for `return` if we're never intending to add code after the `try` or `catch` blocks. You can't fall through from try -> catch without an exception. ::: mobile/android/services/src/main/java/org/mozilla/gecko/background/fxa/FxAccountClient20.java:827 (Diff revision 2) > + return; > + } > + } > + }; > + > + post(resource, body, delegate); nit: `delegate` is unused in the method this calls - how about we remove the parameter? This could be done in a commit at the end of this series (r=you).

https://reviewboard.mozilla.org/r/53666/#review52056 Also, fwiw, I can only guess what HKDF does here but I trust you that's it's needed.

Comment on attachment 8754039 [details] Bug 1250782 - FxAccountClient: add deviceList method. https://reviewboard.mozilla.org/r/53668/#review52046 Same deal as the last commit – the tests will need to compile before this can land. It'd also be cool to remove the duplication between this commit & the last commit (& the other times this pattern is used in the file) but it might be unclear given all the side-effects that will need to take place - your call. ::: mobile/android/services/src/main/java/org/mozilla/gecko/background/fxa/FxAccountClient.java:20 (Diff revision 2) > public void accountStatus(String uid, RequestDelegate<AccountStatusResponse> requestDelegate); > public void recoveryEmailStatus(byte[] sessionToken, RequestDelegate<RecoveryEmailStatusResponse> requestDelegate); > public void keys(byte[] keyFetchToken, RequestDelegate<TwoKeys> requestDelegate); > public void sign(byte[] sessionToken, ExtendedJSONObject publicKey, long certificateDurationInMilliseconds, RequestDelegate<String> requestDelegate); > public void registerOrUpdateDevice(byte[] sessionToken, FxaDevice device, RequestDelegate<FxaDevice> requestDelegate); > + public void deviceList(byte[] sessionToken, RequestDelegate<FxaDevice[]> requestDelegate); The tests fail to compile: `MockFxAccountClient` needs to override this. Make sure to run the tests! I figured out the local test failures, btw - see https://mail.mozilla.org/pipermail/mobile-firefox-dev/2016-May/002085.html and you should be able to run `./mach gradle test` with confidence. ::: mobile/android/services/src/main/java/org/mozilla/gecko/background/fxa/FxAccountClient20.java:862 (Diff revision 2) > + return; > + } catch (Exception e) { > + delegate.handleError(e); > + return; nit: no need to return out of try/catch

Edouard, is there a benefit to using an object for the FxAccountDeviceRegistrator? I feel like static methods would be more appropriate because it makes the dependencies for the method more apparent (unless you wanted to mock the internal methods for testing reasons). I'll finish the rest of the review after ^. We can also talk in person tomorrow.

Comment on attachment 8754040 [details] Bug 1250782 - Add FxAccountDeviceRegistrator. https://reviewboard.mozilla.org/r/53670/#review52060 This is incomplete but potentially some useful comments. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:26 (Diff revision 2) > + public static final Integer DEVICE_REGISTRATION_VERSION = 1; > + > + // FxA api error codes > + private static final Integer ERRNO_UNKNOWN_DEVICE = 123; > + private static final Integer ERRNO_DEVICE_SESSION_CONFLICT = 124; > + private static final Integer ERRNO_INVALID_AUTH_TOKEN = 110; nit: use `int` over `Integer`. `Integer` is an object and will cause unnecessary allocation when a primitive will do. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:36 (Diff revision 2) > + private static final Integer ERRNO_INVALID_AUTH_TOKEN = 110; > + > + private static final String LOGTAG = "FxADeviceRegistrator"; > + > + private final AndroidFxAccount fxAccount; > + private final Context mContext; nit: do not mix hungarian notation with non. i.e. this should be `context`. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:42 (Diff revision 2) > + private final FxAccountClient20 fxAccountClient; > + private final byte[] sessionToken; > + > + public FxAccountDeviceRegistrator(AndroidFxAccount fxAccount, Context context) { > + this.fxAccount = fxAccount; > + this.mContext = context; Holding onto a reference to context is generally discouraged in Android because a context is a god object that contains a reference to many other things and could prevent things from getting GC'd (e.g. if this object is held onto for a long time for whatever reason). Looking through this quickly, it looks like you can pass the context into all of the methods that it is used in so that would be preferred. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:68 (Diff revision 2) > + } > + > + @Nullable > + private byte[] getSessionToken() { > + State state = fxAccount.getState(); > + if (!(state instanceof TokensAndKeysState)) { nit: It's generally discouraged to type-check/cast – is there a better way to do this? ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:68 (Diff revision 2) > + if (!(state instanceof TokensAndKeysState)) { > + Log.e(LOGTAG, "Invalid state: no session token available"); > + return null; > + } Do we ever really expect this method to be called with the wrong instance state? If not, you should throw – it's better to fail early and have a clear idea of where the problem is then to fail mysterious (and maybe silently!) later.

Most of the issues are fixed, I will provide a new patch once the tests pass

Review commit: https://reviewboard.mozilla.org/r/55656/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/55656/

Comment on attachment 8754035 [details] Bug 1250782 - Allow MozResponse to return a Json array body. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53660/diff/2-3/

Comment on attachment 8754036 [details] Bug 1250782 - ResourceDelegate#handleSuccess now gives access to a JsonArray body. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53662/diff/2-3/

Comment on attachment 8754037 [details] Bug 1250782 - FxAccountClient: add accountStatus method and rename status method to recoveryEmailStatus. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53664/diff/2-3/

Comment on attachment 8754038 [details] Bug 1250782 - FxAccountClient: add registerOrUpdateDevice method. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53666/diff/2-3/

Comment on attachment 8754039 [details] Bug 1250782 - FxAccountClient: add deviceList method. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53668/diff/2-3/

Comment on attachment 8754040 [details] Bug 1250782 - Add FxAccountDeviceRegistrator. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53670/diff/2-3/

Comment on attachment 8754041 [details] Bug 1250782 - Register device on married state. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53672/diff/2-3/

Comment on attachment 8754042 [details] Bug 1250782 - Update device registration on device name change. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53674/diff/2-3/

Comment on attachment 8754043 [details] Bug 1250782 - Include fxaDeviceId in sync client records. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53676/diff/2-3/

Comment on attachment 8754044 [details] Bug 1250782 - Update device registration when getting fxaDeviceId if necessary. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53678/diff/2-3/

Comment on attachment 8757116 [details] Bug 1250782 - FxAccountClient: remove post method unused argument delegate. https://reviewboard.mozilla.org/r/55656/#review52354

Comment on attachment 8754040 [details] Bug 1250782 - Add FxAccountDeviceRegistrator. https://reviewboard.mozilla.org/r/53670/#review52672 Not on my dev machine and it's hard to fully review this without an IDE so I'll get back to you next week. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:1 (Diff revision 3) > +package org.mozilla.gecko.fxa; nit: add copyright to top of file ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:25 (Diff revision 3) > +import java.io.UnsupportedEncodingException; > +import java.security.GeneralSecurityException; > +import java.util.concurrent.ExecutorService; > +import java.util.concurrent.Executors; > + > +public class FxAccountDeviceRegistrator { Classes that are not intended to be instantiated should have a private constructor: `private FxAccountDeviceRegistrator() {}` Java inserts implicit no-arg public constructors if no other constructor is specified. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:33 (Diff revision 3) > + > + private static final String LOGTAG = "FxADeviceRegistrator"; > + > + private static void logErrorAndResetDeviceRegistrationVersion( > + final FxAccountClientRemoteException error, final AndroidFxAccount fxAccount) { > + Log.e(LOGTAG, "Device registration failed", error); What are in these exceptions? Is it okay to log them? Will we leak user data? One common source of user data we try not to leak are URLs. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:60 (Diff revision 3) > + return tokensAndKeysState.getSessionToken(); > + } > + return null; > + } > + > + public interface RegisterDelegate { nit: typically classes & interfaces are defined at the top or bottom of files. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:64 (Diff revision 3) > + > + public interface RegisterDelegate { > + void onComplete(String deviceId); > + } > + > + public static void register(final AndroidFxAccount fxAccount, final Context context) { nit: I think the intent of this file would be clearer if these two public methods were defined before any of the private helper methods. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/AndroidFxAccount.java:781 (Diff revision 3) > context.startService(intent); > } > }); > } > > + public String getDeviceId() { nit: `@nullable` to document this returns null Maybe add a javadoc too to explain when it returns null ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/AndroidFxAccount.java:786 (Diff revision 3) > + public String getDeviceId() { > + return accountManager.getUserData(account, ACCOUNT_KEY_DEVICE_ID); > + } > + > + @NonNull > + private Integer getDeviceRegistrationVersion() { `int` over `Integer`. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/AndroidFxAccount.java:789 (Diff revision 3) > + > + @NonNull > + private Integer getDeviceRegistrationVersion() { > + String versionStr = accountManager.getUserData(account, ACCOUNT_KEY_DEVICE_REGISTRATION_VERSION); > + if (TextUtils.isEmpty(versionStr)) { > + return 0; nit: would it be better to return -1 for a clearer "unset" condition? ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/AndroidFxAccount.java:799 (Diff revision 3) > + > + public void setDeviceId(String id) { > + accountManager.setUserData(account, ACCOUNT_KEY_DEVICE_ID, id); > + } > + > + public void setDeviceRegistrationVersion(Integer deviceRegistrationVersion) { It's generally better to use `int` over `Integer`. Rather than `setDeviceRegsistrationVersion(errNo)`, it could be clearer to have a second method, `resetDeviceRegistrationVersion()`, or similar.

https://reviewboard.mozilla.org/r/53670/#review52672 > What are in these exceptions? Is it okay to log them? Will we leak user data? One common source of user data we try not to leak are URLs. We are okay here this exception doesn't contain the request

Comment on attachment 8754040 [details] Bug 1250782 - Add FxAccountDeviceRegistrator. https://reviewboard.mozilla.org/r/53670/#review53432 r+ once we address the threading issue & removing the account. Sorry that this patch series has been taking so long – I'll try to be on top of reviewing them in the future so we can finish this up quickly. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:54 (Diff revision 3) > + @Nullable > + private static byte[] getSessionToken(final AndroidFxAccount fxAccount) { > + State state = fxAccount.getState(); > + State.StateLabel stateLabel = state.getStateLabel(); > + if (stateLabel == State.StateLabel.Engaged || stateLabel == State.StateLabel.Married) { > + TokensAndKeysState tokensAndKeysState = (TokensAndKeysState) state; How do we know this is a `TokensAndKeysState`? fwiw, any time I see casting it throws off a red flag - there's almost always a better way (though perhaps not if the APIs you're doing force it). ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:70 (Diff revision 3) > + final byte[] sessionToken = getSessionToken(fxAccount); > + if (sessionToken == null) { > + throw new IllegalStateException("sessionToken is null, invalid state!"); > + } Let's throw in `getSessionToken` and avoid the indirection. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:75 (Diff revision 3) > + final byte[] sessionToken = getSessionToken(fxAccount); > + if (sessionToken == null) { > + throw new IllegalStateException("sessionToken is null, invalid state!"); > + } > + > + FxAccountDevice device; nit: `final` It enforces initilazation in all branches. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:84 (Diff revision 3) > + device = FxAccountDevice.ForRegister(clientName, "mobile"); > + } else { > + device = FxAccountDevice.ForUpdate(deviceId, clientName); > + } > + > + ExecutorService executor = Executors.newSingleThreadExecutor(); How often is this method called? Every time it's called, it will spawn a new thread (I skimmed [the source](http://androidxref.com/4.4.4_r1/xref/libcore/luni/src/main/java/java/util/concurrent/ThreadPoolExecutor.java)) and that can really affect mobile devices. Perhaps there's an existing thread we can use in sync somewhere? Do you know where it creates its threads from? You can probably use the call hierarchy viewer with the `FxAccountClient20` constructor to gain insight here. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:101 (Diff revision 3) > + if (error.httpStatusCode == 400) { > + if (error.apiErrorNumber == FxAccountRemoteError.UNKNOWN_DEVICE) { > + recoverFromUnknownDevice(fxAccount); > + delegate.onComplete(null); > + } > + if (error.apiErrorNumber == FxAccountRemoteError.DEVICE_SESSION_CONFLICT) { nit: `} else if {` is a bit clearer. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:124 (Diff revision 3) > + delegate.onComplete(result.id); > + } > + }); > + } > + > + private static void handleTokenError(final FxAccountClientRemoteException error, nit: we know from where this method is called that we're handling the token error. Perhaps it should be more descriptive about what it's trying to do? e.g. `attemptToRemoveAccount`. It makes it clearer in the calling method. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:143 (Diff revision 3) > + > + @Override > + public void handleSuccess(AccountStatusResponse result) { > + if (!result.exists) { > + Log.i(LOGTAG, "token invalidated because the account no longer exists"); > + AccountManager.get(context).removeAccount(fxAccount.getAndroidAccount(), null, null); This seems extreme & we don't warn the user that it's happening – is that reasonable? Is this ever expected to happen? Is there something better we can do? ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:147 (Diff revision 3) > + Log.i(LOGTAG, "token invalidated because the account no longer exists"); > + AccountManager.get(context).removeAccount(fxAccount.getAndroidAccount(), null, null); > + return; > + } > + Log.e(LOGTAG, "sessionToken invalid"); > + // TODO: sessionToken expired, what do? Do we want to address this TODO before landing? If not, file a follow-up and list a bug number. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:158 (Diff revision 3) > + private static void recoverFromUnknownDevice(final AndroidFxAccount fxAccount) { > + Log.i(LOGTAG, "unknown device id, clearing the cached device id"); > + fxAccount.setDeviceId(null); > + } > + > + private static void recoverFromDeviceSessionConflict(final FxAccountClientRemoteException error, nit: I'd comment that this method is intended to call `delegate.onComplete(null)` once it's done. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:175 (Diff revision 3) > + delegate.onComplete(null); > + } > + > + @Override > + public void handleFailure(FxAccountClientRemoteException e) { > + Log.e(LOGTAG, "failed to recover from device-session conflict"); nit: Did you want to distinguish between this case & handleError? ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:182 (Diff revision 3) > + for (FxAccountDevice device : devices) { > + if (device.isCurrentDevice) { > + fxAccount.setDeviceId(device.id); > + fxAccount.setDeviceRegistrationVersion(null); > + register(fxAccount, context, delegate); > + return; > + } > + } > + logErrorAndResetDeviceRegistrationVersion(error, fxAccount); nit: it might be good to log that you failed at the end of this.

Comment on attachment 8754041 [details] Bug 1250782 - Register device on married state. https://reviewboard.mozilla.org/r/53672/#review52676 Supposedly the JS code that passes the message says: "Create a new Android Account corresponding to the given fxa-content-server "login" JSON datum. The new account will be in the "Engaged" state, and will start syncing immediately." I don't know when the fxa-content-server "login" JSON datum is received, and thus if this is the appropriate place to call register. Given that, on Android, login only happens once when we create the Android account, this seems fine. You can address my other concerns if they're not misplaced. ::: mobile/android/base/java/org/mozilla/gecko/AccountsHelper.java:125 (Diff revision 3) > tokenServerEndpoint, > profileServerEndpoint, > state, > AndroidFxAccount.DEFAULT_AUTHORITIES_TO_SYNC_AUTOMATICALLY_MAP); > > + // Device registration nit: I think these comments are redundant. If you don't think the code is specific enough itself, consider renaming the methods! e.g. `FxAccountDeviceRegistrator.registerDevice`. fwiw, when I'm writing an inline comment, I try to see if comment can be expressed with better code (since the comments can go out of date). I feel this usually leads to more readable code. For example, if I was to add `// Device registration` because I make three or four calls and I want it to be clear what the code block is doing, I'll usually pull it out into helper method call `registerDevice()`, or similar. ::: mobile/android/base/java/org/mozilla/gecko/AccountsHelper.java:191 (Diff revision 3) > final State state = new Engaged(email, uid, verified, unwrapkB, sessionToken, keyFetchToken); > > final AndroidFxAccount fxAccount = new AndroidFxAccount(mContext, account); > fxAccount.setState(state); > > + // Device registration nit: here too ::: mobile/android/base/java/org/mozilla/gecko/AccountsHelper.java:192 (Diff revision 3) > > final AndroidFxAccount fxAccount = new AndroidFxAccount(mContext, account); > fxAccount.setState(state); > > + // Device registration > + FxAccountDeviceRegistrator.register(fxAccount, mContext); This appears like it will be called when the password is updated [1] – is that okay? Is device registration idempotent? [1]: https://dxr.mozilla.org/mozilla-central/rev/4d63dde701b47b8661ab7990f197b6b60e543839/mobile/android/modules/FxAccountsWebChannel.jsm#302

Comment on attachment 8754042 [details] Bug 1250782 - Update device registration on device name change. https://reviewboard.mozilla.org/r/53674/#review53470 ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/SharedPreferencesClientsDataDelegate.java:44 (Diff revision 3) > sharedPreferences.edit().putString(SyncConfiguration.PREF_ACCOUNT_GUID, accountGUID).commit(); > } > return accountGUID; > } > > + private synchronized void saveClientName(String clientName, long now) { nit: -> `saveClientNameToSharedPreferences` If you so general, the difference between `saveClientName` and `setClientName` get lost. ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/SharedPreferencesClientsDataDelegate.java:49 (Diff revision 3) > + private synchronized void saveClientName(String clientName, long now) { > + sharedPreferences > + .edit() > + .putString(SyncConfiguration.PREF_CLIENT_NAME, clientName) > + .putLong(SyncConfiguration.PREF_CLIENT_DATA_TIMESTAMP, now) > + .commit(); You should use `apply` here – apply's occur in memory immediately so all accessors can access it immediately & SharedPrefs is synchronized under the hood. ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/SharedPreferencesClientsDataDelegate.java:80 (Diff revision 3) > public synchronized String getClientName() { > String clientName = sharedPreferences.getString(SyncConfiguration.PREF_CLIENT_NAME, null); > if (clientName == null) { > clientName = getDefaultClientName(); > long now = System.currentTimeMillis(); > - setClientName(clientName, now); > + saveClientName(clientName, now); We wouldn't we want to update the name here? Consider adding a comment.

https://reviewboard.mozilla.org/r/53670/#review53472 Also, we should figure out this possible concurrency issue before r+. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:117 (Diff revision 3) > + fxAccount.setDeviceId(result.id); > + fxAccount.setDeviceRegistrationVersion(DEVICE_REGISTRATION_VERSION); We're updating multiple values at the same time on a background thread but I assume other people can access these values while we update them – do we need locking?

Comment on attachment 8754043 [details] Bug 1250782 - Include fxaDeviceId in sync client records. https://reviewboard.mozilla.org/r/53676/#review52064 ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/SyncClientsEngineStage.java:404 (Diff revision 2) > + if (!TextUtils.isEmpty(deviceId)) { > + r.fxaDeviceId = deviceId; > + } > + } > + > + nit: extra nl ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/SyncClientsEngineStage.java:404 (Diff revision 3) > + if (!TextUtils.isEmpty(deviceId)) { > + r.fxaDeviceId = deviceId; > + } > + } > + > + nit: excess ws (two nl)

Comment on attachment 8754044 [details] Bug 1250782 - Update device registration when getting fxaDeviceId if necessary. https://reviewboard.mozilla.org/r/53678/#review53474 r+ once we resolve the notify/wait issues. By the way, I think Futures are probably the way to go here – they have time-outs and don't have to deal with this while loop/check the condition mumbo jumbo. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/AndroidFxAccount.java:93 (Diff revision 3) > // Each such token is prefixed with "oauth::" and a service-dependent scope. > // Such tokens should be destroyed when the account is removed from the device. > // This list collects all the known "oauth::" token types in order to delete them when necessary. > private static final List<String> KNOWN_OAUTH_TOKEN_TYPES; > > + private static final int GET_DEVICE_ID_TIMEOUT = 5000; nit: -> `BLAH_BLAH_BLAH_MILLIS` Including units is good practice when doing durations. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/AndroidFxAccount.java:794 (Diff revision 3) > + /** > + * Returns the cached device id if it's fresh or try to get it from the server if > + * the device registration is stale or the cached device id is empty. > + * This is a blocking method even though we make http requests. > + */ > + public String getFreshDeviceId() { nit: -> `getFreshDeviceIdBlocking` I like to be explicit in the method names when I'm blocking, but it's not necessary. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/AndroidFxAccount.java:806 (Diff revision 3) > + final AndroidFxAccount fxAccount = new AndroidFxAccount(context, account); > + FxAccountDeviceRegistrator.register(fxAccount, context, new RegisterDelegate() { > + @Override > + public void onComplete(String deviceId) { > + newDeviceId[0] = deviceId; > + spinningCallback.notify(); The internet says you have to synchronize on the same object when using `notify`/`wait`: http://stackoverflow.com/a/6221775 ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/AndroidFxAccount.java:810 (Diff revision 3) > + newDeviceId[0] = deviceId; > + spinningCallback.notify(); > + } > + }); > + try { > + spinningCallback.wait(GET_DEVICE_ID_TIMEOUT); This should be called in a while loop to check the condition: https://developer.android.com/reference/java/lang/Object.html#wait() ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/SyncClientsEngineStage.java:398 (Diff revision 3) > > Context context = session.getContext(); > final Account account = FirefoxAccounts.getFirefoxAccount(context); > if (account != null) { > final AndroidFxAccount fxAccount = new AndroidFxAccount(context, account); > - final String deviceId = fxAccount.getDeviceId(); > + final String deviceId = fxAccount.getFreshDeviceId(); How do we know this is being called from a thread that doesn't mind being blocked? Add a comment.

Comment on attachment 8754035 [details] Bug 1250782 - Allow MozResponse to return a Json array body. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53660/diff/3-4/

Comment on attachment 8754036 [details] Bug 1250782 - ResourceDelegate#handleSuccess now gives access to a JsonArray body. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53662/diff/3-4/

Comment on attachment 8754037 [details] Bug 1250782 - FxAccountClient: add accountStatus method and rename status method to recoveryEmailStatus. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53664/diff/3-4/

Comment on attachment 8754038 [details] Bug 1250782 - FxAccountClient: add registerOrUpdateDevice method. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53666/diff/3-4/

Comment on attachment 8754039 [details] Bug 1250782 - FxAccountClient: add deviceList method. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53668/diff/3-4/

Comment on attachment 8754040 [details] Bug 1250782 - Add FxAccountDeviceRegistrator. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53670/diff/3-4/

Comment on attachment 8754041 [details] Bug 1250782 - Register device on married state. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53672/diff/3-4/

Comment on attachment 8754042 [details] Bug 1250782 - Update device registration on device name change. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53674/diff/3-4/

Comment on attachment 8754043 [details] Bug 1250782 - Include fxaDeviceId in sync client records. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53676/diff/3-4/

Comment on attachment 8754044 [details] Bug 1250782 - Update device registration when getting fxaDeviceId if necessary. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53678/diff/3-4/

Comment on attachment 8757116 [details] Bug 1250782 - FxAccountClient: remove post method unused argument delegate. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55656/diff/1-2/

https://reviewboard.mozilla.org/r/53670/#review53432 > How do we know this is a `TokensAndKeysState`? > > fwiw, any time I see casting it throws off a red flag - there's almost always a better way (though perhaps not if the APIs you're doing force it). Didn't find a better solution for this and I'd like to avoid adding a getSessionToken() method in the base class. > How often is this method called? Every time it's called, it will spawn a new thread (I skimmed [the source](http://androidxref.com/4.4.4_r1/xref/libcore/luni/src/main/java/java/util/concurrent/ThreadPoolExecutor.java)) and that can really affect mobile devices. > > Perhaps there's an existing thread we can use in sync somewhere? Do you know where it creates its threads from? You can probably use the call hierarchy viewer with the `FxAccountClient20` constructor to gain insight here. We're ok here, this method is not called very often after the first login. > This seems extreme & we don't warn the user that it's happening – is that reasonable? Is this ever expected to happen? Is there something better we can do? I'm gonna ask a sync-team person review of this patch

https://reviewboard.mozilla.org/r/53672/#review52676 > This appears like it will be called when the password is updated [1] – is that okay? Is device registration idempotent? > > [1]: https://dxr.mozilla.org/mozilla-central/rev/4d63dde701b47b8661ab7990f197b6b60e543839/mobile/android/modules/FxAccountsWebChannel.jsm#302 It's okay because it will be considered as an update.

https://reviewboard.mozilla.org/r/53678/#review53474 I replaced the low levels wait/notify calls by a CountDownLatch which seems better suited for this. I'm not sure how to implement this using Futures since we're waiting on a callback (or we would have to modify all the network request implementation).

https://reviewboard.mozilla.org/r/53670/#review53432 > Didn't find a better solution for this and I'd like to avoid adding a getSessionToken() method in the base class. The pattern is to compare `label` and then cast: https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/fxa/sync/FxAccountSyncAdapter.java#335 This is just an API choice. Inheritance allows re-use but doesn't capture sub-typing of this sort well. In Swift, I used a rich enum type, which captures sub-typing well but doesn't allow re-use.

Comment on attachment 8754040 [details] Bug 1250782 - Add FxAccountDeviceRegistrator. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53670/diff/4-5/

Comment on attachment 8754041 [details] Bug 1250782 - Register device on married state. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53672/diff/4-5/

Comment on attachment 8754042 [details] Bug 1250782 - Update device registration on device name change. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53674/diff/4-5/

Comment on attachment 8754043 [details] Bug 1250782 - Include fxaDeviceId in sync client records. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53676/diff/4-5/

Comment on attachment 8754044 [details] Bug 1250782 - Update device registration when getting fxaDeviceId if necessary. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53678/diff/4-5/

Comment on attachment 8757116 [details] Bug 1250782 - FxAccountClient: remove post method unused argument delegate. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55656/diff/2-3/

Comment on attachment 8754040 [details] Bug 1250782 - Add FxAccountDeviceRegistrator. https://reviewboard.mozilla.org/r/53670/#review54914 I'm not sure the error cases are what we want to do but you mentioned that you'll have someone else review it so wfm. It might be good for me to look over the patch again after you add `synchronized` but this looks like it should work so r+. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:62 (Diff revision 5) > + device = FxAccountDevice.ForRegister(clientName, "mobile"); > + } else { > + device = FxAccountDevice.ForUpdate(deviceId, clientName); > + } > + > + ExecutorService executor = Executors.newSingleThreadExecutor(); nit: add a comment explaining why it's okay to use the single thread executor. Document your assumptions! :) ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:79 (Diff revision 5) > + if (error.httpStatusCode == 400) { > + if (error.apiErrorNumber == FxAccountRemoteError.UNKNOWN_DEVICE) { > + recoverFromUnknownDevice(fxAccount); > + delegate.onComplete(null); > + } else if (error.apiErrorNumber == FxAccountRemoteError.DEVICE_SESSION_CONFLICT) { > + recoverFromDeviceSessionConflict(error, fxAccountClient, sessionToken, fxAccount, nit: add a comment mentioning this is expected to call onComplete at the end ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:82 (Diff revision 5) > + delegate.onComplete(null); > + } else if (error.apiErrorNumber == FxAccountRemoteError.DEVICE_SESSION_CONFLICT) { > + recoverFromDeviceSessionConflict(error, fxAccountClient, sessionToken, fxAccount, > + context, delegate); > + } > + } else if (error.httpStatusCode == 401 nit: I think this would be slightly more readable if there was a newline above this and `else` ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:155 (Diff revision 5) > + } > + Log.e(LOG_TAG, "sessionToken invalid"); > + // TODO: sessionToken expired, what do? > + } > + }); > + logErrorAndResetDeviceRegistrationVersion(error, fxAccount); nit: it'd be harder to miss this method if it was above the `accountStatus` call. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:194 (Diff revision 5) > + > + @Override > + public void handleSuccess(FxAccountDevice[] devices) { > + for (FxAccountDevice device : devices) { > + if (device.isCurrentDevice) { > + fxAccount.setFxAUserData(device.id, 0); Shouldn't the device registration version be `DEVICE_REGISTRATION_VERSION`? If not (e.g. because this is an error), this is a good time to use a helper method like, `resetDeviceRegistrationVersion()` instead. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/AndroidFxAccount.java:789 (Diff revision 5) > + public String getDeviceId() { > + fxaUserDataReadLock.lock(); > + try { > + return accountManager.getUserData(account, ACCOUNT_KEY_DEVICE_ID); > + } finally { > + fxaUserDataReadLock.unlock(); > + } > + } You can greatly simplify this locking with the Java `synchronized` keyword, such as: `public String getDeviceId()` Note that it locks on `this` (the current instance). It's slightly less efficient than your solution (e.g. without complete method locking & read/write concerns), but I'd take the readability and simplicity over the performance. One downside is that if you synchronize on `this`, other methods that are `synchronized` will block on the same value. For that reason, be sure to add a comment to explain why you lock. And, if there is a conflict, you can use the inline syntax and lock on a separate object specific to these methods, e.g. public String ... { synchronized (accountManager) { return ... } } ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/AndroidFxAccount.java:818 (Diff revision 5) > + private void setDeviceRegistrationVersionNoLock(int deviceRegistrationVersion) { > + accountManager.setUserData(account, ACCOUNT_KEY_DEVICE_REGISTRATION_VERSION, > + Integer.toString(deviceRegistrationVersion)); > + } nit: if you use synchronized, the code should be simple enough that you can get rid of these methods.

Comment on attachment 8754044 [details] Bug 1250782 - Update device registration when getting fxaDeviceId if necessary. https://reviewboard.mozilla.org/r/53678/#review54932 I'd like to see this one more time, but it's close. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/AndroidFxAccount.java:812 (Diff revision 5) > + * This is a blocking method even though we make http requests. > + */ > + public String getFreshDeviceIdBlocking() { > + final String cachedDeviceId = getDeviceId(); > + > + if (TextUtils.isEmpty(cachedDeviceId) || nit: I find it much easier to read code that handles the one line case first, e.g.: if (condition) { return thingInOneLine; } // do lots of stuff. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/AndroidFxAccount.java:815 (Diff revision 5) > + final String cachedDeviceId = getDeviceId(); > + > + if (TextUtils.isEmpty(cachedDeviceId) || > + getDeviceRegistrationVersion() < FxAccountDeviceRegistrator.DEVICE_REGISTRATION_VERSION) { > + final String[] newDeviceId = new String[1]; > + final CountDownLatch spinningCallback = new CountDownLatch(1); nit: this isn't really spinning - the thread should be sleeping. I'd just call it a callbackCompleteLatch or similar. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/AndroidFxAccount.java:824 (Diff revision 5) > + try { > + spinningCallback.await(GET_DEVICE_ID_TIMEOUT_MILLIS, TimeUnit.MILLISECONDS); > + } catch (InterruptedException e) { > + return cachedDeviceId; > + } > + return newDeviceId[0]; I think you'd want: boolean completed; while (true) { try { int calculatedTimeLeft = ...; // based on current time, use System.nanoTime completed = await(calculatedTimeLeft, Unit); break; } catch (InterruptedException e) {} } if (ret) { return newDeviceId[0]; } else { return explicitErrorValue; // (e.g. null) }

Comment on attachment 8754035 [details] Bug 1250782 - Allow MozResponse to return a Json array body. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53660/diff/4-5/

Comment on attachment 8754036 [details] Bug 1250782 - ResourceDelegate#handleSuccess now gives access to a JsonArray body. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53662/diff/4-5/

Comment on attachment 8754037 [details] Bug 1250782 - FxAccountClient: add accountStatus method and rename status method to recoveryEmailStatus. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53664/diff/4-5/

Comment on attachment 8754038 [details] Bug 1250782 - FxAccountClient: add registerOrUpdateDevice method. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53666/diff/4-5/

Comment on attachment 8754039 [details] Bug 1250782 - FxAccountClient: add deviceList method. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53668/diff/4-5/

Comment on attachment 8754040 [details] Bug 1250782 - Add FxAccountDeviceRegistrator. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53670/diff/5-6/

Comment on attachment 8754041 [details] Bug 1250782 - Register device on married state. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53672/diff/5-6/

Comment on attachment 8754042 [details] Bug 1250782 - Update device registration on device name change. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53674/diff/5-6/

Comment on attachment 8754043 [details] Bug 1250782 - Include fxaDeviceId in sync client records. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53676/diff/5-6/

Comment on attachment 8754044 [details] Bug 1250782 - Update device registration when getting fxaDeviceId if necessary. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53678/diff/5-6/

Comment on attachment 8757116 [details] Bug 1250782 - FxAccountClient: remove post method unused argument delegate. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55656/diff/3-4/

Patches updated, thanks again Michael!

Comment on attachment 8754040 [details] Bug 1250782 - Add FxAccountDeviceRegistrator. https://reviewboard.mozilla.org/r/53670/#review55744 I'm basically okay with this, but I'm going to leave one without r+ since I want to see the entire series one more time. (At least.) ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:30 (Diff revision 6) > +import java.io.UnsupportedEncodingException; > +import java.security.GeneralSecurityException; > +import java.util.concurrent.ExecutorService; > +import java.util.concurrent.Executors; > + > +public class FxAccountDeviceRegistrator { Class comment explaining what this is and how it interacts with other components of the system, please. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:45 (Diff revision 6) > + private FxAccountDeviceRegistrator() {} > + > + public static void register(final AndroidFxAccount fxAccount, final Context context) { > + register(fxAccount, context, new RegisterDelegate() { > + @Override > + public void onComplete(String deviceId) {} Log, please. Use `Logger.pii` if the device ID is sensitive. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:51 (Diff revision 6) > + }); > + } > + > + public static void register(final AndroidFxAccount fxAccount, final Context context, > + final RegisterDelegate delegate) { > + final byte[] sessionToken = getSessionToken(fxAccount); This needs to be really explicit about what happens when the account is not healthy (needs PW or similar). Right now, you throw an *unchecked* (!) exception all the way out, with no type signature and no documentation. Explain how this works in this case. For the record, you can use the Account Status view to debug the states. Use the menu button to enable "Debug Mode"; from their you can make the account require password or similar. See https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/fxa/activities/FxAccountStatusActivity.java#180. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:150 (Diff revision 6) > + > + @Override > + public void handleSuccess(AccountStatusResponse result) { > + if (!result.exists) { > + Log.i(LOG_TAG, "token invalidated because the account no longer exists"); > + AccountManager.get(context).removeAccount(fxAccount.getAndroidAccount(), null, null); Don't do this; this is tricky to get right. You can have an Android account that corresponds to a deleted FxA; if you delete it automatically, it's basically impossible to interrogate what happened. The way to handle this properly is to add a new State (like Doghouse, but different -- RunAway or Orphaned :)) that corresponds to "I have an Android account, but the FxA is gone." Drive the Android Account to that state, and then update the UI in the status activity to help the user understand. (And provide the option to delete it.) This is non-trivial work; do it as follow-up. For now, just drive the Account state to Doghouse, please. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/AndroidFxAccount.java:795 (Diff revision 6) > + public synchronized int getDeviceRegistrationVersion() { > + String versionStr = accountManager.getUserData(account, ACCOUNT_KEY_DEVICE_REGISTRATION_VERSION); > + if (TextUtils.isEmpty(versionStr)) { > + return 0; > + } else { > + return Integer.parseInt(versionStr); This can fail and throw an unchecked exception. What do you want it to do when it does? I think you want to return 0.

Comment on attachment 8754041 [details] Bug 1250782 - Register device on married state. https://reviewboard.mozilla.org/r/53672/#review55750 I don't think this approach makes sense. We have lots of accounts out in the wild; they need to be registered forward. This should happen during the Account state update state machine, possibly as a final post-Married state. Perhaps. In any case, we need a plan to register devices for existing accounts, and that can't be at the UI interaction point.

Comment on attachment 8754042 [details] Bug 1250782 - Update device registration on device name change. https://reviewboard.mozilla.org/r/53674/#review55752 Again, this isn't the right point to do this. This will fail if the network (or the FxA device service!) isn't available when the client name is changed, which happens in the FxAccountStatusActivity and doesn't require connectivity. Follow instead the pattern that already exists, where we set the "last modified" timestamp and keep trying to upload until we succeed. In Sync, we do that in the tabs uploader, and also in the clients uploader (see https://dxr.mozilla.org/mozilla-central/search?q=getLastModifiedTimestamp). For you, I think you want to bake this into `FxAccountLoginStateMachine` or the "I've advanced the state machine" logic around https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/fxa/sync/FxAccountSyncAdapter.java#539. This looks a lot like updating the profile data -- it's (essentially) optional, after we advance the Account state. That means you'll want to witness client changes and keep a "last uploaded" stamp specific for the device info, and keep trying until you upload after the "last modified" timestamp of the clients data delegate. Hope that makes sense; ask for details if needed!

Comment on attachment 8754043 [details] Bug 1250782 - Include fxaDeviceId in sync client records. https://reviewboard.mozilla.org/r/53676/#review55762 lgtm!

Comment on attachment 8754044 [details] Bug 1250782 - Update device registration when getting fxaDeviceId if necessary. https://reviewboard.mozilla.org/r/53678/#review55764 This is awkward, but basically okay. Clearly mcomella wants otherwise, so I'll defer to him, mostly. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/AndroidFxAccount.java:800 (Diff revision 6) > + /** > + * Returns the cached device id if it's fresh or try to get it from the server if > + * the device registration is stale or the cached device id is empty. > + * This is a blocking method even though we make http requests. > + */ > + public String getFreshDeviceIdBlocking() { This seems mis-named. It's definitely not getting a fresh device ID most of the time (after the initial registration). ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/SyncClientsEngineStage.java:398 (Diff revision 6) > > Context context = session.getContext(); > final Account account = FirefoxAccounts.getFirefoxAccount(context); > if (account != null) { > final AndroidFxAccount fxAccount = new AndroidFxAccount(context, account); > - final String deviceId = fxAccount.getDeviceId(); > + final String deviceId = fxAccount.getFreshDeviceIdBlocking(); // We're in the Sync thread, we can block here I think you're aware, but this will be called frequently -- about once a week, the client will try to upload its client record.

Comment on attachment 8757116 [details] Bug 1250782 - FxAccountClient: remove post method unused argument delegate. https://reviewboard.mozilla.org/r/55656/#review55768 lgtm.

eoger: there's some great work here -- thanks for digging in! Sorry to come in late with some significant restructuring, but c'est la vie. There are some lifecycle things to address here, but it's mostly re-arranging (and re-testing!) rather than rewriting.

Comment on attachment 8754044 [details] Bug 1250782 - Update device registration when getting fxaDeviceId if necessary. https://reviewboard.mozilla.org/r/53678/#review55950 lgtm, thanks Edouard! ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/AndroidFxAccount.java:98 (Diff revision 6) > // Each such token is prefixed with "oauth::" and a service-dependent scope. > // Such tokens should be destroyed when the account is removed from the device. > // This list collects all the known "oauth::" token types in order to delete them when necessary. > private static final List<String> KNOWN_OAUTH_TOKEN_TYPES; > > + private static final int GET_DEVICE_ID_TIMEOUT_MILLIS = 5000; super-nit: Apparently we can specify this in seconds so why not specify it in seconds? :P Don't forget to update the name to include `SECS` OR `SECONDS` accordingly.

Comment on attachment 8754035 [details] Bug 1250782 - Allow MozResponse to return a Json array body. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53660/diff/5-6/

Comment on attachment 8754036 [details] Bug 1250782 - ResourceDelegate#handleSuccess now gives access to a JsonArray body. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53662/diff/5-6/

Comment on attachment 8754037 [details] Bug 1250782 - FxAccountClient: add accountStatus method and rename status method to recoveryEmailStatus. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53664/diff/5-6/

Comment on attachment 8754038 [details] Bug 1250782 - FxAccountClient: add registerOrUpdateDevice method. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53666/diff/5-6/

Comment on attachment 8754039 [details] Bug 1250782 - FxAccountClient: add deviceList method. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53668/diff/5-6/

Comment on attachment 8754040 [details] Bug 1250782 - Add FxAccountDeviceRegistrator. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53670/diff/6-7/

Comment on attachment 8754041 [details] Bug 1250782 - Register device on married state. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53672/diff/6-7/

Comment on attachment 8754042 [details] Bug 1250782 - Update device registration on device name change. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53674/diff/6-7/

Comment on attachment 8754043 [details] Bug 1250782 - Include fxaDeviceId in sync client records. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53676/diff/6-7/

Comment on attachment 8754044 [details] Bug 1250782 - Update device registration when getting fxaDeviceId if necessary. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53678/diff/6-7/

Comment on attachment 8757116 [details] Bug 1250782 - FxAccountClient: remove post method unused argument delegate. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55656/diff/4-5/

Thank you Nick, I changed the way I do the registration: it's now in handleMarried() inside FxAccountSyncAdapter. When changing the device name we just reset the device registration version and ask for a new clients engine sync. I think we could also remove the "Update device registration when getting fxaDeviceId if necessary." commit, but it's okay to leave it there.

Comment on attachment 8754038 [details] Bug 1250782 - FxAccountClient: add registerOrUpdateDevice method. https://reviewboard.mozilla.org/r/53666/#review57998 ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDevice.java:28 (Diff revision 6) > + this.id = id; > + this.type = type; > + this.isCurrentDevice = isCurrentDevice; > + } > + > + public static FxAccountDevice ForRegister(String name, String type) { nit: downcase all these static methods: `forRegister`, `forUpdate`, `fromJSON`. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDevice.java:44 (Diff revision 6) > + String type = json.getString(JSON_KEY_TYPE); > + Boolean isCurrentDevice = json.getBoolean(JSON_KEY_ISCURRENTDEVICE); > + return new FxAccountDevice(name, id, type, isCurrentDevice); > + } > + > + public ExtendedJSONObject json() { nit: consider `toJSON` or `asJSON`; whatever we do most.

Comment on attachment 8754040 [details] Bug 1250782 - Add FxAccountDeviceRegistrator. https://reviewboard.mozilla.org/r/53670/#review58000 I think this is okay. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:218 (Diff revision 7) > + } > + > + @Override > + public void handleSuccess(FxAccountDevice[] devices) { > + for (FxAccountDevice device : devices) { > + if (device.isCurrentDevice) { This can infinite loop, can it not? Include a flag (to the delegate?) for whether you're allowing recursion. Or, don't recurse at all -- just do the right thing the next time we try to advance the account state. (Since you need to handle that already.)

Comment on attachment 8754041 [details] Bug 1250782 - Register device on married state. https://reviewboard.mozilla.org/r/53672/#review58002 OK. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/sync/FxAccountSyncAdapter.java:541 (Diff revision 7) > final SessionCallback sessionCallback = new SessionCallback(syncDelegate, schedulePolicy); > final KeyBundle syncKeyBundle = married.getSyncKeyBundle(); > final String clientState = married.getClientState(); > syncWithAssertion(audience, assertion, tokenServerEndpointURI, tokenBackoffHandler, sharedPrefs, syncKeyBundle, clientState, sessionCallback, extras, fxAccount); > > + // Register the device if necessary Probably worth saying this spawns a new thread and isn't synchronous. (Consider updating the profile avatar comment to say the same.) ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/sync/FxAccountSyncAdapter.java:542 (Diff revision 7) > final KeyBundle syncKeyBundle = married.getSyncKeyBundle(); > final String clientState = married.getClientState(); > syncWithAssertion(audience, assertion, tokenServerEndpointURI, tokenBackoffHandler, sharedPrefs, syncKeyBundle, clientState, sessionCallback, extras, fxAccount); > > + // Register the device if necessary > + if(fxAccount.getDeviceRegistrationVersion() != FxAccountDeviceRegistrator.DEVICE_REGISTRATION_VERSION nit: `if (` with space.

Comment on attachment 8754042 [details] Bug 1250782 - Update device registration on device name change. https://reviewboard.mozilla.org/r/53674/#review58004 This isn't right. The client data delegate shouldn't be triggering a Sync immediately. Do this in the FxAccountStatusActivity, which is the only place that this gets changed now. If the registration process itself can change the local device name, persist it to the shared prefs and process it next sync, like we currently do with offline changes.

Comment on attachment 8754043 [details] Bug 1250782 - Include fxaDeviceId in sync client records. https://reviewboard.mozilla.org/r/53676/#review58006

Comment on attachment 8754044 [details] Bug 1250782 - Update device registration when getting fxaDeviceId if necessary. https://reviewboard.mozilla.org/r/53678/#review58008

https://reviewboard.mozilla.org/r/53664/#review58010 I find the `DEVICE_REGISTRATION_VERSION` checks odd. You're using the version as a flag. This doesn't seem future proof -- what happens if something gets confused and you suddenly have a version 2 floating around? Everything will fail. If you really want a flag, use `> 0`. Otherwise, actually implement versions incrementing over time. Or explain why this makes sense...

Comment on attachment 8754035 [details] Bug 1250782 - Allow MozResponse to return a Json array body. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53660/diff/6-7/

Comment on attachment 8754036 [details] Bug 1250782 - ResourceDelegate#handleSuccess now gives access to a JsonArray body. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53662/diff/6-7/

Comment on attachment 8754037 [details] Bug 1250782 - FxAccountClient: add accountStatus method and rename status method to recoveryEmailStatus. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53664/diff/6-7/

Comment on attachment 8754038 [details] Bug 1250782 - FxAccountClient: add registerOrUpdateDevice method. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53666/diff/6-7/

Comment on attachment 8754039 [details] Bug 1250782 - FxAccountClient: add deviceList method. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53668/diff/6-7/

Comment on attachment 8754040 [details] Bug 1250782 - Add FxAccountDeviceRegistrator. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53670/diff/7-8/

Comment on attachment 8754041 [details] Bug 1250782 - Register device on married state. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53672/diff/7-8/

Comment on attachment 8754042 [details] Bug 1250782 - Update device registration on device name change. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53674/diff/7-8/

Comment on attachment 8754043 [details] Bug 1250782 - Include fxaDeviceId in sync client records. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53676/diff/7-8/

Comment on attachment 8754044 [details] Bug 1250782 - Update device registration when getting fxaDeviceId if necessary. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53678/diff/7-8/

Comment on attachment 8757116 [details] Bug 1250782 - FxAccountClient: remove post method unused argument delegate. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55656/diff/5-6/

Comment on attachment 8754040 [details] Bug 1250782 - Add FxAccountDeviceRegistrator. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53670/diff/8-9/

Comment on attachment 8754041 [details] Bug 1250782 - Register device on married state. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53672/diff/8-9/

Comment on attachment 8754042 [details] Bug 1250782 - Update device registration on device name change. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53674/diff/8-9/

Comment on attachment 8754043 [details] Bug 1250782 - Include fxaDeviceId in sync client records. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53676/diff/8-9/

Comment on attachment 8754044 [details] Bug 1250782 - Update device registration when getting fxaDeviceId if necessary. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53678/diff/8-9/

Comment on attachment 8757116 [details] Bug 1250782 - FxAccountClient: remove post method unused argument delegate. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55656/diff/6-7/

Comment on attachment 8754040 [details] Bug 1250782 - Add FxAccountDeviceRegistrator. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53670/diff/9-10/

Comment on attachment 8754041 [details] Bug 1250782 - Register device on married state. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53672/diff/9-10/

Comment on attachment 8754042 [details] Bug 1250782 - Update device registration on device name change. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53674/diff/9-10/

Comment on attachment 8754043 [details] Bug 1250782 - Include fxaDeviceId in sync client records. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53676/diff/9-10/

Comment on attachment 8757116 [details] Bug 1250782 - FxAccountClient: remove post method unused argument delegate. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55656/diff/7-8/

... I wish Bugzilla collapsed all these reviewboard comments Deleted the "Update device registration when getting fxaDeviceId if necessary" commit, this was too convoluted and we're gonna register the device on the state machine transition anyway. I think we're ready :)

Comment on attachment 8754037 [details] Bug 1250782 - FxAccountClient: add accountStatus method and rename status method to recoveryEmailStatus. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53664/diff/7-8/

Comment on attachment 8754038 [details] Bug 1250782 - FxAccountClient: add registerOrUpdateDevice method. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53666/diff/7-8/

Comment on attachment 8754039 [details] Bug 1250782 - FxAccountClient: add deviceList method. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53668/diff/7-8/

Comment on attachment 8754040 [details] Bug 1250782 - Add FxAccountDeviceRegistrator. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53670/diff/10-11/

Comment on attachment 8754041 [details] Bug 1250782 - Register device on married state. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53672/diff/10-11/

Comment on attachment 8754042 [details] Bug 1250782 - Update device registration on device name change. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53674/diff/10-11/

Comment on attachment 8754043 [details] Bug 1250782 - Include fxaDeviceId in sync client records. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53676/diff/10-11/

Comment on attachment 8757116 [details] Bug 1250782 - FxAccountClient: remove post method unused argument delegate. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55656/diff/8-9/

Comment on attachment 8754037 [details] Bug 1250782 - FxAccountClient: add accountStatus method and rename status method to recoveryEmailStatus. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53664/diff/8-9/

Comment on attachment 8754038 [details] Bug 1250782 - FxAccountClient: add registerOrUpdateDevice method. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53666/diff/8-9/

Comment on attachment 8754039 [details] Bug 1250782 - FxAccountClient: add deviceList method. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53668/diff/8-9/

Comment on attachment 8754040 [details] Bug 1250782 - Add FxAccountDeviceRegistrator. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53670/diff/11-12/

Comment on attachment 8754041 [details] Bug 1250782 - Register device on married state. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53672/diff/11-12/

Comment on attachment 8754042 [details] Bug 1250782 - Update device registration on device name change. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53674/diff/11-12/

Comment on attachment 8754043 [details] Bug 1250782 - Include fxaDeviceId in sync client records. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53676/diff/11-12/

Comment on attachment 8757116 [details] Bug 1250782 - FxAccountClient: remove post method unused argument delegate. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55656/diff/9-10/

Comment on attachment 8754042 [details] Bug 1250782 - Update device registration on device name change. https://reviewboard.mozilla.org/r/53674/#review58430 ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/activities/FxAccountStatusFragment.java:944 (Diff revision 12) > if (TextUtils.isEmpty(newClientName)) { > newClientName = clientsDataDelegate.getDefaultClientName(); > } > final long now = System.currentTimeMillis(); > clientsDataDelegate.setClientName(newClientName, now); > - requestDelayedSync(); // Try to update our remote client record. > + fxAccount.requestImmediateSync(STAGES_TO_SYNC_ON_DEVICE_NAME_CHANGE, null); // Try to update our remote client record. There's no advantage to forcing here. Also, immediate sync with just "clients" is not the same as a full sync. Just drop the changes to this file.

eoger: this is looking good! I'll let you clear review requests, etc, as needed. In general, I think you're going to want to add logging throughout. When I tested this, I saw no indication of success at all; I had to use the debugger to inspect the state of the account. It's worth being chatty, since it's essentially impossible to debug without verbose logging. Use Logger, and don't leak PII (personally identifying information)! Also, it's worth using Logger.pii in general, and also worth adding the new device registration information to the debug/dump option in the status activity. If you trace it back, you'll need to add it around https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/AndroidFxAccount.java?q=path%3AAndroidFxAcc&redirect_type=single#379. Land when you're ready.

Comment on attachment 8754035 [details] Bug 1250782 - Allow MozResponse to return a Json array body. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53660/diff/7-8/

Comment on attachment 8754036 [details] Bug 1250782 - ResourceDelegate#handleSuccess now gives access to a JsonArray body. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53662/diff/7-8/

Comment on attachment 8754037 [details] Bug 1250782 - FxAccountClient: add accountStatus method and rename status method to recoveryEmailStatus. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53664/diff/9-10/

Comment on attachment 8754038 [details] Bug 1250782 - FxAccountClient: add registerOrUpdateDevice method. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53666/diff/9-10/

Comment on attachment 8754039 [details] Bug 1250782 - FxAccountClient: add deviceList method. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53668/diff/9-10/

Comment on attachment 8754040 [details] Bug 1250782 - Add FxAccountDeviceRegistrator. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53670/diff/12-13/

Comment on attachment 8754041 [details] Bug 1250782 - Register device on married state. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53672/diff/12-13/

Comment on attachment 8754042 [details] Bug 1250782 - Update device registration on device name change. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53674/diff/12-13/

Comment on attachment 8754043 [details] Bug 1250782 - Include fxaDeviceId in sync client records. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53676/diff/12-13/

Comment on attachment 8757116 [details] Bug 1250782 - FxAccountClient: remove post method unused argument delegate. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55656/diff/10-11/

Review commit: https://reviewboard.mozilla.org/r/61606/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/61606/

Comment on attachment 8766891 [details] Bug 1250782 - Add FxA registration details to the debug_dump command. https://reviewboard.mozilla.org/r/61606/#review58456 wfm. ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/AndroidFxAccount.java:394 (Diff revision 1) > o.put("emailUTF8", Utils.byte2Hex(account.name.getBytes("UTF-8"))); > } catch (UnsupportedEncodingException e) { > // Ignore. > } > + o.put("fxaDeviceId", getDeviceId()); > + o.put("deviceRegistrationVersion", getDeviceRegistrationVersion()); maybe "fxaDeviceRegistration...", just so the prefix is the same?

Comment on attachment 8766891 [details] Bug 1250782 - Add FxA registration details to the debug_dump command. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61606/diff/1-2/

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/fx-team/rev/ec25edc09a60 Allow MozResponse to return a Json array body. r=mcomella https://hg.mozilla.org/integration/fx-team/rev/346623206d14 ResourceDelegate#handleSuccess now gives access to a JsonArray body. r=mcomella https://hg.mozilla.org/integration/fx-team/rev/985cadcc9ed5 FxAccountClient: add accountStatus method and rename status method to recoveryEmailStatus. r=mcomella https://hg.mozilla.org/integration/fx-team/rev/17541521c2ec FxAccountClient: add registerOrUpdateDevice method. r=mcomella https://hg.mozilla.org/integration/fx-team/rev/c8020e8d970e FxAccountClient: add deviceList method. r=mcomella https://hg.mozilla.org/integration/fx-team/rev/28cff660f4e6 Add FxAccountDeviceRegistrator. r=mcomella, nalexander https://hg.mozilla.org/integration/fx-team/rev/bbc96e84aec0 Register device on married state. r=mcomella, nalexander https://hg.mozilla.org/integration/fx-team/rev/2b40afc36569 Update device registration on device name change. r=mcomella, nalexander https://hg.mozilla.org/integration/fx-team/rev/5013e7bb1c9f Include fxaDeviceId in sync client records. r=mcomella, nalexander https://hg.mozilla.org/integration/fx-team/rev/5ee8820711c1 FxAccountClient: remove post method unused argument delegate. r=eoger https://hg.mozilla.org/integration/fx-team/rev/f60d8a9c1a17 Add FxA registration details to the debug_dump command. r=nalexander

Ryan can we have a dashboard to see if registrations are being made as excepted?

https://hg.mozilla.org/mozilla-central/rev/ec25edc09a60 https://hg.mozilla.org/mozilla-central/rev/346623206d14 https://hg.mozilla.org/mozilla-central/rev/985cadcc9ed5 https://hg.mozilla.org/mozilla-central/rev/17541521c2ec https://hg.mozilla.org/mozilla-central/rev/c8020e8d970e https://hg.mozilla.org/mozilla-central/rev/28cff660f4e6 https://hg.mozilla.org/mozilla-central/rev/bbc96e84aec0 https://hg.mozilla.org/mozilla-central/rev/2b40afc36569 https://hg.mozilla.org/mozilla-central/rev/5013e7bb1c9f https://hg.mozilla.org/mozilla-central/rev/5ee8820711c1 https://hg.mozilla.org/mozilla-central/rev/f60d8a9c1a17

Here's a kibana dashboard for /account/device requests from Android: https://kibana.fxa.us-west-2.prod.mozaws.net/index.html#/dashboard/temp/AVXYMhZ1ayLd6KaGMV_b If you zoom out to the 7-day view, you can see an increase in activity corresponding to this landing in Nightly, which is an encouraging start.