1346061 - Register for push notifications from FxA before account is verified

Status: RESOLVED FIXED

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

Description

[:Grisha Kruglov]

Implementation details to come.

Doing this will unblock a lot of good UX:
- push-driven "verified" state
- start sync as soon as account/login is verified
- send tabs will start working sooner

Comment 1

[Edouard Oger [:eoger]]

Attachment: Bug 1346061 part 1 - Expose getSessionToken() on State.

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

Comment 2

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

Comment on attachment 8849271 [details]
Bug 1346061 part 1 - Expose getSessionToken() on State.

https://reviewboard.mozilla.org/r/122088/#review124114

This looks like a good first cut, but I think there's an error in where the state machine is plumbed.  I expect you'll need to keep at least some of the `Married` logic in place, if not all of it.  If you need both parts of the functionality in both states, extract a helper and invoke both.  Also, be sure that you don't accidentally re-register unnecessarily if you get from `Engaged` through to `Married` in one shot.  (I expect your version and timestamp checking will ensure this just works, but there's asynchronous code in play...)

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountPushHandler.java:66
(Diff revision 1)
>      }
>  
> +    private static void handleVerification(Context context) {
> +        AndroidFxAccount fxAccount = AndroidFxAccount.fromContext(context);
> +        if (fxAccount == null) {
> +            Log.e(LOG_TAG, "The account does not exist anymore");

nit: the _Android_ account does not exist anymore.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountPushHandler.java:69
(Diff revision 1)
> +        AndroidFxAccount fxAccount = AndroidFxAccount.fromContext(context);
> +        if (fxAccount == null) {
> +            Log.e(LOG_TAG, "The account does not exist anymore");
> +            return;
> +        }
> +        fxAccount.requestImmediateSync(null, null); // This will trigger verification check and a sync

nit: full stop at the end.  (We always use full sentence comments in this code base.)

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/AndroidFxAccount.java:617
(Diff revision 1)
>    }
>  
>    public byte[] getSessionToken() throws InvalidFxAState {
>      State state = getState();
>      StateLabel stateLabel = state.getStateLabel();
>      if (stateLabel == StateLabel.Cohabiting || stateLabel == StateLabel.Married) {

If grisha agrees, let's expose `getSessionToken` on all states, and return `null` if it's not available.  You'll need to ensure consumers do the right thing when they get `null`.  This'll prevent some of this casting.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/AndroidFxAccount.java:623
(Diff revision 1)
>        TokensAndKeysState tokensAndKeysState = (TokensAndKeysState) state;
>        return tokensAndKeysState.getSessionToken();
> +    } else if (stateLabel == StateLabel.Engaged) {
> +      return ((Engaged) state).getSessionToken();
>      }
> -    throw new InvalidFxAState("Cannot get sessionToken: not in a TokensAndKeysState state");
> +    throw new InvalidFxAState("Cannot get sessionToken: not in a state with a session token");

Maybe include the `stateLabel` in this message to aid debugging.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/sync/FxAccountSyncAdapter.java:530
(Diff revision 1)
>          private boolean shouldRequestToken(final BackoffHandler tokenBackoffHandler, final Bundle extras) {
>            return shouldPerformSync(tokenBackoffHandler, "token", extras);
>          }
>  
>          @Override
> +        public void handleEngaged(Engaged state) {

This changes the semantics in a way I don't think you intend.  Referring to https://people-mozilla.org/~nalexander/FxA_client_states.pdf (which I believe is still correct for Android; for iOS, we split some of the states in fairly obvious ways) you'll see that you expect to get to `Married` every sync, but you expect to get to `Engaged` only when you've _lost_ the `sessionToken` in some way and -- generally by having the remote password change underneath you -- and recovered it -- generally by having the user sign in again on this device.

So, as written, you'll not execute this code as frequently as you were before, which I think is not what you wanted, since you're handling your period registration renewal here.

Comment 3

[Edouard Oger [:eoger]]

Comment on attachment 8849271 [details]
Bug 1346061 part 1 - Expose getSessionToken() on State.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/122088/diff/1-2/

Comment 4

[Edouard Oger [:eoger]]

Thank you Nick, I addressed your comments.
Specifically, I am now doing the device registration in two places (Married and Engaged), better safe than sorry.
I also added a simple lock mechanism to avoid making 2 concurrent requests.

Comment 5

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

Comment on attachment 8849271 [details]
Bug 1346061 part 1 - Expose getSessionToken() on State.

https://reviewboard.mozilla.org/r/122088/#review124672

Sorry to make you loop on this again, but there's a simpler, more robust way to achieve this.  You might need to make the executor service static or longer lived.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:63
(Diff revision 2)
>    // devices after we update what we send on device registration.
>    public static final Integer DEVICE_REGISTRATION_VERSION = 2;
>  
>    private static FxAccountDeviceRegistrator instance;
>    private final WeakReference<Context> context;
> +  private final ExpiringLock lock = new ExpiringLock(LOCK_EXPIRE_MS);

This assumes that there is only one `FxAccountDeviceRegistrator`, which it appears is the case since you have a singleton pattern in place.  I'd still consider making the lock static in order to be really clear about the expectations here.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:185
(Diff revision 2)
>        Log.i(LOG_TAG, "Attempting registration for an existing device");
>        Logger.pii(LOG_TAG, "Device ID: " + deviceId);
>        device = FxAccountDevice.forUpdate(deviceId, clientName, pushCallback, pushPublicKey, pushAuthKey);
>      }
>  
>      ExecutorService executor = Executors.newSingleThreadExecutor(); // Not called often, it's okay to spawn another thread

This lock/unlock is fragile.  You already have a much more robust way to make your requests sequentially and ensure they're non-overlapping, the executor here.  (See https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/Executors.html#newSingleThreadExecutor() for notes.)

So just always submit a new task, but make your task check to see if it's still needed (timestamp, version, whatever).  Then if you succeed in `Engaged` you just don't do anything in `Married`; if you fail in `Engaged`, you'll retry in `Married`, but that's okay I think.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:217
(Diff revision 2)
>                  && error.apiErrorNumber == FxAccountRemoteError.INVALID_AUTHENTICATION_TOKEN) {
>            handleTokenError(error, fxAccountClient, fxAccount);
>          } else {
>            logErrorAndResetDeviceRegistrationVersionAndTimestamp(error, fxAccount);
>          }
> +        lock.unlock();

I'd feel more confident if you had a `finally` block that unlocked.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:352
(Diff revision 2)
> +    public ExpiringLock(long timeout) {
> +      this.timeout = timeout;
> +    }
> +
> +    private synchronized boolean tryGetLock() {
> +      long now = new Date().getTime();

We generally use `System.currentTimeMillis()` for such timestamps.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/login/Doghouse.java:27
(Diff revision 2)
>  
>    @Override
>    public Action getNeededAction() {
>      return Action.NeedsUpgrade;
>    }
> +

Extract this `sessionToken` bit to a Pre: patch, please.

Comment 6

[Edouard Oger [:eoger]]

From my conversation on IRC with nalexander:

I think using the Executor is a good idea, however response callbacks are executed in a different thread [0][1].
My problem with this is that if we post a new device registration while another one is already in flight, the Executor will first call the new-device-registration Runnable before executing the Runnable callbacks (setting DEVICE_REGISTRATION_VERSION, used to determine if we start a registration or not..).

We came up with 3 solutions:

(1) I proposed to allow the FxAccountClient20 constructor to accept a null Executor, and execute the callbacks in the current thread instead of posting a new Runnable. We would still maybe have multiple concurrent Push registrations, but the device registration would be completely sequential.

(2) Nick proposed we could use a PriorityBlockingQueue inside the ExecutorService, and have the callbacks have a higher priority than the registration start.

(3) We could also peek at the previous stages of the FxA state machine (stateLabelsSeen), and don't trigger a device registration on Married when we already went through Engaged.


[0]: https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/background/fxa/FxAccountClient20.java#270-286
[1]: https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java#201-203

Comment 7

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

(In reply to Edouard Oger [:eoger] from comment #6)
> From my conversation on IRC with nalexander:
> 
> I think using the Executor is a good idea, however response callbacks are
> executed in a different thread [0][1].
> My problem with this is that if we post a new device registration while
> another one is already in flight, the Executor will first call the
> new-device-registration Runnable before executing the Runnable callbacks
> (setting DEVICE_REGISTRATION_VERSION, used to determine if we start a
> registration or not..).
> 
> We came up with 3 solutions:
> 
> (1) I proposed to allow the FxAccountClient20 constructor to accept a null
> Executor, and execute the callbacks in the current thread instead of posting
> a new Runnable. We would still maybe have multiple concurrent Push
> registrations, but the device registration would be completely sequential.
> 
> (2) Nick proposed we could use a PriorityBlockingQueue inside the
> ExecutorService, and have the callbacks have a higher priority than the
> registration start.
> 
> (3) We could also peek at the previous stages of the FxA state machine
> (stateLabelsSeen), and don't trigger a device registration on Married when
> we already went through Engaged.

I prefer (3), 'cuz it's simple and uses "local" information (rather than a "global" lock).

I'd be happy with (1) as well, although this opens you up to races when updating the FxA information locally (which you should have already protected against).  If you prefer this, use https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/sync/Utils.java#568 rather than allowing the ExecutorService to be null.

Comment 8

[:Grisha Kruglov]

Comment on attachment 8849271 [details]
Bug 1346061 part 1 - Expose getSessionToken() on State.

https://reviewboard.mozilla.org/r/122088/#review125036

Apologies for a late review. Some quick thoughts below, and i'll jot some more notes soon.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountPushHandler.java:41
(Diff revision 2)
>              }
>          }
>          if (message == null) {
>              // An empty body means we should check the verification state of the account (FxA sends this
>              // when the account email is verified for example).
> -            // TODO: We're only registering the push endpoint when we are in the Married state, that's why we're skipping the message :(
> +            handleVerification(context);

If you're going to split the patch up for readability, this bit would make a good "Part 2".

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountPushHandler.java:48
(Diff revision 2)
>          }
>          try {
>              String command = message.getString("command");
>              JSONObject data = message.getJSONObject("data");
>              switch (command) {
>                  case COMMAND_DEVICE_DISCONNECTED:

From the handler implementation, this command seems to be "your device disconnected", telling us to remove our account.


There's a "deviceConnected" event as well - I'm not sure if that might include "your device connected", and what "connected" means - verified? or just "logged-in"?:
https://github.com/mozilla/fxa-auth-server/blob/master/docs/pushpayloads.schema.json#L14

Is that something you might want to handle similarly to "empty push message", requesting a verification (that is, requesting a sync)? Either as Part 2 or as a follow-up.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/login/Doghouse.java:27
(Diff revision 2)
>  
>    @Override
>    public Action getNeededAction() {
>      return Action.NeedsUpgrade;
>    }
> +

+1

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/sync/FxAccountSyncAdapter.java:546
(Diff revision 2)
>          private boolean shouldRequestToken(final BackoffHandler tokenBackoffHandler, final Bundle extras) {
>            return shouldPerformSync(tokenBackoffHandler, "token", extras);
>          }
>  
>          @Override
> +        public void handleEngaged(Engaged state) {

If you're unverified, you will end up in an Engaged state as you pump through the state machine, correct?

The `handleFinal` handler will then call either `handleMarried` or `handleNotMarried`. Couldn't you instead of performing registration in `handleEngaged` do it as a a side-effect of the `handleFinal` IF you are in an Engaged state at the end?

Unless I'm missing something this should eliminate the "we're going to try and register twice in a row" case.

Comment 9

[:Grisha Kruglov]

Comment on attachment 8849271 [details]
Bug 1346061 part 1 - Expose getSessionToken() on State.

https://reviewboard.mozilla.org/r/122088/#review125118

Going to clear the flag for now, and wait for a simpler version of this which does registration at the end of the state machine transitions (unless there's a great reason we somehow can't do that).

Comment 10

[Edouard Oger [:eoger]]

Attachment: Bug 1346061 part 3 - Trigger a sync/email-verification-check on empty FxA push message.

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

Comment 11

[Edouard Oger [:eoger]]

Attachment: Bug 1346061 part 2 - Register device on FxA on Engaged state.

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

Comment 12

[Edouard Oger [:eoger]]

Comment on attachment 8849271 [details]
Bug 1346061 part 1 - Expose getSessionToken() on State.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/122088/diff/2-3/

Comment 13

[:Grisha Kruglov]

Comment on attachment 8850607 [details]
Bug 1346061 part 2 - Register device on FxA on Engaged state.

https://reviewboard.mozilla.org/r/123142/#review125578

I think this is fine. I'll try it out to see what the new experience feels like.

::: commit-message-cd130:1
(Diff revision 1)
> +Bug 1346061 part 3 - Register device on FxA on Engaged state. r?nalexander, grisha

Complete nit, but I would have made this part 2, and your part 2 would have been part 3.

Comment 14

[:Grisha Kruglov]

Comment on attachment 8849271 [details]
Bug 1346061 part 1 - Expose getSessionToken() on State.

https://reviewboard.mozilla.org/r/122088/#review125584

Why do you still need this?

Comment 15

[:Grisha Kruglov]

Comment on attachment 8850606 [details]
Bug 1346061 part 3 - Trigger a sync/email-verification-check on empty FxA push message.

https://reviewboard.mozilla.org/r/123140/#review125588

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountPushHandler.java:40
(Diff revision 1)
>                  return;
>              }
>          }
>          if (message == null) {
>              // An empty body means we should check the verification state of the account (FxA sends this
>              // when the account email is verified for example).

Can you please link to server docs, API docs, code, or anything else that definitely describes this?

Comment 16

[Edouard Oger [:eoger]]

> Why do you still need this?

We're still getting a session token on Engaged state, see nalexander's comment 2

Comment 17

[Edouard Oger [:eoger]]

Comment on attachment 8849271 [details]
Bug 1346061 part 1 - Expose getSessionToken() on State.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/122088/diff/3-4/

Comment 18

[Edouard Oger [:eoger]]

Comment on attachment 8850607 [details]
Bug 1346061 part 2 - Register device on FxA on Engaged state.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123142/diff/1-2/

Comment 19

[Edouard Oger [:eoger]]

Comment on attachment 8850606 [details]
Bug 1346061 part 3 - Trigger a sync/email-verification-check on empty FxA push message.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123140/diff/1-2/

Comment 20

[:Grisha Kruglov]

This seems to work pretty well in local testing! The beginnings of Push driven sync :-)

Comment 21

[:Grisha Kruglov]

Comment on attachment 8850606 [details]
Bug 1346061 part 3 - Trigger a sync/email-verification-check on empty FxA push message.

https://reviewboard.mozilla.org/r/123140/#review125652

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountPushHandler.java:70
(Diff revision 2)
> +        AndroidFxAccount fxAccount = AndroidFxAccount.fromContext(context);
> +        if (fxAccount == null) {
> +            Log.e(LOG_TAG, "The Android account does not exist anymore");
> +            return;
> +        }
> +        // This will trigger an email verification check and a sync.

I'd add a Log.i statemenet here, something like "Received 'accountVerified' push event, requesting immediate sync".

For completeness as you're looking at logs.

Comment 22

[:Grisha Kruglov]

Comment on attachment 8849271 [details]
Bug 1346061 part 1 - Expose getSessionToken() on State.

https://reviewboard.mozilla.org/r/122088/#review125654

Is there a good reason we would ever try to obtain a sessionToken from sessions which are not one of Married, Engaged and, I guess, Cohabitating? Trying to get a sessionToken when you're in Separated or Doghouse state seems like a programming error to me, something is either off with our state machine flow or with how we're treating its final state. So I suggest you still throw in those getters, or throw by default in `getSessionToken` method in State base class.

::: commit-message-05bfa:1
(Diff revision 4)
> +Bug 1346061 part 1 - Expose getSessionToken() on State. r?nalexander

Describe the "why" as well as the "what". Specifically, say that we'll need access to sessionToken while we're in the Engaged state in order to perform device registration, and so we're exposing it here as a pre-requisite for further work.

Comment 23

[:Grisha Kruglov]

Comment on attachment 8849271 [details]
Bug 1346061 part 1 - Expose getSessionToken() on State.

https://reviewboard.mozilla.org/r/122088/#review125656

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/login/State.java:76
(Diff revision 4)
>    public abstract void execute(ExecuteDelegate delegate);
>  
>    public abstract Action getNeededAction();
> +
> +  @Nullable
> +  public abstract byte[] getSessionToken();

Perhaps this should throw. See my other comment.

Comment 24

[Edouard Oger [:eoger]]

Comment on attachment 8849271 [details]
Bug 1346061 part 1 - Expose getSessionToken() on State.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/122088/diff/4-5/

Comment 25

[Edouard Oger [:eoger]]

Comment on attachment 8850607 [details]
Bug 1346061 part 2 - Register device on FxA on Engaged state.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123142/diff/2-3/

Comment 26

[Edouard Oger [:eoger]]

Comment on attachment 8850606 [details]
Bug 1346061 part 3 - Trigger a sync/email-verification-check on empty FxA push message.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123140/diff/2-3/

Comment 27

[Edouard Oger [:eoger]]

Thanks Grisha, I made the method throw by default

Comment 28

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

Comment on attachment 8849271 [details]
Bug 1346061 part 1 - Expose getSessionToken() on State.

https://reviewboard.mozilla.org/r/122088/#review126514

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/login/MigratedFromSync11.java:34
(Diff revision 5)
>    }
> +
> +  @Nullable
> +  @Override
> +  public byte[] getSessionToken() {
> +    return null;

Shouldn't this throw, like the other place?

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/SyncClientsEngineStage.java:195
(Diff revision 5)
>          return;
>        }
>        final AndroidFxAccount fxAccount = new AndroidFxAccount(context, account);
>        final ExtendedJSONObject payload = createNotifyDevicesPayload();
>  
> -      final byte[] sessionToken;
> +      final byte[] sessionToken = fxAccount.getState().getSessionToken();

Why don't you need to catch exceptions here?

Are you assuming that the state is `Married`, since you're syncing?  This should be true, but there may be races in the system -- things can happen (like debug actions in the FxAccountStatusActivity that force the account into different states) while a Sync is ongoing!

Or are you assuming that unhandled exceptions will be caught outside if this method?

`IllegalStateException` is unchecked so the compiler won't tell you statically that you're not catching it.

Comment 29

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

Comment on attachment 8849271 [details]
Bug 1346061 part 1 - Expose getSessionToken() on State.

https://reviewboard.mozilla.org/r/122088/#review126514

I'm pretty sure you've introduced new uncaught exception flows in this patch.  You'd be safer to just re-use/re-purpose the illegal state exception, which is checked and would reveal many of these things statically.

If I'm mis-reading or missing things, please comment (or add code comments!) explaining why you don't need to catch these unchecked exceptions.

Comment 30

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

Comment on attachment 8850607 [details]
Bug 1346061 part 2 - Register device on FxA on Engaged state.

https://reviewboard.mozilla.org/r/123142/#review126518

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/sync/FxAccountSyncAdapter.java:549
(Diff revision 3)
>          @Override
>          public void handleNotMarried(State notMarried) {
>            Logger.info(LOG_TAG, "handleNotMarried: in " + notMarried.getStateLabel());
>            schedulePolicy.onHandleFinal(notMarried.getNeededAction());
>            syncDelegate.handleCannotSync(notMarried);
> +          if (notMarried.getStateLabel() == StateLabel.Engaged) {

This might be more pleasant if you handled it before branching on the `StateLabel`, around https://dxr.mozilla.org/mozilla-central/rev/e03e0c60462c775c7558a1dc9d5cf2076c3cd1f9/mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/FxAccountAuthenticator.java#172.

If you do that, the `null` session token might make more sense than raising an exception.  I leave this choice to you (and possibly Grisha, since he suggested the exception).

Comment 31

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

Comment on attachment 8850606 [details]
Bug 1346061 part 3 - Trigger a sync/email-verification-check on empty FxA push message.

https://reviewboard.mozilla.org/r/123140/#review126520

lgtm.

Comment 32

[Edouard Oger [:eoger]]

Comment on attachment 8849271 [details]
Bug 1346061 part 1 - Expose getSessionToken() on State.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/122088/diff/5-6/

Comment 33

[Edouard Oger [:eoger]]

Comment on attachment 8850607 [details]
Bug 1346061 part 2 - Register device on FxA on Engaged state.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123142/diff/3-4/

Comment 34

[Edouard Oger [:eoger]]

Comment on attachment 8850606 [details]
Bug 1346061 part 3 - Trigger a sync/email-verification-check on empty FxA push message.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123140/diff/3-4/

Comment 35

[Edouard Oger [:eoger]]

Comment on attachment 8849271 [details]
Bug 1346061 part 1 - Expose getSessionToken() on State.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/122088/diff/6-7/

Comment 36

[Edouard Oger [:eoger]]

Comment on attachment 8850607 [details]
Bug 1346061 part 2 - Register device on FxA on Engaged state.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123142/diff/4-5/

Comment 37

[Edouard Oger [:eoger]]

Comment on attachment 8850606 [details]
Bug 1346061 part 3 - Trigger a sync/email-verification-check on empty FxA push message.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123140/diff/4-5/

Comment 38

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

Comment on attachment 8849271 [details]
Bug 1346061 part 1 - Expose getSessionToken() on State.

https://reviewboard.mozilla.org/r/122088/#review126848

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/login/State.java:17
(Diff revisions 5 - 7)
>    public static final long CURRENT_VERSION = 3L;
>  
> +  public class NotASessionTokenState extends Exception {
> +
> +    private static final long serialVersionUID = 8628129091996684799L;
> +    

nit: whitespace.

Comment 39

[Edouard Oger [:eoger]]

Comment on attachment 8849271 [details]
Bug 1346061 part 1 - Expose getSessionToken() on State.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/122088/diff/7-8/

Comment 40

[Edouard Oger [:eoger]]

Comment on attachment 8850607 [details]
Bug 1346061 part 2 - Register device on FxA on Engaged state.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123142/diff/5-6/

Comment 41

[Edouard Oger [:eoger]]

Comment on attachment 8850606 [details]
Bug 1346061 part 3 - Trigger a sync/email-verification-check on empty FxA push message.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123140/diff/5-6/

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0d14c27bb279
part 1 - Expose getSessionToken() on State. r=Grisha,nalexander
https://hg.mozilla.org/integration/autoland/rev/5602cad1c4a1
part 2 - Register device on FxA on Engaged state. r=Grisha,nalexander
https://hg.mozilla.org/integration/autoland/rev/9b0e58ec80cb
part 3 - Trigger a sync/email-verification-check on empty FxA push message. r=Grisha,nalexander

Comment 43

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0d14c27bb279
https://hg.mozilla.org/mozilla-central/rev/5602cad1c4a1
https://hg.mozilla.org/mozilla-central/rev/9b0e58ec80cb